[CT-1584] New top level commands: interactive compile #7008

aranke · 2023-02-19T09:24:52Z

resolves #6358

Description

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
- Document interactive compile docs.getdbt.com#2994
I have run changie new to create a changelog entry

github-actions · 2023-02-19T09:25:06Z

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

ChenyuLInx

Looks great! Thanks for traversing through the codebase and add this feature!

I did run into one issue that running dbt compile -s customers(I have a model named that), compiled result was not logged.

The rest of the comments are mostly informational/confirm things.

ChenyuLInx · 2023-03-07T16:37:57Z

core/dbt/cli/flags.py

@@ -42,6 +43,7 @@
    "fail_fast",
    "indirect_selection",
    "store_failures",
+    "introspect",


Why we want to add introspect here?

I did it to help a test pass, but it might not be required.

Update: removing this doesn't help, so I'll leave it as-is for now.

Do you mean removing it would cause things to fail? This doesn't feel right given that introspect is only a parameter for compile task command, and all parameter in this list is defiled both as cli click group level and a task command level.

Punted into a new ticket: #7156

ChenyuLInx · 2023-03-07T16:40:27Z

core/dbt/cli/flags.py

@@ -29,6 +29,7 @@
    "FULL_REFRESH": False,
    "STRICT_MODE": False,
    "STORE_FAILURES": False,
+    "INTROSPECT": True,


I think we want to figure out a way to remove this section at some point. Just write down what I think, this works before we do that.

ChenyuLInx · 2023-03-07T16:43:16Z

core/dbt/events/types.proto

@@ -1600,7 +1600,16 @@ message ConcurrencyLineMsg {
    ConcurrencyLine data = 2;
 }

-// Skipped Q028
+// Q028


Just curious, what is Q stand for?

Node execution, it's not the perfect category, but the best one I could find.

ChenyuLInx · 2023-03-07T16:49:18Z

core/dbt/task/base.py

@@ -309,7 +310,7 @@ def skip_result(self, node, message):

    def compile_and_execute(self, manifest, ctx):
        result = None
-        with self.adapter.connection_for(self.node):
+        with self.adapter.connection_for(self.node) if get_flags().INTROSPECT else nullcontext():


If a user set DBT_INTROSPECT to False, but we are not running compile/preview command, get_flags().INTROSPECT will be True since it is using default value right?

Yes, correct.

core/dbt/task/compile.py

stu-k

With the exception of those relative imports, nothing stands out 👍

aranke · 2023-03-08T23:37:14Z

core/dbt/events/types.py

+        return "Q028"
+
+    def message(self) -> str:
+        return f"Compiled node '{self.node_name}' is:\n{self.compiled}"


@dbeatty10 Does this message make sense, or should I change it to something else?

Do you have an example what this message would look like with realistic values?

Yup:

❯ dbt compile --inline "select * from {{ ref('raw_orders') }}" ... 17:50:59 Compiled node 'inline_query' is: select * from "jaffle_shop"."main"."raw_orders" 17:50:59 Done.

❯ dbt compile --select stg_payments ... 17:51:27 Compiled node 'stg_payments' is: with source as ( select * from "jaffle_shop"."main"."raw_payments" ), renamed as ( select id as payment_id, order_id, payment_method, -- `amount` is currently stored in cents, so we convert it to dollars amount / 100 as amount from source ) select * from renamed 17:51:27 Done.

dbeatty10 · 2023-03-08T23:50:03Z

core/dbt/tests/util.py

+def file_exists(*paths):
+    """Check if file exists at path"""
+    return os.path.exists(os.path.join(*paths))
+
+


Not a problem to solve in this PR, but I wonder why os.path was used in this module rather than pathlib?

We're already using os in several places instead of pathlib so wanted to keep the status quo to avoid bringing in a new import.
But I don't feel too strongly either way.

Yep, going with the flow make sense in this PR.

Something for us to consider in a refactor down the road though.

ChenyuLInx · 2023-03-09T17:29:04Z

core/dbt/task/compile.py

@@ -43,19 +43,32 @@ def raise_on_first_error(self):
        return True

    def get_node_selector(self) -> ResourceTypeSelector:
+        if getattr(self.args, "inline", None):
+            resource_types = [NodeType.SqlOperation]
+        elif getattr(self.args, "select", None):


This will be a behavior change, we can do it in task_end_message if that change is not acceptable.(I don't know whether it is or not)

jtcohen6 · 2023-03-10T20:29:23Z

core/dbt/task/compile.py

+        if getattr(self.args, "inline", None):
+            resource_types = [NodeType.SqlOperation]
+        elif getattr(self.args, "select", None):
+            resource_types = [NodeType.Model]


Discussed offline yesterday with @aranke @ChenyuLInx:

This is to handle the fact that --select my_model will also select the tests on my_model, and we really only want to be compiling one node. The right way to handle this behavior is to turn off "indirect selection," and the cleanest approach there is probably to introduce a new option to turn off indirect selection entirely (--indirect-selection empty).

Depending on how much work is involved there, we can scope & estimate that as a separate issue.

Added a new selection method empty to only select a node that matches the selector.

Did y'all consider any alternative names to empty? For example, none feels more intuitive to me, but maybe y'all considered it already.

Adding empty to the current options would give:

eager (default) - include ANY test that references the selected nodes

cautious - restrict to tests that ONLY refer to selected nodes

buildable - restrict to tests that ONLY refer to selected nodes (or their ancestors)

empty - don't include any tests

If it is named something like none, then the list would be this instead:

eager

cautious

buildable

none

We can't name a Click option none without some trickery, since that's a built-in Python type 😞
I anticipate Cloud to be the only people who use this, so didn't give it too much thought.

ChenyuLInx

Looks good over all! Last two questions

ChenyuLInx · 2023-03-10T23:12:56Z

tests/functional/compile/test_compile.py

+    def test_select_pass(self, project):
+        (results, log_output) = run_dbt_and_capture(["compile", "--select", "second_model"])
+        assert len(results) == 3
+        assert "Compiled node 'second_model' is:" in log_output


I thought in this case we would't print the compiled result for second model?

Talked with @jtcohen6 in the office, we should print this out even if we did compile it.
I've included the filtering behavior you mentioned to only print out exact matches for nodes.

Then --indirect-selection empty is only to make sure we don't compile test etc? makes sense to me!

Yeah, it's to only compile the node that is selected and nothing else.

ChenyuLInx · 2023-03-10T23:16:37Z

core/dbt/cli/flags.py

@@ -42,6 +43,7 @@
    "fail_fast",
    "indirect_selection",
    "store_failures",
+    "introspect",


Do you mean removing it would cause things to fail? This doesn't feel right given that introspect is only a parameter for compile task command, and all parameter in this list is defiled both as cli click group level and a task command level.

ChenyuLInx

LGTM with one question

core/dbt/task/compile.py

dbeatty10 · 2023-03-13T15:30:24Z

@aranke Do we have an issue opened in the docs.getdbt.com repo for this new feature?

If not, then we'd want to fill out the template here.

We'd want to add the dbt-core v1.5 label so that it gets tracked with the other 1.5 features that need documentation.

Co-authored-by: Github Build Bot <[email protected]>

aranke · 2023-03-14T17:08:39Z

@dbeatty10 done, thank you!
dbt-labs/docs.getdbt.com#2994

dbeatty10 · 2023-03-14T17:14:53Z

@dbeatty10 done, thank you! dbt-labs/docs.getdbt.com#2994

Sure thing @aranke !

Could you also add the dbt-core v1.5 label to dbt-labs/docs.getdbt.com#2994 ?

I just updated the PR checklist so it's easy to see the linked issue and its status:

* ct-2198: clean up some type names and uses * CT-2198: Unify constraints and constraints_check properties on columns * Make mypy version consistently 0.981 (#7134) * CT 1808 diff based partial parsing (#6873) * model contracts on models materialized as views (#7120) * first pass * rename tests * fix failing test * changelog * fix functional test * Update core/dbt/parser/base.py * Update core/dbt/parser/schemas.py * Create method for env var deprecation (#7086) * update to allow adapters to change model name resolution in py models (#7115) * update to allow adapters to change model name resolution in py models * add changie * fix newline adds * move quoting into macro * use single quotes * add env DBT_PROJECT_DIR support #6078 (#6659) Co-authored-by: Jeremy Cohen <[email protected]> * Add new index.html and changelog yaml files from dbt-docs (#7141) * Make version configs optional (#7060) * [CT-1584] New top level commands: interactive compile (#7008) Co-authored-by: Github Build Bot <[email protected]> * CT-2198: Add changelog entry * CT-2198: Fix tests which broke after merge * CT-2198: Add explicit validation of constraint types w/ unit test * CT-2198: Move access property, per code review * CT-2198: Remove a redundant macro * CT-1298: Rework constraints to be adapter-generated in Python code * CT-2198: Clarify function name per review --------- Co-authored-by: Gerda Shank <[email protected]> Co-authored-by: Emily Rockman <[email protected]> Co-authored-by: Stu Kilgore <[email protected]> Co-authored-by: colin-rogers-dbt <[email protected]> Co-authored-by: Leo Schick <[email protected]> Co-authored-by: Jeremy Cohen <[email protected]> Co-authored-by: FishtownBuildBot <[email protected]> Co-authored-by: dave-connors-3 <[email protected]> Co-authored-by: Kshitij Aranke <[email protected]> Co-authored-by: Github Build Bot <[email protected]>

Over a year ago in [#7008](https://github.com/dbt-labs/dbt-core/pull/7008/files#diff-c10753a61a1d8f973ec3206206d72b3c5200e41673ab2c6b256f333103eadb8f) the tests in `test_compiler.py` got moved into other testing files. Since then this file has been sitting around empty. It is probably a reasonable time to say goodbye to it.

#9878) * Delete empty `test_compiler.py` file Over a year ago in [#7008](https://github.com/dbt-labs/dbt-core/pull/7008/files#diff-c10753a61a1d8f973ec3206206d72b3c5200e41673ab2c6b256f333103eadb8f) the tests in `test_compiler.py` got moved into other testing files. Since then this file has been sitting around empty. It is probably a reasonable time to say goodbye to it. * Delete empty `searcher.py` file The file `searcher.py` was added, as an empty file, four years ago in [#2312](https://github.com/dbt-labs/dbt-core/pull/2312/files#diff-c8e9e62cf63356f44fe1a2b0272b239d7a650c57911477ed4549b15ee3de16fa). Since then it has never been touched or added to. I'm not sure of the initial purpose of the file as it never contained anything and I have not found any documentation in reference to it. It's safe to say, it can go away.

Replace test_compiler unit test with test_compile functional test

94ac502

cla-bot bot added the cla:yes label Feb 19, 2023

aranke and others added 5 commits February 20, 2023 19:44

Implement flag for introspection

aa00da6

Add generated CLI API docs

2f1a915

Implement inline compile for code

45d0b49

merge main

271b9b8

Restore test/unit/test_lib.py

97db5d0

aranke marked this pull request as ready for review March 7, 2023 01:01

aranke requested a review from a team March 7, 2023 01:01

aranke requested review from a team as code owners March 7, 2023 01:01

aranke requested review from ChenyuLInx and emmyoop March 7, 2023 01:01

aranke added 4 commits March 6, 2023 17:03

Add changie

8c8d720

Fix some tests

0430de7

fix more tests

b64662f

change quoting

a5fcd10

aranke requested review from stu-k and peterallenwebb and removed request for emmyoop March 7, 2023 03:25

ChenyuLInx reviewed Mar 7, 2023

View reviewed changes

stu-k approved these changes Mar 8, 2023

View reviewed changes

make dbt compile for models only

7be301a

aranke requested a review from ChenyuLInx March 8, 2023 22:58

aranke added 2 commits March 8, 2023 14:59

merge from main

0df650e

make test for interactive compile cleaner

9cb6a09

aranke commented Mar 8, 2023

View reviewed changes

dbeatty10 reviewed Mar 8, 2023

View reviewed changes

ChenyuLInx reviewed Mar 9, 2023

View reviewed changes

jtcohen6 reviewed Mar 10, 2023

View reviewed changes

Create empty selector to only select node with ID

f1d0238

aranke requested a review from ChenyuLInx March 10, 2023 23:04

ChenyuLInx reviewed Mar 10, 2023

View reviewed changes

aranke added 2 commits March 10, 2023 15:24

Only loop over model nodes to print

d684e53

Match on model name instead of model type

0bc04ef

aranke mentioned this pull request Mar 10, 2023

[CT-2295] Refactor expected_duplicate_params #7156

Open

Get tests to pass

b02447b

ChenyuLInx approved these changes Mar 11, 2023

View reviewed changes

core/dbt/task/compile.py Outdated Show resolved Hide resolved

use getattr instead of direct access

dd0b361

aranke merged commit 3050369 into main Mar 11, 2023

aranke deleted the interactive_compile branch March 11, 2023 01:58

peterallenwebb pushed a commit that referenced this pull request Mar 14, 2023

[CT-1584] New top level commands: interactive compile (#7008)

996cfa8

Co-authored-by: Github Build Bot <[email protected]>

jtcohen6 mentioned this pull request Mar 22, 2023

Replace test_compiler unit test with test_compile functional test #7133

Closed

6 tasks

dbeatty10 mentioned this pull request May 21, 2023

empty indirect selection mode dbt-labs/docs.getdbt.com#3404

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CT-1584] New top level commands: interactive compile #7008

[CT-1584] New top level commands: interactive compile #7008

aranke commented Feb 19, 2023 •

edited by dbeatty10

Loading

github-actions bot commented Feb 19, 2023

ChenyuLInx left a comment

ChenyuLInx Mar 7, 2023 •

edited

Loading

aranke Mar 7, 2023

aranke Mar 7, 2023

ChenyuLInx Mar 10, 2023

aranke Mar 10, 2023

ChenyuLInx Mar 7, 2023

ChenyuLInx Mar 7, 2023

aranke Mar 7, 2023

ChenyuLInx Mar 7, 2023

aranke Mar 7, 2023

stu-k left a comment

aranke Mar 8, 2023 •

edited

Loading

dbeatty10 Mar 9, 2023

aranke Mar 9, 2023

dbeatty10 Mar 8, 2023

aranke Mar 9, 2023

dbeatty10 Mar 9, 2023

ChenyuLInx Mar 9, 2023

jtcohen6 Mar 10, 2023

aranke Mar 10, 2023

dbeatty10 Mar 11, 2023

aranke Mar 13, 2023

ChenyuLInx left a comment

ChenyuLInx Mar 10, 2023

aranke Mar 10, 2023

ChenyuLInx Mar 10, 2023

aranke Mar 10, 2023

ChenyuLInx Mar 10, 2023

ChenyuLInx left a comment

dbeatty10 commented Mar 13, 2023

aranke commented Mar 14, 2023

dbeatty10 commented Mar 14, 2023

[CT-1584] New top level commands: interactive compile #7008

[CT-1584] New top level commands: interactive compile #7008

Conversation

aranke commented Feb 19, 2023 • edited by dbeatty10 Loading

Description

Checklist

github-actions bot commented Feb 19, 2023

ChenyuLInx left a comment

Choose a reason for hiding this comment

ChenyuLInx Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stu-k left a comment

Choose a reason for hiding this comment

aranke Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenyuLInx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenyuLInx left a comment

Choose a reason for hiding this comment

dbeatty10 commented Mar 13, 2023

aranke commented Mar 14, 2023

dbeatty10 commented Mar 14, 2023

aranke commented Feb 19, 2023 •

edited by dbeatty10

Loading

ChenyuLInx Mar 7, 2023 •

edited

Loading

aranke Mar 8, 2023 •

edited

Loading