Add ability to `silence` warnings AND allow `error` and `warn` to be used for `include` and `exclude` #10058

QMalcolm · 2024-04-26T19:11:22Z

resolves #9644

Problem

People wanted to be able to silence warnings
People wanted to be able to use error instead of include
People wanted to be able to use warn instead of exclude

Solution

Bumped dbt_common to 1.0.2 which added silence to WarnErrorOptions
Support silence arg to warn_error_options via CLI args and dbt_project.yml
Support error and warn to warn_error_options as alternatives to include and `exclude

Checklist

I have read the contributing guide and understand what's expected of me
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
This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
This PR includes type annotations for new and modified functions

…ther tests

We're going to start depending on `silence` existing as a attr of `WarnErrorOptions`. The `silence` attr was only added in dbt_common 1.0.2, thus that is our new minimum.

…options`

…nsFromCLI`

…oject` flags Support for `silence` was _automatically_ added when we upgraded to dbt_common 1.0.2. This is because we build the project flags in a `.from_dict` style, which is cool. In this case it was _automatically_ handled in `read_project_flags` in `project.py`. More specifically here https://github.com/dbt-labs/dbt-core/blob/bcbde3ac4204f00d964a3ea60896b6af1df18c71/core/dbt/config/project.py#L839

…lude` Typically we can't have multiple tests in the same `test class` if they're utilizing/modifying file system fixtures. That is because the file system fixtures are scoped to test classes, so they don't reset inbetween tests within the same test class. This problem _was_ affectin these tests as they modify the `dbt_project.yml` file which is set by a class based fixture. To get around this, because I find it annoying to create multiple test classes when the tests really should be grouped, I created a "function" scoped fixture to reset the `dbt_project.yml`.

… options Setting `error` is the same as setting `include`, but only one can be specified. Setting `warn` is the same as setting `exclude`, but only one can be specified.

…warn` options As part of this I refactored `exclusive_primary_alt_value_setting` into an upstream location `/config/utils`. That is because importing it in `/config/project.py` from `cli/option_types.py` caused some circular dependency issues.

github-actions · 2024-04-26T19:11:42Z

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.

github-actions · 2024-04-26T19:11:42Z

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.

codecov · 2024-04-26T19:15:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.15%. Comparing base (f6b2cb7) to head (41e6d7c).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10058      +/-   ##
==========================================
- Coverage   88.20%   88.15%   -0.05%     
==========================================
  Files         181      181              
  Lines       22665    22683      +18     
==========================================
+ Hits        19991    19996       +5     
- Misses       2674     2687      +13

Flag	Coverage Δ
integration	`85.46% <100.00%> (-0.12%)`	⬇️
unit	`62.34% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…g` instead of `DbtConfigError` Using `DbtConfigError` seemed reasonable. However in order to make sure the error got raised in `read_project_flags` we had to mark it to be `DbtConfigError`s to be re-raised. This had the uninteded consequence of reraising a smattering of errors which were previously being swallowed. I'd argue that if those are errors we're swallowing, the functions that raise those errors should be modified perhaps to conditionally not raise them, but that's not the world we live in and is out of scope for this branch of work. Thus instead we've created a error specific to `WarnErrorOptions` issues, and now use, and catch for re-raising.

I debated about parametrizing these tests, and it can be done. However, I found that the resulting code ended up being about the same number of lines and slightly less readable (in my opinion). Given the simplicity of these tests, I think not parametrizing them is okay.

emmyoop

Mostly implementation questions.

emmyoop · 2024-04-30T12:10:54Z

core/setup.py

@@ -75,7 +75,7 @@
        "minimal-snowplow-tracker>=0.0.2,<0.1",
        "dbt-semantic-interfaces>=0.5.1,<0.6",
        # Minor versions for these are expected to be backwards-compatible
-        "dbt-common>=1.0.1,<2.0",
+        "dbt-common>=1.0.2,<2.0",


Why are we bumping a patch pin?

The addition of the attr silence to the class WarnErrorOptions is new, and got released in dbt-common 1.0.2. If we don't bump the minimum, then it would be possible for us to get dbt-common 1.0.1 which doesn't support silence, and we'd break at run time. Here is the work in dbt-common which added the functionality

I started a thread in slack about this. Unrelated to you actual PR - more around how this should have been a minor bump instead of a patch.

emmyoop · 2024-04-30T12:11:54Z

core/dbt/config/utils.py

+            f"Only `{alt}` or `{primary}` can be specified in `warn_error_options`, not both"
+        )


Is it also acceptable to use warn and include? Or are we restricting warn/error & include/exclude to be used together?

I'm not sure I fully understand the question, however I think the answer is contained in the following:

error or include can be used, but not both

warn or exclude can be used, but not both

using error or include does not restrict whether one must specify warn or exclude

i.e. these are all valid pairings:

error + warn

error + exclude

include + exclude

include + warn

I finally read the question correctly 🙃 I don't think I had enough coffee when I first read it. Glad I managed to answer to answer the question in my sleep deprived state. Also github needs to start supporting the :melt: emoji.

.changes/unreleased/Features-20240426-233126.yaml

MichelleArk · 2024-04-30T17:54:55Z

core/dbt/config/utils.py

+    alt_options = dictionary.get(alt)
+
+    if primary_options and alt_options:
+        raise DbtWarnErrorOptionsError(


This error indicates to me that the method should only be used in the context of warn_error_options configuration, and not more generally. However, it's defined generality in config.utils. Is there somewhere closer to WarnErrorOptions (perhaps on the class itself) that this method would be more appropriate?

Alternatively, we could raise a more generic error here and have the caller catch + handle it for its specific context.

I think we should probably go the route of having a more generic error here and have the the caller catch + handle it for its specific context. The question is what existing error to use or what new error to create. How about something like DbtExclusivePropertyUseError? 🙂

Currently we intentionally skip a whole bunch of errors in read_project_flags. I originally wrote exclusive_primary_alt_value_setting to raise a DbtRuntimeError, but this is explicitly being swallowed by read_project_flags exception handling. I then tried DbtConfigError, which actually seemed more reasonable and better fitting. However it turns out that DbtConfigError is a subclass of DbtRuntimeError and thus is being swallowed by the same logic mentioned previously. Additionally if we start raising instead of swallowing DbtConfigErrors some tests start failing, which means we actually depend on DbtConfigError being swallowed here 🙃

Separately, we could put this in WarnErrorOptions in dbt-common, and if we want to we can. However, I want to push back against doing so:

This likely won't be our last instance of changing terminology where we want to use this logic (thus having the generalized implementation is useful)

The conceptually the flexibility is on the input end, not on the object end. Thus by implementing the flexibility on the input end, the implementation matches the conceptual mental model.

(fuzzier) It's been discussed whether events should be more dynamic (i.e. the level -- debug, info, warn, error -- can configured for any event). If we move that way, we'd likely want to create a new object and deprecate WarnErrorOptions, and instead of having to duplicate the logic across objects, it exists separately in core prior to instantiation.

DbtExclusivePropertyUseError sounds good 👍 . We can also pass in the config_name (in this case, "warn_error_options") as a parameter to this method for templating the error message

core/dbt/config/project.py

MichelleArk

one outstanding comment but otherwise LGTM! Thank you for taking the extra care to make this change backward compatible ✨

…d error

Back when I did the work for #10058 (specifically c52d653) I thought that the `warn_error_options` would automatically be converted from the yaml to the `WarnErrorOptions` object as we were building the `ProjectFlags` object, which holds `warn_error_options`, via `ProjectFlags.from_dict`. And I thought this was validated by the `test_can_silence` test added in c52d653. However, there were two problems: 1. The definition of `warn_error_options` on `PrjectFlags` is a dict, not a `WarnErrorOptions` object 2. The `test_can_silence` test was broken, and not testing what I thought The quick fix (this commit) is to ensure `silence` is passed to `WarnErrorOptions` instantiation in `dbt.cli.flags.convert_config`. The better fix would be to make the `warn_error_options` of `ProjectFlags` a `WarnErrorOptions` object instead of a dict. However, to do this we first need to update dbt-common's `WarnErrorOptions` definition to default `include` to an empty list. Doing so would allow us to get rid of `convert_config` entirely.

…flags (#10359) * Fix `test_can_silence` tests in `test_warn_error_options.py` to ensure silencing We're fixing an issue wherein `silence` specifications in the `dbt_project.yaml` weren't being respected. This was odd since we had tests specifically for this. It turns out the tests were broken. Essentially the warning was instead being raised as an error due to `include: 'all'`. Then because it was being raised as an error, the event wasn't going through the logger. We were only asserting in these tests that the silenced event wasn't going through the logger (which it wasn't) so everything "appeared" to be working. Unfortunately everything wasn't actually working. This is now highlighted because `test_warn_error_options::TestWarnErrorOptionsFromProject:test_can_silence` is now failing with this commit. * Fix setting `warn_error_options` via `dbt_project.yaml` flags. Back when I did the work for #10058 (specifically c52d653) I thought that the `warn_error_options` would automatically be converted from the yaml to the `WarnErrorOptions` object as we were building the `ProjectFlags` object, which holds `warn_error_options`, via `ProjectFlags.from_dict`. And I thought this was validated by the `test_can_silence` test added in c52d653. However, there were two problems: 1. The definition of `warn_error_options` on `PrjectFlags` is a dict, not a `WarnErrorOptions` object 2. The `test_can_silence` test was broken, and not testing what I thought The quick fix (this commit) is to ensure `silence` is passed to `WarnErrorOptions` instantiation in `dbt.cli.flags.convert_config`. The better fix would be to make the `warn_error_options` of `ProjectFlags` a `WarnErrorOptions` object instead of a dict. However, to do this we first need to update dbt-common's `WarnErrorOptions` definition to default `include` to an empty list. Doing so would allow us to get rid of `convert_config` entirely.

…flags (#10359) * Fix `test_can_silence` tests in `test_warn_error_options.py` to ensure silencing We're fixing an issue wherein `silence` specifications in the `dbt_project.yaml` weren't being respected. This was odd since we had tests specifically for this. It turns out the tests were broken. Essentially the warning was instead being raised as an error due to `include: 'all'`. Then because it was being raised as an error, the event wasn't going through the logger. We were only asserting in these tests that the silenced event wasn't going through the logger (which it wasn't) so everything "appeared" to be working. Unfortunately everything wasn't actually working. This is now highlighted because `test_warn_error_options::TestWarnErrorOptionsFromProject:test_can_silence` is now failing with this commit. * Fix setting `warn_error_options` via `dbt_project.yaml` flags. Back when I did the work for #10058 (specifically c52d653) I thought that the `warn_error_options` would automatically be converted from the yaml to the `WarnErrorOptions` object as we were building the `ProjectFlags` object, which holds `warn_error_options`, via `ProjectFlags.from_dict`. And I thought this was validated by the `test_can_silence` test added in c52d653. However, there were two problems: 1. The definition of `warn_error_options` on `PrjectFlags` is a dict, not a `WarnErrorOptions` object 2. The `test_can_silence` test was broken, and not testing what I thought The quick fix (this commit) is to ensure `silence` is passed to `WarnErrorOptions` instantiation in `dbt.cli.flags.convert_config`. The better fix would be to make the `warn_error_options` of `ProjectFlags` a `WarnErrorOptions` object instead of a dict. However, to do this we first need to update dbt-common's `WarnErrorOptions` definition to default `include` to an empty list. Doing so would allow us to get rid of `convert_config` entirely. (cherry picked from commit 11ee2b9)

…ia `dbt_project.yaml` flags (#10361) * Fix setting `silence` of `warn_error_options` via `dbt_project.yaml` flags (#10359) * Fix `test_can_silence` tests in `test_warn_error_options.py` to ensure silencing We're fixing an issue wherein `silence` specifications in the `dbt_project.yaml` weren't being respected. This was odd since we had tests specifically for this. It turns out the tests were broken. Essentially the warning was instead being raised as an error due to `include: 'all'`. Then because it was being raised as an error, the event wasn't going through the logger. We were only asserting in these tests that the silenced event wasn't going through the logger (which it wasn't) so everything "appeared" to be working. Unfortunately everything wasn't actually working. This is now highlighted because `test_warn_error_options::TestWarnErrorOptionsFromProject:test_can_silence` is now failing with this commit. * Fix setting `warn_error_options` via `dbt_project.yaml` flags. Back when I did the work for #10058 (specifically c52d653) I thought that the `warn_error_options` would automatically be converted from the yaml to the `WarnErrorOptions` object as we were building the `ProjectFlags` object, which holds `warn_error_options`, via `ProjectFlags.from_dict`. And I thought this was validated by the `test_can_silence` test added in c52d653. However, there were two problems: 1. The definition of `warn_error_options` on `PrjectFlags` is a dict, not a `WarnErrorOptions` object 2. The `test_can_silence` test was broken, and not testing what I thought The quick fix (this commit) is to ensure `silence` is passed to `WarnErrorOptions` instantiation in `dbt.cli.flags.convert_config`. The better fix would be to make the `warn_error_options` of `ProjectFlags` a `WarnErrorOptions` object instead of a dict. However, to do this we first need to update dbt-common's `WarnErrorOptions` definition to default `include` to an empty list. Doing so would allow us to get rid of `convert_config` entirely. (cherry picked from commit 11ee2b9) --------- Co-authored-by: Quigley Malcolm <[email protected]> Co-authored-by: Quigley Malcolm <[email protected]>

…flags (#10359) * Fix `test_can_silence` tests in `test_warn_error_options.py` to ensure silencing We're fixing an issue wherein `silence` specifications in the `dbt_project.yaml` weren't being respected. This was odd since we had tests specifically for this. It turns out the tests were broken. Essentially the warning was instead being raised as an error due to `include: 'all'`. Then because it was being raised as an error, the event wasn't going through the logger. We were only asserting in these tests that the silenced event wasn't going through the logger (which it wasn't) so everything "appeared" to be working. Unfortunately everything wasn't actually working. This is now highlighted because `test_warn_error_options::TestWarnErrorOptionsFromProject:test_can_silence` is now failing with this commit. * Fix setting `warn_error_options` via `dbt_project.yaml` flags. Back when I did the work for #10058 (specifically c52d653) I thought that the `warn_error_options` would automatically be converted from the yaml to the `WarnErrorOptions` object as we were building the `ProjectFlags` object, which holds `warn_error_options`, via `ProjectFlags.from_dict`. And I thought this was validated by the `test_can_silence` test added in c52d653. However, there were two problems: 1. The definition of `warn_error_options` on `PrjectFlags` is a dict, not a `WarnErrorOptions` object 2. The `test_can_silence` test was broken, and not testing what I thought The quick fix (this commit) is to ensure `silence` is passed to `WarnErrorOptions` instantiation in `dbt.cli.flags.convert_config`. The better fix would be to make the `warn_error_options` of `ProjectFlags` a `WarnErrorOptions` object instead of a dict. However, to do this we first need to update dbt-common's `WarnErrorOptions` definition to default `include` to an empty list. Doing so would allow us to get rid of `convert_config` entirely.

QMalcolm added 11 commits April 26, 2024 11:30

Refactor test class EventCatcher into utils to make accessible to o…

8e5432d

…ther tests

Raise minimum version of dbt_common to 1.0.2

3292c3a

We're going to start depending on `silence` existing as a attr of `WarnErrorOptions`. The `silence` attr was only added in dbt_common 1.0.2, thus that is our new minimum.

Add ability to silence warnings from warn_error_options CLI arguments

f892f3d

Add flush to EventCatcher test util, and use in `test_warn_error_…

4a94d6a

…options`

Add tests to TestWarnErrorOptionsFromCLI for include and exclude

e02df2d

Refactor runner and catcher into fixtures for `TestWarnErrorOptio…

9f05e1f

…nsFromCLI`

Abstract repetitive assertion logic in test_warn_error_options.py

4c7e04b

Update warn_error_options in CLI args to support error and warn…

ebf9106

… options Setting `error` is the same as setting `include`, but only one can be specified. Setting `warn` is the same as setting `exclude`, but only one can be specified.

cla-bot bot added the cla:yes label Apr 26, 2024

QMalcolm added Skip Changelog Skips GHA to check for changelog file and removed cla:yes labels Apr 26, 2024

cla-bot bot added the cla:yes label Apr 27, 2024

Add changie documentation for 9644 features

0e87a17

QMalcolm removed the Skip Changelog Skips GHA to check for changelog file label Apr 27, 2024

QMalcolm marked this pull request as ready for review April 27, 2024 14:57

QMalcolm requested a review from a team as a code owner April 27, 2024 14:57

emmyoop reviewed Apr 30, 2024

View reviewed changes

QMalcolm requested a review from emmyoop April 30, 2024 17:13

MichelleArk reviewed Apr 30, 2024

View reviewed changes

.changes/unreleased/Features-20240426-233126.yaml Outdated Show resolved Hide resolved

MichelleArk reviewed Apr 30, 2024

View reviewed changes

QMalcolm requested a review from MichelleArk April 30, 2024 21:22

Fixup typo in changie doc Features-20240426-233126.yaml

e1c7839

MichelleArk reviewed Apr 30, 2024

View reviewed changes

core/dbt/config/project.py Outdated Show resolved Hide resolved

MichelleArk approved these changes Apr 30, 2024

View reviewed changes

QMalcolm added 2 commits April 30, 2024 15:37

Update exclusive_primary_alt_value_setting to handle None dictionary

0d960b2

Update exclusive_primary_alt_value_setting to raise more generalize…

41e6d7c

…d error

QMalcolm merged commit 1a9fb61 into main Apr 30, 2024
62 checks passed

QMalcolm deleted the qmalcolm--9644-add-option-to-silence branch April 30, 2024 23:30

graciegoheen added user docs [docs.getdbt.com] Needs better documentation and removed user docs [docs.getdbt.com] Needs better documentation labels May 1, 2024

QMalcolm mentioned this pull request Jun 25, 2024

[Bug] flags.warn_error_options.silence should be settable dbt_project.yml #10160

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to `silence` warnings AND allow `error` and `warn` to be used for `include` and `exclude` #10058

Add ability to `silence` warnings AND allow `error` and `warn` to be used for `include` and `exclude` #10058

QMalcolm commented Apr 26, 2024 •

edited

Loading

github-actions bot commented Apr 26, 2024

github-actions bot commented Apr 26, 2024

codecov bot commented Apr 26, 2024 •

edited

Loading

emmyoop left a comment

emmyoop Apr 30, 2024

QMalcolm Apr 30, 2024 •

edited

Loading

emmyoop Apr 30, 2024

emmyoop Apr 30, 2024

QMalcolm Apr 30, 2024

QMalcolm Apr 30, 2024

MichelleArk Apr 30, 2024 •

edited

Loading

QMalcolm Apr 30, 2024 •

edited

Loading

MichelleArk Apr 30, 2024

MichelleArk left a comment

		f"Only `{alt}` or `{primary}` can be specified in `warn_error_options`, not both"
		)

Add ability to silence warnings AND allow error and warn to be used for include and exclude #10058

Add ability to silence warnings AND allow error and warn to be used for include and exclude #10058

Conversation

QMalcolm commented Apr 26, 2024 • edited Loading

Problem

Solution

Checklist

github-actions bot commented Apr 26, 2024

github-actions bot commented Apr 26, 2024

codecov bot commented Apr 26, 2024 • edited Loading

Codecov Report

emmyoop left a comment

Choose a reason for hiding this comment

emmyoop Apr 30, 2024

Choose a reason for hiding this comment

QMalcolm Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

emmyoop Apr 30, 2024

Choose a reason for hiding this comment

emmyoop Apr 30, 2024

Choose a reason for hiding this comment

QMalcolm Apr 30, 2024

Choose a reason for hiding this comment

QMalcolm Apr 30, 2024

Choose a reason for hiding this comment

MichelleArk Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

QMalcolm Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

MichelleArk Apr 30, 2024

Choose a reason for hiding this comment

MichelleArk left a comment

Choose a reason for hiding this comment

Add ability to `silence` warnings AND allow `error` and `warn` to be used for `include` and `exclude` #10058

Add ability to `silence` warnings AND allow `error` and `warn` to be used for `include` and `exclude` #10058

QMalcolm commented Apr 26, 2024 •

edited

Loading

codecov bot commented Apr 26, 2024 •

edited

Loading

QMalcolm Apr 30, 2024 •

edited

Loading

MichelleArk Apr 30, 2024 •

edited

Loading

QMalcolm Apr 30, 2024 •

edited

Loading