Feature - Warn on Unknown Flags#12842
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12842 +/- ##
==========================================
- Coverage 91.32% 91.32% -0.01%
==========================================
Files 203 203
Lines 25981 26000 +19
==========================================
+ Hits 23728 23744 +16
- Misses 2253 2256 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
QMalcolm
left a comment
QMalcolm
left a comment
There was a problem hiding this comment.
Thank you for doing this work! It's exciting to get this addressed 🚀
I do think we need to do some more work before we get this out though. As is, we're using the deprecations module to buffer and fire a non deprecation. I assume we're buffering because the event manager isn't set up at the time this event would be normally fired? Interestingly, @aahel is also working on deferring events, but notably that system would require that the event manager be in place (dbt-labs/dbt-common#361).
I think there are really 4 solutions
- Make the warning a deprecation, fully set up a real deprecation event, and then use the existing
deprecations.buffer - Find away to move the instantiation of the event manager forward in time
- Move the logic for checking on unknown flags later in the execution when the event manager has been instantiated
- Split out the "buffering" logic out of
deprecationsmodule into anevent_bufferingmodule
|
I think this can be considered a deprecation since using unknown flags will error out in the future when we toggle the flag. However, it ties the warning to the deprecation lifecycle which may not be desired. Option 4 is also good which can be used to buffer events. What do you suggest between these two @QMalcolm ? FYI @graciegoheen |
|
@QMalcolm I've pushed some new changes to introduce a behaviour flag that'll raise errors when it finds an unknown flag. |
@sriramr98 why would tying it to the deprecation lifecycle not be desired? |
|
@QMalcolm The way I see it, deprecation is something that was once valid/supported is being phased out. Unknown flags were never valid, we were just ignoring them. There's nothing being "deprecated" here. It's adding validation that was always missing. On the other hand, maybe it was supported for custom use cases and is being "deprecated" here in which case, making this a deprecation is the right choice. This might be a NIT and we should be good to make this a deprecation. This was just a thought lingering in me |
@sriramr98 I think, given this will be converted to an error in the future, we should make it a deprecation, i.e. Option 1. Though it might feel weird making it a deprecation in some regard (it wasn't accessible), we did the same with "custom keys on objects". I.e. we deprecated the custom key in the following yaml models
- name: my_model
description: describing my model
custom_key: my custom stuffSuch custom_key's prior to the deprecation, just got silently dropped ignored, the same that is happening with the custom flags. Additionally, making it a deprecation is the "least" work 😂 Not saying that is the reason we should do it that way, but it is a nice advantage. |
|
@QMalcolm the deprecation warning is added |
QMalcolm
left a comment
QMalcolm
left a comment
There was a problem hiding this comment.
Two things to handle
- What do we want to do with
require_no_unknown_flagsbehavior flag - We should bump the minimum version of dbt-protos in
core/pyproject.tomlto be the a version that includes the new event.- We often forget to bump this. I definitely have in the past 🫠 It can cause problems in OSS because on a non-fresh install they might retain an earlier version of dbt-protos if we don't bump it
| require_sql_header_in_test_configs: bool = False | ||
| support_custom_ref_kwargs: bool = False | ||
| require_corrected_analysis_fqns: bool = False | ||
| require_no_unknown_flags: bool = False |
There was a problem hiding this comment.
It doesn't appear we're using this anywhere anymore? Probably just an oversight. If we want to use this, we should edit the logic in project.py to raise an exception instead of buffering the deprecation. If we don't want to use it then we should drop it. I'm fine with either outcome 🙂
There was a problem hiding this comment.
We are using it to raise an error if it's set @QMalcolm
There was a problem hiding this comment.
🤦🏻 I can't read today. Time for more coffee. Sorry about that 😞
There was a problem hiding this comment.
So it's just the the package bump then
|
I just realized we might need to re-think this implementation. There are a couple drawbacks to this implementation.
I think this is much more trickier than anticipated. We might have to go back to the drawing board and re-think this. |
3 participants
Resolves #12590
Problem
When users set flags in dbt_project.yml that dbt doesn't recognize (e.g., typos like prrint instead of print), dbt silently ignores them. There's no feedback to the user that their configuration is being ignored, leading to confusion when expected behavior doesn't change.
Solution
Added validation in read_project_flags() at core/dbt/config/project.py that compares the user's flags against known ProjectFlags field names. Unknown flags trigger a warning via fire_event(InvalidOptionYAML(...)), guiding users to fix their config.
The warning message lists the first 10 known flag names to help users identify the correct option.
Since event logger is being setup after flags parsing, the warning needs to be deferred to be shown after event logger is setup for
fire_eventto work properly here.Credits to @chinar-amrutkar for the Original PR
Checklist