From 14433d1abb0f5ee31f0fc9a2224edd8acb4ba08f Mon Sep 17 00:00:00 2001 From: Ben Pankow Date: Thu, 2 Nov 2023 13:52:13 -0700 Subject: [PATCH] [pythonic config] Add warning message when config arg is not named 'config' (#16563) ## Summary Raises a warning message if an op/asset function has a parameter annotated with `dagster.Config` subclass that is not called `config`: ```python >>> @op ... def my_op(not_called_config: MyConfig): ... pass ... ConfigArgumentWarning: Parameter 'not_called_config' on op/asset function 'my_op' was annotated as a dagster.Config type. Did you mean to name this parameter 'config' instead? To mute this warning, invoke warnings.filterwarnings("ignore", category=dagster.ConfigArgumentWarning) or use one of the other methods described at https://docs.python.org/3/library/warnings.html#describing-warning-filters. warnings.warn( ``` Meant to address/improve situations like https://github.com/dagster-io/internal/pull/6683#discussion_r1318072959 The user may quiet this warning by muting the new `dagster.ConfigArgumentWarning` warning type. ## Test Plan New unit test. --- python_modules/dagster/dagster/__init__.py | 5 ++- .../definitions/decorators/op_decorator.py | 13 ++++++- .../dagster/dagster/_utils/warnings.py | 26 +++++++++++++ .../pythonic_config_tests/test_errors.py | 37 +++++++++++++++++++ 4 files changed, 79 insertions(+), 2 deletions(-) diff --git a/python_modules/dagster/dagster/__init__.py b/python_modules/dagster/dagster/__init__.py index 59f1ff0055478..29af3039ade86 100644 --- a/python_modules/dagster/dagster/__init__.py +++ b/python_modules/dagster/dagster/__init__.py @@ -583,7 +583,10 @@ ) from dagster._utils.dagster_type import check_dagster_type as check_dagster_type from dagster._utils.log import get_dagster_logger as get_dagster_logger -from dagster._utils.warnings import ExperimentalWarning as ExperimentalWarning +from dagster._utils.warnings import ( + ConfigArgumentWarning as ConfigArgumentWarning, + ExperimentalWarning as ExperimentalWarning, +) from dagster.version import __version__ as __version__ # ruff: isort: split diff --git a/python_modules/dagster/dagster/_core/definitions/decorators/op_decorator.py b/python_modules/dagster/dagster/_core/definitions/decorators/op_decorator.py index 78b04f790d908..15035035e599c 100644 --- a/python_modules/dagster/dagster/_core/definitions/decorators/op_decorator.py +++ b/python_modules/dagster/dagster/_core/definitions/decorators/op_decorator.py @@ -31,7 +31,7 @@ ) from dagster._core.errors import DagsterInvalidDefinitionError from dagster._core.types.dagster_type import DagsterTypeKind -from dagster._utils.warnings import normalize_renamed_param +from dagster._utils.warnings import config_argument_warning, normalize_renamed_param from ..input import In, InputDefinition from ..output import Out @@ -91,6 +91,8 @@ def __call__(self, fn: Callable[..., Any]) -> "OpDefinition": else NoContextDecoratedOpFunction(decorated_fn=fn) ) + compute_fn.validate_malformed_config() + if compute_fn.has_config_arg(): check.param_invariant( self.config_schema is None or self.config_schema == {}, @@ -302,6 +304,15 @@ def has_config_arg(self) -> bool: return False + def validate_malformed_config(self) -> None: + from dagster._config.pythonic_config.config import Config + from dagster._config.pythonic_config.type_check_utils import safe_is_subclass + + positional_inputs = self.positional_inputs() + for param in get_function_params(self.decorated_fn): + if safe_is_subclass(param.annotation, Config) and param.name in positional_inputs: + config_argument_warning(param.name, self.name) + def get_config_arg(self) -> Parameter: for param in get_function_params(self.decorated_fn): if param.name == "config": diff --git a/python_modules/dagster/dagster/_utils/warnings.py b/python_modules/dagster/dagster/_utils/warnings.py index b7476b7a8db96..5d8f35747bda4 100644 --- a/python_modules/dagster/dagster/_utils/warnings.py +++ b/python_modules/dagster/dagster/_utils/warnings.py @@ -95,6 +95,32 @@ def experimental_warning( ) +# ######################## +# ##### Config arg warning +# ######################## + +CONFIG_WARNING_HELP = ( + "To mute this warning, invoke" + ' warnings.filterwarnings("ignore", category=dagster.ConfigArgumentWarning) or use' + " one of the other methods described at" + " https://docs.python.org/3/library/warnings.html#describing-warning-filters." +) + + +class ConfigArgumentWarning(SyntaxWarning): + pass + + +def config_argument_warning(param_name: str, function_name: str) -> None: + warnings.warn( + f"Parameter '{param_name}' on op/asset function '{function_name}' was annotated as" + " a dagster.Config type. Did you mean to name this parameter 'config'" + " instead?\n\n" + f"{CONFIG_WARNING_HELP}", + ConfigArgumentWarning, + ) + + # ######################## # ##### DISABLE DAGSTER WARNINGS # ######################## diff --git a/python_modules/dagster/dagster_tests/core_tests/pythonic_config_tests/test_errors.py b/python_modules/dagster/dagster_tests/core_tests/pythonic_config_tests/test_errors.py index cf6ce00ad9dd2..562463eb1b183 100644 --- a/python_modules/dagster/dagster_tests/core_tests/pythonic_config_tests/test_errors.py +++ b/python_modules/dagster/dagster_tests/core_tests/pythonic_config_tests/test_errors.py @@ -1,3 +1,4 @@ +import warnings from typing import Tuple import pytest @@ -452,3 +453,39 @@ def test_custom_dagster_type_as_config_type() -> None: class MyOpConfig(Config): dagster_type_field: DagsterDatetime = datetime(year=2023, month=4, day=30) # type: ignore + + +def test_config_named_wrong_thing() -> None: + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + + class DoSomethingConfig(Config): + a_str: str + + assert len(w) == 0 + + @op + def my_op(config_named_wrong: DoSomethingConfig): + pass + + assert len(w) == 1 + assert ( + w[0] + .message.args[0] # type: ignore + .startswith( + "Parameter 'config_named_wrong' on op/asset function 'my_op' was annotated as a dagster.Config type. Did you mean to name this parameter 'config' instead?" + ) + ) + + @asset + def my_asset(config_named_wrong: DoSomethingConfig): + pass + + assert len(w) == 2 + assert ( + w[1] + .message.args[0] # type: ignore + .startswith( + "Parameter 'config_named_wrong' on op/asset function 'my_asset' was annotated as a dagster.Config type. Did you mean to name this parameter 'config' instead?" + ) + )