Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better error messaging for YAML selectors #2700

Closed
jtcohen6 opened this issue Aug 13, 2020 · 4 comments · Fixed by #2781
Closed

Better error messaging for YAML selectors #2700

jtcohen6 opened this issue Aug 13, 2020 · 4 comments · Fixed by #2781
Assignees
Labels

Comments

@jtcohen6
Copy link
Contributor

I chatted with Drew yesterday about two pieces of YAML selectors:

  1. Excludes: As acknowledged in the Feature/yaml selections #2640 description, nesting exclude within union is a little unintuitive. I think it's ok as long as we document it well: exclude acts as a set difference operator, and it is always applied after the other elements are unioned together. This gets us more intricate subset definitions than what's available on the CLI, where we can only pass one yeslist (--models, --select) and one nolist (--exclude).
  2. Definition dict: definition is a dictionary with a number of potential arguments:
    • CLI-style selection string
    • key: value
    • method + value
    • union
    • intersection

Action steps

  1. We already raise an error if two exclude arguments are passed to the same set operator. We could make this message clearer:
Could not read selector file data: Runtime Error
    Got multiple exclusion definitions in definition list ['models/export', 'config.materialized:incremental', {'exclude': [{'method': 'tag', 'value': 'nightly'}]}, {'exclude': [{'method': 'tag', 'value': 'hourly'}]}]
  1. If definition has either union or intersection as an argument and more than one argument, we should raise an error. Something along the lines of:
Only one root-level set operator is supported in a selector definition. Got 2: [union, anything_else] in my_selector_name

I'm going to leave this issue open to continue collecting feedback as people start using YAML selectors more widely in the forthcoming release candidate.

@jtcohen6 jtcohen6 added this to the Marian Anderson milestone Aug 13, 2020
@jtcohen6 jtcohen6 added enhancement New feature or request bug Something isn't working and removed enhancement New feature or request labels Aug 14, 2020
@jtcohen6 jtcohen6 modified the milestones: Marian Anderson, 0.19.0 Aug 18, 2020
@jtcohen6 jtcohen6 mentioned this issue Aug 20, 2020
4 tasks
@mlavoie-sm360
Copy link

#2640 (comment)
When executing a [run|ls|compile|etc.] using a --select, it would be nice if the logs printed the expanded version of that selector, i.e.

selectors:
  - name: my_selector
    definition:
      union:
        - method: tag
          value: something
          parents: true
        - exclude:
            - method: tag
              value: something_else

Would then print out something like:

dbt run --select my_selector
Running with dbt=0.18.0-rc1
Found 562 models, 578 tests, 8 snapshots, 177 analyses, 323 macros, 4 operations, 40 seed files, 198 sources

15:40:52 | Executing dbt run --models +tag:something --exclude tag:something_else
15:40:52 | 
15:40:52 | Running 2 on-run-start hooks
...

@jtcohen6 jtcohen6 modified the milestones: Marian Anderson, 0.18.1 Aug 28, 2020
@jtcohen6 jtcohen6 added enhancement New feature or request and removed bug Something isn't working labels Aug 28, 2020
@gshank gshank self-assigned this Sep 1, 2020
@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Sep 9, 2020

@mlavoie-sm360 Thanks for re-commenting here, and sorry for the delay following up.

What's your hope with printing out the CLI-arg representation?

  • If it's copy-pasting and sharing with a colleague, that's the larger goal of YAML selectors—check into version control, share, anyone can reference with --selector.
  • If it's seeing that grouping of resources in the dbt-docs DAG viz—fair! We're planning to add selector support there in the next version of dbt (Make YML selectors available in DAG view dbt-docs#119).

Without knowing more, I'm quite hesitant to add this functionality, for two reasons:

  • UX: YAML selectors will often define selection criteria that would be very verbose and clunky to represent with CLI args. There's a good chance this will be a source of clutter and confusion in the log output.
  • Functional: We're committing to ensuring that a YAML selector and the functionally equivalent CLI-arg representation will map to the same set of selected nodes. We're not committing, however, to maintaining a bidirectional translation between CLI + YAML syntaxes. (Someone else is welcome to write that python script.)

@mlavoie-sm360
Copy link

@jtcohen6, thanks for taking the time to reply!

To answer your question, my hope was to have feedback on the definition of my YAML selector in a more digestible way. It may be just me, but dbt run --models +tag:something --exclude tag:something_else speaks to me more than the yaml structure that represents the same thing. I often find myself doing the wrong thing in a YAML structure and I find it useful when I can visualize it in a different format.

If doing that is opening a can of worms, my nice-to-have proposition is probably not worth it.

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Sep 9, 2020

Gotcha. Thanks for the explanation, and sorry to be a dasher of hopes. I totally hear what you're saying. While writing YAML selectors, I frequently find myself using dbt ls --selector to check that my selector definition is doing the thing I think it is.

I do think it would be cool if someone (?) built a handy script to parse a YAML selector and return a translation into the language of your choosing: CLI args, English sentences, ...

`my_selector` includes all resources tagged 'something', plus their parents, except for resources with tag 'something_else'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants