fix small issues with flags and logger to support dbt-rpc #6990
Conversation
|
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. |
core/dbt/events/functions.py
Outdated
| @@ -133,7 +140,7 @@ def cleanup_event_logger(): | |||
| EVENT_MANAGER.add_logger( | |||
| _get_logbook_log_config(debug=False) # type: ignore | |||
| if ENABLE_LEGACY_LOGGER | |||
| else _get_stdout_config(log_format="text", debug=False, use_colors=True) # type: ignore | |||
| else _get_stdout_config(log_format="text", debug=False, use_colors=True, log_cache_events=True, quiet=False) # type: ignore | |||
There was a problem hiding this comment.
I think we'll want log_cache_events=False by default
FYI @peterallenwebb since you're:
- assigned to [CT-2123] [Bug] Flags are referenced incorrectly (and sometimes at the wrong time) in functions.py #6979
- thinking about how to handle
--quietin the default logger, before we have full config ([CT-2022] Non-error log is not silenced by -q flag, Click Version #6847) — it's just being set toFalsehere
To be honest, I don't know if it's possible, and it might be motivation for us to really finally uncouple UserConfig from profiles.yml (#6207), so that we can have a configured logger before we need to parse the profile (and encounter any warnings)
There was a problem hiding this comment.
Updated, what exactly does it do?
There was a problem hiding this comment.
Super verbose logging every time a relation is added, removed, updated from the cache. Adds a ton of noise in the general case, but very useful to turn on when debugging issues that might be due to cache misses/inconsistencies
ChenyuLInx
force-pushed
the
cl/fix_logbook_rebased
branch
from
6945d8a to
3f4c6f9
Compare
ChenyuLInx
requested review from
racheldaniel
and removed request for
emmyoop
| @@ -49,13 +49,33 @@ def convert_config(config_name, config_value): | |||
| # This function should take care of converting the values from config and original | |||
| # set_from_args to the correct type | |||
| ret = config_value | |||
| if config_name.lower() == "warn_error_options": | |||
| if config_name.lower() == "warn_error_options" and type(config_value) == dict: | |||
There was a problem hiding this comment.
Why is this change necessary? Shouldn't warn_error_options always be a dictionary?
Docs: https://docs.getdbt.com/reference/global-configs#warnings-as-errors
There was a problem hiding this comment.
There are other places that passes in something that's not a dict. I can do another round of verify it is a dict and update input from other places.
There was a problem hiding this comment.
I remember why, it is because the default value for warn_error_options in UserConfig is None, so if you pass in a UserConfig object the original logic would fail
There was a problem hiding this comment.
👍 gotcha. config_value is not None would be more precise here then
core/dbt/cli/flags.py
Outdated
| ret = WarnErrorOptions( | ||
| include=config_value.get("include", []), exclude=config_value.get("exclude", []) | ||
| ) | ||
| return ret | ||
|
|
||
|
|
||
| def args_to_context(args): |
| @@ -89,14 +88,18 @@ def get_dbt_config(project_dir, args=None, single_threaded=False): | |||
| target=getattr(args, "target", None), | |||
| ) | |||
|
|
|||
| # set global flags from arguments | |||
There was a problem hiding this comment.
The functions in this file are still by large untested, gonna revisit to replace this file and test the interfaces we have to replace it.
| sub_command_name, sub_command, args = cli.resolve_command(cli_ctx, args) | ||
|
|
||
| # handle source and docs group | ||
| if type(sub_command) == Group: |
There was a problem hiding this comment.
I think we only have one level of Group nesting at the moment (sources and docs, as you mentioned in the comment), but we could make this a while instead so that it works for multiple levels of Groups too.
There was a problem hiding this comment.
Planning on revisit all of this and add more things to it as we rethink the flags object.
core/dbt/cli/flags.py
Outdated
| ret = WarnErrorOptions( | ||
| include=config_value.get("include", []), exclude=config_value.get("exclude", []) | ||
| ) | ||
| return ret | ||
|
|
||
|
|
||
| def args_to_context(args): |
There was a problem hiding this comment.
What is the intended usage of this method? Is it just to create a click context that can be used to construct a Flags instance, or would we also want callers invoking the click command on the built context? If it's just the first use case, perhaps we could add this as a class method on Flags instead, and get eventually use it to get rid of set_from_args?
There was a problem hiding this comment.
Yes it is, and that's what I mean by Flag object is leaky.
The only place that uses this right now is in dbt-rpc, and we are gonna deprecate it in 1.6. So I am thinking of merging this change for now to make the 1.5 beta release, and do a revisit of the whole flags object before final cut of 1.5
resolves #6979 dbt-labs/dbt-rpc#122
The helper functions are not in ideal situation but I think we can revisit them in the revisit of Flags module that's happening soon