Skip to content

Commit

Permalink
[pythonic config] Add warning message when config arg is not named 'c…
Browse files Browse the repository at this point in the history
…onfig' (#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
dagster-io/internal#6683 (comment)

The user may quiet this warning by muting the new
`dagster.ConfigArgumentWarning` warning type.

## Test Plan

New unit test.
  • Loading branch information
benpankow authored Nov 2, 2023
1 parent 2f64ce1 commit 14433d1
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 2 deletions.
5 changes: 4 additions & 1 deletion python_modules/dagster/dagster/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 == {},
Expand Down Expand Up @@ -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":
Expand Down
26 changes: 26 additions & 0 deletions python_modules/dagster/dagster/_utils/warnings.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
# ########################
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import warnings
from typing import Tuple

import pytest
Expand Down Expand Up @@ -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?"
)
)

0 comments on commit 14433d1

Please sign in to comment.