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

fix small issues with flags and logger to support dbt-rpc #6990

Merged
merged 4 commits into from
Feb 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions core/dbt/cli/flags.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from typing import Set, List

from click import Context, get_current_context, BadOptionUsage
from click.core import ParameterSource
from click.core import ParameterSource, Command, Group

from dbt.config.profile import read_user_config
from dbt.contracts.project import UserConfig
Expand Down Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 gotcha. config_value is not None would be more precise here then

ret = WarnErrorOptions(
include=config_value.get("include", []), exclude=config_value.get("exclude", [])
)
return ret


def args_to_context(args: List[str]) -> Context:
"""Convert a list of args to a click context with proper hierarchy for dbt commands"""
from dbt.cli.main import cli

cli_ctx = cli.make_context(cli.name, args)
# args would get converted during make context
if len(args) == 1 and "," in args[0]:
args = args[0].split(",")
sub_command_name, sub_command, args = cli.resolve_command(cli_ctx, args)

# handle source and docs group
if type(sub_command) == Group:
Copy link
Contributor

@MichelleArk MichelleArk Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Planning on revisit all of this and add more things to it as we rethink the flags object.

sub_command_name, sub_command, args = sub_command.resolve_command(cli_ctx, args)

assert type(sub_command) == Command
sub_command_ctx = sub_command.make_context(sub_command_name, args)
sub_command_ctx.parent = cli_ctx
return sub_command_ctx


@dataclass(frozen=True)
class Flags:
def __init__(self, ctx: Context = None, user_config: UserConfig = None) -> None:
Expand Down
2 changes: 2 additions & 0 deletions core/dbt/cli/requires.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ def wrapper(*args, **kwargs):
flags.LOG_FORMAT,
flags.USE_COLORS,
flags.DEBUG,
flags.LOG_CACHE_EVENTS,
flags.QUIET,
)

# Now that we have our logger, fire away!
Expand Down
Binary file modified core/dbt/docs/build/doctrees/environment.pickle
Binary file not shown.
39 changes: 24 additions & 15 deletions core/dbt/events/functions.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import betterproto
from dbt.constants import METADATA_ENV_PREFIX
from dbt.events.base_types import BaseEvent, Cache, EventLevel, NoFile, NoStdOut, EventMsg
from dbt.events.eventmgr import EventManager, LoggerConfig, LineFormat, NoFilter
from dbt.events.eventmgr import EventManager, LoggerConfig, LineFormat
from dbt.events.helpers import env_secrets, scrub_secrets
from dbt.events.types import Formatting
from dbt.flags import get_flags, ENABLE_LEGACY_LOGGER
Expand All @@ -18,20 +18,31 @@
metadata_vars: Optional[Dict[str, str]] = None


def setup_event_logger(log_path: str, log_format: str, use_colors: bool, debug: bool):
def setup_event_logger(
log_path: str,
log_format: str,
use_colors: bool,
debug: bool,
log_cache_events: bool,
quiet: bool,
) -> None:
cleanup_event_logger()
make_log_dir_if_missing(log_path)

if ENABLE_LEGACY_LOGGER:
EVENT_MANAGER.add_logger(_get_logbook_log_config(debug))
else:
EVENT_MANAGER.add_logger(_get_stdout_config(log_format, debug, use_colors))
EVENT_MANAGER.add_logger(
_get_stdout_config(log_format, debug, use_colors, log_cache_events, quiet)
)

if _CAPTURE_STREAM:
# Create second stdout logger to support test which want to know what's
# being sent to stdout.
# debug here is true because we need to capture debug events, and we pass in false in main
capture_config = _get_stdout_config(log_format, True, use_colors)
capture_config = _get_stdout_config(
log_format, True, use_colors, log_cache_events, quiet
)
capture_config.output_stream = _CAPTURE_STREAM
EVENT_MANAGER.add_logger(capture_config)

Expand All @@ -41,19 +52,15 @@ def setup_event_logger(log_path: str, log_format: str, use_colors: bool, debug:
)


def _get_stdout_config(log_format: str, debug: bool, use_colors: bool) -> LoggerConfig:
flags = get_flags()
def _get_stdout_config(
log_format: str, debug: bool, use_colors: bool, log_cache_events: bool, quiet: bool
) -> LoggerConfig:
fmt = LineFormat.PlainText
if log_format == "json":
fmt = LineFormat.Json
elif debug:
fmt = LineFormat.DebugText
level = EventLevel.DEBUG if debug else EventLevel.INFO
# We don't have access to these values when we need to setup the default stdout logger!
log_cache_events = (
bool(flags.LOG_CACHE_EVENTS) if hasattr(flags, "LOG_CACHE_EVENTS") else False
)
quiet = bool(flags.QUIET) if hasattr(flags, "QUIET") else False
return LoggerConfig(
name="stdout_log",
level=level,
Expand Down Expand Up @@ -105,11 +112,11 @@ def _logfile_filter(log_cache_events: bool, log_format: str, msg: EventMsg) -> b

def _get_logbook_log_config(debug: bool) -> LoggerConfig:
# use the default one since this code should be removed when we remove logbook
flags = get_flags()
config = _get_stdout_config("", debug, bool(flags.USE_COLORS))
config = _get_stdout_config("", debug, False, False, False)
config.name = "logbook_log"
config.filter = NoFilter if flags.LOG_CACHE_EVENTS else lambda e: not isinstance(e.data, Cache)
config.filter = lambda e: not isinstance(e.data, Cache)
config.logger = GLOBAL_LOGGER
config.output_stream = None
return config


Expand All @@ -133,7 +140,9 @@ 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=False, quiet=False
) # type: ignore
)

# This global, and the following two functions for capturing stdout logs are
Expand Down
14 changes: 9 additions & 5 deletions core/dbt/flags.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,6 @@ def env_set_truthy(key: str) -> Optional[str]:

# for setting up logger for legacy logger
ENABLE_LEGACY_LOGGER = env_set_truthy("DBT_ENABLE_LEGACY_LOGGER")
LOG_FORMAT = None
DEBUG = None
USE_COLORS = None
LOG_CACHE_EVENTS = None
QUIET = None

# This is not a flag, it's a place to store the lock
MP_CONTEXT = get_context()
Expand All @@ -48,6 +43,15 @@ def set_from_args(args: Namespace, user_config):
from dbt.cli.main import cli
from dbt.cli.flags import Flags, convert_config

# we set attributes of args after initialize the flags, but user_config
# is being read in the Flags constructor, so we need to read it here and pass in
# to make sure we use the correct user_config
if (hasattr(args, "PROFILES_DIR") or hasattr(args, "profiles_dir")) and not user_config:
from dbt.config.profile import read_user_config

profiles_dir = getattr(args, "PROFILES_DIR", None) or getattr(args, "profiles_dir")
user_config = read_user_config(profiles_dir)

# make a dummy context to get the flags, totally arbitrary
ctx = cli.make_context("run", ["run"])
flags = Flags(ctx, user_config)
Expand Down
9 changes: 6 additions & 3 deletions core/dbt/lib.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
from dbt.events.functions import fire_event
from dbt.events.types import NodeCompiling, NodeExecuting
from dbt.exceptions import DbtRuntimeError
from dbt import flags
from dbt.task.sql import SqlCompileRunner
from dataclasses import dataclass
from dbt.cli.resolvers import default_profiles_dir
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

set_from_args(runtime_args, None)
profile, project = load_profile_project(project_dir, profile_name)
assert type(project) is Project

config = RuntimeConfig.from_parts(project, profile, runtime_args)

# Set global flags from arguments
flags.set_from_args(args, config)
# the only thing this set_from_args does differently than
# the one above is that it pass runtime config over, I don't think
# we need that. but leaving this for now for future reference

# flags.set_from_args(runtime_args, config)

# This is idempotent, so we can call it repeatedly
dbt.adapters.factory.register_adapter(config)
Expand Down
8 changes: 4 additions & 4 deletions core/dbt/task/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from datetime import datetime

from dbt import tracking
from dbt import flags
from dbt.flags import get_flags
from dbt.contracts.graph.manifest import Manifest
from dbt.contracts.results import (
NodeStatus,
Expand Down Expand Up @@ -56,7 +56,7 @@ def from_args(cls, args):
def read_profiles(profiles_dir=None):
"""This is only used for some error handling"""
if profiles_dir is None:
profiles_dir = flags.PROFILES_DIR
profiles_dir = get_flags().PROFILES_DIR

raw_profiles = read_profile(profiles_dir)

Expand Down Expand Up @@ -86,7 +86,7 @@ def pre_init_hook(cls, args):

@classmethod
def set_log_format(cls):
if flags.LOG_FORMAT == "json":
if get_flags().LOG_FORMAT == "json":
log_manager.format_json()
else:
log_manager.format_text()
Expand All @@ -102,7 +102,7 @@ def from_args(cls, args, *pargs, **kwargs):
tracking.track_invalid_invocation(args=args, result_type=exc.result_type)
raise dbt.exceptions.DbtRuntimeError("Could not run dbt") from exc
except dbt.exceptions.DbtProfileError as exc:
all_profile_names = list(read_profiles(flags.PROFILES_DIR).keys())
all_profile_names = list(read_profiles(get_flags().PROFILES_DIR).keys())
fire_event(LogDbtProfileError(exc=str(exc), profiles=all_profile_names))
tracking.track_invalid_invocation(args=args, result_type=exc.result_type)
raise dbt.exceptions.DbtRuntimeError("Could not run dbt") from exc
Expand Down
5 changes: 2 additions & 3 deletions core/dbt/task/list.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import json

import dbt.flags

from dbt.contracts.graph.nodes import Exposure, SourceDefinition, Metric
from dbt.flags import get_flags
from dbt.graph import ResourceTypeSelector
from dbt.task.runnable import GraphRunnableTask
from dbt.task.test import TestSelector
Expand Down Expand Up @@ -143,7 +142,7 @@ def output_results(self, results):
"""Log, or output a plain, newline-delimited, and ready-to-pipe list of nodes found."""
for result in results:
self.node_results.append(result)
if dbt.flags.LOG_FORMAT == "json":
if get_flags().LOG_FORMAT == "json":
fire_event(ListCmdOut(msg=result))
else:
# Cleaner to leave as print than to mutate the logger not to print timestamps.
Expand Down
2 changes: 1 addition & 1 deletion core/dbt/tests/fixtures/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ def project(
# Logbook warnings are ignored so we don't have to fork logbook to support python 3.10.
# This _only_ works for tests in `tests/` that use the project fixture.
warnings.filterwarnings("ignore", category=DeprecationWarning, module="logbook")
setup_event_logger(logs_dir, "json", False, False)
setup_event_logger(logs_dir, "json", False, False, False, False)
orig_cwd = os.getcwd()
os.chdir(project_root)
# Return whatever is needed later in tests but can only come from fixtures, so we can keep
Expand Down