Parser no longer takes greedy. Accepts indirect selection, a bool. #4104

VersusFacit · 2021-10-21T01:28:52Z

resolves #4082

Description

This removes the greedy flag and instead adds the indirect-selection flag with eager and cautious as options for which files to pull in as nodes when running tests.

Checklist

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 updated the CHANGELOG.md and added information about my change

core/dbt/flags.py

core/dbt/graph/selector_spec.py

…for option.

VersusFacit · 2021-10-28T02:53:00Z

@jtcohen6 After some trial and error, Joel and I were able to get every test working except for one involving setting the indirect_selection flag in a yaml selector.

We do plan to add a couple more cautious CLI tests in time.

But, first, we were unable to find in selector and selector spec the parsing of the yaml file actually takes place. We suspect we need to update the logic in that file. Can you give us a pointer?

test/integration/066_test_selection_tests/test_selection_expansion.py

core/dbt/flags.py

jtcohen6

@VersusFacit @joellabes Really nice work!! I love how this has come along.

What do you think of keeping the name and type of this config consistent across the board—from the flag, to the yaml selector property, all the way up to the final expand_selection method? I find it a bit confusing to switch between the string flag/property and the boolean arg, especially if we envision a future where there could be other modes.

I managed to switch this to use an IndirectSelection enum: 8f41453. I also snuck a performance boost into incorporate_indirect_nodes that should shave off a few seconds in large projects, now that indirect selection is eager by default.

Feel free to use, adapt, or toss that code!

VersusFacit · 2021-10-30T01:07:26Z

@jtcohen6 Burnt some midnight oil and figured out a way to solve this. Tests are working and I added some. The code allows users to add the field to a selector yaml and override whatever the CLI flag may or may not be.

Do you want to mix in the changes in that linked sha? Is the total renaming necessary? I almost like the way the predicates flow but you know the structure here better than I.

Also, a bit unsure how to address the changelog.

jtcohen6

Thanks for the hard work to see this through!

As long as the end-user behavior is right, and the argument/property names make enough sense to follow through the code, this is as much as we need for now. At such time as we want to swing back through and add a third (or fourth) indirect selection mode, we can lightly refactor as it makes sense. I don't think we need to spend any more time here now.

@VersusFacit As far as the changelog: Could you add a line under the dbt-core 1.0.0 (Release TBD) > Features, with a link to the issue + this PR? You can also add yourself + Joel to the list of contributors. As soon as you do that, we can merge and lock this in for v1 :)

After that, there'll just be the matter of updating docs: dbt-labs/docs.getdbt.com#896

jtcohen6 · 2021-11-03T09:58:22Z

core/dbt/graph/selector_spec.py

+            else:
+                raise RuntimeException(
+                    f'indirect_selection value "{indirect_selection}" is not valid!'
+                )


A more ergonomic way to do this might be by registering IndirectSelection as a dataclass, with accepted values and a validation step. That will make more sense in a future where we have more than two indirect selection modes. This works well enough for now.

test/integration/066_test_selection_tests/test_selection_expansion.py

jtcohen6 · 2021-11-07T16:08:42Z

I want to be sure this change gets in before v1 (especially the switch to the default), so I'm going to add a changelog entry now and merge!

* Rm unused events, per #4104 * More structured ConcurrencyLine * Replace \n prefixes with EmptyLine * Reimplement ui.warning_tag to centralize logic * Use warning_tag for deprecations too * Rm more unused event types * Exclude EmptyLine from json logs * loglines are not always created by events (#4406) Co-authored-by: Nathaniel May <[email protected]>

* A few final logging touch-ups (#4388) * Rm unused events, per #4104 * More structured ConcurrencyLine * Replace \n prefixes with EmptyLine * Reimplement ui.warning_tag to centralize logic * Use warning_tag for deprecations too * Rm more unused event types * Exclude EmptyLine from json logs * loglines are not always created by events (#4406) Co-authored-by: Nathaniel May <[email protected]> * Rollover + backup for dbt.log (#4405) Co-authored-by: Nathaniel May <[email protected]>

* Rm unused events, per #4104 * More structured ConcurrencyLine * Replace \n prefixes with EmptyLine * Reimplement ui.warning_tag to centralize logic * Use warning_tag for deprecations too * Rm more unused event types * Exclude EmptyLine from json logs * loglines are not always created by events (#4406) Co-authored-by: Nathaniel May <[email protected]> automatic commit by git-black, original commits: 8bdaa69 b90ab74

* Rm unused events, per #4104 * More structured ConcurrencyLine * Replace \n prefixes with EmptyLine * Reimplement ui.warning_tag to centralize logic * Use warning_tag for deprecations too * Rm more unused event types * Exclude EmptyLine from json logs * loglines are not always created by events (#4406) Co-authored-by: Nathaniel May <[email protected]> automatic commit by git-black, original commits: b90ab74

…4104) * Parser no longer takes greedy. Accepts indirect selection, a bool. * Remove references to greedy and supporting functions. * 1. Set testing flag default to True. 2. Improve arg parsing. * Update tests and add new case for when flag unset. * Update names and styling to fit test requirements. Add default value for option. * Correct several failing tests now that default behavior was flipped. * Tests expect eager on by default. * All but selector test passing. * Get integration tests working, add them, and mix in selector syntax. * Clean code and correct test. * Add changelog entry Co-authored-by: Mila Page <[email protected]> Co-authored-by: Jeremy Cohen <[email protected]> automatic commit by git-black, original commits: a976e54 e6df426

…4104) * Parser no longer takes greedy. Accepts indirect selection, a bool. * Remove references to greedy and supporting functions. * 1. Set testing flag default to True. 2. Improve arg parsing. * Update tests and add new case for when flag unset. * Update names and styling to fit test requirements. Add default value for option. * Correct several failing tests now that default behavior was flipped. * Tests expect eager on by default. * All but selector test passing. * Get integration tests working, add them, and mix in selector syntax. * Clean code and correct test. * Add changelog entry Co-authored-by: Mila Page <[email protected]> Co-authored-by: Jeremy Cohen <[email protected]> automatic commit by git-black, original commits: e6df426

Parser no longer takes greedy. Accepts indirect selection, a bool.

a8f8ff0

VersusFacit requested review from joellabes and jtcohen6 October 21, 2021 01:28

VersusFacit self-assigned this Oct 21, 2021

dbt-labs deleted a comment from cla-bot bot Oct 21, 2021

VersusFacit marked this pull request as draft October 21, 2021 01:31

Remove references to greedy and supporting functions.

47df825

cla-bot bot added the cla:yes label Oct 21, 2021

jtcohen6 reviewed Oct 21, 2021

View reviewed changes

core/dbt/flags.py Outdated Show resolved Hide resolved

jtcohen6 reviewed Oct 21, 2021

View reviewed changes

core/dbt/graph/selector_spec.py Outdated Show resolved Hide resolved

VersusFacit added 2 commits October 26, 2021 22:20

1. Set testing flag default to True. 2. Improve arg parsing.

6afc0ab

Update tests and add new case for when flag unset.

d347fb8

VersusFacit force-pushed the feature/4082-eager-test-select branch 6 times, most recently from 205b3b5 to 7ce2077 Compare October 27, 2021 07:44

Update names and styling to fit test requirements. Add default value …

f2b6e65

…for option.

VersusFacit force-pushed the feature/4082-eager-test-select branch from 7ce2077 to f2b6e65 Compare October 27, 2021 07:59

VersusFacit added 3 commits October 27, 2021 01:22

Correct several failing tests now that default behavior was flipped.

be11769

Tests expect eager on by default.

e3f699c

All but selector test passing.

fa42880

jtcohen6 reviewed Oct 28, 2021

View reviewed changes

test/integration/066_test_selection_tests/test_selection_expansion.py Show resolved Hide resolved

core/dbt/flags.py Outdated Show resolved Hide resolved

jtcohen6 reviewed Oct 28, 2021

View reviewed changes

VersusFacit added 2 commits October 29, 2021 17:12

Get integration tests working, add them, and mix in selector syntax.

5c41583

Clean code and correct test.

3d54b6c

VersusFacit marked this pull request as ready for review October 30, 2021 01:05

VersusFacit requested a review from jtcohen6 October 30, 2021 01:05

Merge branch 'main' into feature/4082-eager-test-select

8cc45a9

jtcohen6 mentioned this pull request Nov 3, 2021

greedy -> indirect_selection dbt-labs/docs.getdbt.com#896

Closed

jtcohen6 approved these changes Nov 3, 2021

View reviewed changes

jtcohen6 mentioned this pull request Nov 5, 2021

Wasted time spent in incorporate_indirect_nodes during node selection #4213

Closed

jtcohen6 added 3 commits November 7, 2021 17:16

Add changelog entry

428ad87

Merge branch 'main' into feature/4082-eager-test-select

2dc0ed5

Merge branch 'main' into feature/4082-eager-test-select

7003ee8

jtcohen6 merged commit e6df426 into main Nov 7, 2021

jtcohen6 deleted the feature/4082-eager-test-select branch November 7, 2021 16:41

VersusFacit mentioned this pull request Nov 11, 2021

Feature/3997 profiles test selection flag #4270

Merged

4 tasks

jtcohen6 added a commit that referenced this pull request Nov 17, 2021

Rm unused events, per #4104

045de47

jtcohen6 added a commit that referenced this pull request Dec 2, 2021

Rm unused events, per #4104

1301fb6

jtcohen6 added a commit that referenced this pull request Dec 2, 2021

Rm unused events, per #4104

2fe32f8

jtcohen6 added a commit that referenced this pull request Dec 2, 2021

Rm unused events, per #4104

614d405

jtcohen6 mentioned this pull request Dec 2, 2021

A few final logging touch-ups #4388

Merged

2 tasks

jtcohen6 added a commit that referenced this pull request Dec 2, 2021

Rm unused events, per #4104

6e2c141

jtcohen6 added a commit that referenced this pull request Dec 2, 2021

Rm unused events, per #4104

fceaf5b

jtcohen6 added a commit that referenced this pull request Dec 2, 2021

Rm unused events, per #4104

494aa0f

dbeatty10 mentioned this pull request Dec 10, 2022

[CT-1590] [Feature] Improving dbt selection options #6365

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parser no longer takes greedy. Accepts indirect selection, a bool. #4104

Parser no longer takes greedy. Accepts indirect selection, a bool. #4104

VersusFacit commented Oct 21, 2021 •

edited

Loading

VersusFacit commented Oct 28, 2021

jtcohen6 left a comment

VersusFacit commented Oct 30, 2021 •

edited

Loading

jtcohen6 left a comment

jtcohen6 Nov 3, 2021

jtcohen6 commented Nov 7, 2021

Parser no longer takes greedy. Accepts indirect selection, a bool. #4104

Parser no longer takes greedy. Accepts indirect selection, a bool. #4104

Conversation

VersusFacit commented Oct 21, 2021 • edited Loading

Description

Checklist

VersusFacit commented Oct 28, 2021

jtcohen6 left a comment

Choose a reason for hiding this comment

VersusFacit commented Oct 30, 2021 • edited Loading

jtcohen6 left a comment

Choose a reason for hiding this comment

jtcohen6 Nov 3, 2021

Choose a reason for hiding this comment

jtcohen6 commented Nov 7, 2021

VersusFacit commented Oct 21, 2021 •

edited

Loading

VersusFacit commented Oct 30, 2021 •

edited

Loading