Copilot commented on code in PR #62174:
URL: https://github.com/apache/airflow/pull/62174#discussion_r2972345895
##########
task-sdk/src/airflow/sdk/bases/decorator.py:
##########
@@ -342,6 +369,17 @@ def __init__(
else:
signature.bind(*op_args, **op_kwargs)
+ # Params in injected_for_ordering are semantically required even
though they received a
+ # None default to satisfy Python's ordering constraint. Verify they
are actually provided.
+ if injected_for_ordering and not
kwargs.get("_airflow_mapped_validation_only"):
+ positional_param_names = [p.name for p in parameters if p.kind in
positional_kinds]
+ covered_by_pos = set(positional_param_names[: len(op_args)])
+ provided = covered_by_pos | set(op_kwargs.keys())
+ missing = injected_for_ordering - provided
+ if missing:
+ missing_str = ", ".join(sorted(missing))
+ raise TypeError(f"missing required argument(s): {missing_str}")
Review Comment:
Consider aligning this error message with the wording/format produced by
`inspect.Signature.bind()` for missing required parameters (e.g. quoting
parameter names). Since this is user-facing during DAG parse, consistency can
make it easier to understand and search for.
```suggestion
missing_names = sorted(missing)
formatted_missing = ", ".join(f"'{name}'" for name in
missing_names)
if len(missing_names) == 1:
raise TypeError(f"missing a required argument:
{formatted_missing}")
raise TypeError(f"missing {len(missing_names)} required
arguments: {formatted_missing}")
```
##########
task-sdk/tests/task_sdk/bases/test_decorator.py:
##########
@@ -69,6 +69,160 @@ def
test_get_python_source_strips_decorator_and_comment(self, tmp_path: Path):
assert cleaned.lstrip().splitlines()[0].startswith("def a_task")
+class DummyDecoratedOperator(DecoratedOperator):
+ custom_operator_name = "@task.dummy"
+
+ def execute(self, context):
+ return self.python_callable(*self.op_args, **self.op_kwargs)
+
+
+class TestDefaultFillingLogic:
+ @pytest.mark.parametrize(
+ ("func", "kwargs", "args"),
+ [
+ pytest.param(
+ lambda: 42,
+ {},
+ [],
+ id="no_params",
+ ),
+ pytest.param(
+ lambda x, y=99: (x, y),
+ {"x": 1},
+ [],
+ id="param_after_first_default_is_given_none",
+ ),
+ pytest.param(
+ lambda a, b=5, c=None: (a, b, c),
+ {"a": 1},
+ [],
+ id="all_params_after_first_default_already_have_defaults",
+ ),
+ pytest.param(
+ lambda a, b, c=99: (a, b, c),
+ {},
+ [1, 2],
+ id="single_trailing_optional",
+ ),
+ ],
+ )
+ def test_construction_succeeds(self, func, kwargs, args):
+ op = make_op(func, op_kwargs=kwargs, op_args=args)
+ assert op is not None
+
+ def test_construction_succeeds_with_context_key_params(self):
+ def foo(ds, my_data):
+ return my_data
+
+ assert make_op(foo, op_args=[None, None]) is not None
+
+ def test_context_key_default_none_does_not_raise(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
+ ctx_key = next(iter(sorted(KNOWN_CONTEXT_KEYS)))
+ f = _make_func(f"def dummy_task(x, {ctx_key}=None): return x")
+ assert make_op(f, op_kwargs={"x": 1}) is not None
+
+ def test_context_key_with_non_none_default_raises(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
+ ctx_key = next(iter(sorted(KNOWN_CONTEXT_KEYS)))
+ f = _make_func(f"def dummy_task(x, {ctx_key}='bad_default'): return x")
+ with pytest.raises(ValueError, match="can't have a default other than
None"):
+ make_op(f, op_kwargs={"x": 1})
+
+ @pytest.mark.parametrize(
+ ("func_src", "op_kwargs"),
+ [
+ pytest.param(
+ "def dummy_task({ctx0}, x, y=10): return (x, y)",
+ {"x": 1},
+ id="context_key_before_first_default_shifts_boundary",
+ ),
+ pytest.param(
+ "def dummy_task(x, y=5, {ctx0}=None): return (x, y)",
+ {"x": 1},
+ id="context_key_after_regular_default",
+ ),
+ pytest.param(
+ "def dummy_task(a, {ctx0}=None, b=7, {ctx1}=None): return (a,
b)",
+ {"a": 1},
+ id="multiple_context_keys_mixed_with_regular_defaults",
+ ),
+ pytest.param(
+ "def dummy_task({ctx0}, x, y=10): return (x, y)",
+ {"x": 42},
+
id="required_param_between_context_key_and_regular_default_gets_none",
+ ),
+ pytest.param(
+ "def dummy_task({ctx0}=None, {ctx1}=None, {ctx2}=None): return
True",
+ {},
+ id="context_key_only_signature",
+ ),
+ ],
+ )
+ def test_context_key_construction_succeeds(self, func_src, op_kwargs):
+ """All context-key signature shapes must construct without raising."""
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
+ ctx_keys = sorted(KNOWN_CONTEXT_KEYS)
+ src = func_src.format(
+ ctx0=ctx_keys[0],
+ ctx1=ctx_keys[1] if len(ctx_keys) > 1 else ctx_keys[0],
+ ctx2=ctx_keys[2] if len(ctx_keys) > 2 else ctx_keys[0],
+ )
+ op = make_op(_make_func(src), op_kwargs=op_kwargs)
+ assert op is not None
+
+ def test_non_context_param_after_context_key_gets_none_injected(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
+ ctx_key = next(iter(sorted(KNOWN_CONTEXT_KEYS)))
+ f = _make_func(f"def dummy_task({ctx_key}, a): ...")
+ assert make_op(f, op_kwargs={"a": "2024-01-01"}) is not None
+
+ def test_non_context_param_after_context_key_without_value_raises(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
Review Comment:
Move the `KNOWN_CONTEXT_KEYS` import to module scope (and reuse it) instead
of importing inside the test function. This file currently uses top-level
imports, and the repeated local imports add noise and can trip
import-order/lint rules.
##########
task-sdk/tests/task_sdk/bases/test_decorator.py:
##########
@@ -69,6 +69,160 @@ def
test_get_python_source_strips_decorator_and_comment(self, tmp_path: Path):
assert cleaned.lstrip().splitlines()[0].startswith("def a_task")
+class DummyDecoratedOperator(DecoratedOperator):
+ custom_operator_name = "@task.dummy"
+
+ def execute(self, context):
+ return self.python_callable(*self.op_args, **self.op_kwargs)
+
+
+class TestDefaultFillingLogic:
+ @pytest.mark.parametrize(
+ ("func", "kwargs", "args"),
+ [
+ pytest.param(
+ lambda: 42,
+ {},
+ [],
+ id="no_params",
+ ),
+ pytest.param(
+ lambda x, y=99: (x, y),
+ {"x": 1},
+ [],
+ id="param_after_first_default_is_given_none",
Review Comment:
The parametrized case id `param_after_first_default_is_given_none` is
misleading: this lambda signature is already valid and nothing is actually
being “given None”. Consider renaming the id to describe the scenario being
validated (e.g. that construction succeeds with existing defaults).
```suggestion
id="required_param_with_trailing_default_uses_existing_default",
```
##########
task-sdk/tests/task_sdk/bases/test_decorator.py:
##########
@@ -69,6 +69,160 @@ def
test_get_python_source_strips_decorator_and_comment(self, tmp_path: Path):
assert cleaned.lstrip().splitlines()[0].startswith("def a_task")
+class DummyDecoratedOperator(DecoratedOperator):
+ custom_operator_name = "@task.dummy"
+
+ def execute(self, context):
+ return self.python_callable(*self.op_args, **self.op_kwargs)
+
+
+class TestDefaultFillingLogic:
+ @pytest.mark.parametrize(
+ ("func", "kwargs", "args"),
+ [
+ pytest.param(
+ lambda: 42,
+ {},
+ [],
+ id="no_params",
+ ),
+ pytest.param(
+ lambda x, y=99: (x, y),
+ {"x": 1},
+ [],
+ id="param_after_first_default_is_given_none",
+ ),
+ pytest.param(
+ lambda a, b=5, c=None: (a, b, c),
+ {"a": 1},
+ [],
+ id="all_params_after_first_default_already_have_defaults",
+ ),
+ pytest.param(
+ lambda a, b, c=99: (a, b, c),
+ {},
+ [1, 2],
+ id="single_trailing_optional",
+ ),
+ ],
+ )
+ def test_construction_succeeds(self, func, kwargs, args):
+ op = make_op(func, op_kwargs=kwargs, op_args=args)
+ assert op is not None
+
+ def test_construction_succeeds_with_context_key_params(self):
+ def foo(ds, my_data):
+ return my_data
+
+ assert make_op(foo, op_args=[None, None]) is not None
+
+ def test_context_key_default_none_does_not_raise(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
+ ctx_key = next(iter(sorted(KNOWN_CONTEXT_KEYS)))
+ f = _make_func(f"def dummy_task(x, {ctx_key}=None): return x")
+ assert make_op(f, op_kwargs={"x": 1}) is not None
+
+ def test_context_key_with_non_none_default_raises(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
+ ctx_key = next(iter(sorted(KNOWN_CONTEXT_KEYS)))
+ f = _make_func(f"def dummy_task(x, {ctx_key}='bad_default'): return x")
+ with pytest.raises(ValueError, match="can't have a default other than
None"):
+ make_op(f, op_kwargs={"x": 1})
+
+ @pytest.mark.parametrize(
+ ("func_src", "op_kwargs"),
+ [
+ pytest.param(
+ "def dummy_task({ctx0}, x, y=10): return (x, y)",
+ {"x": 1},
+ id="context_key_before_first_default_shifts_boundary",
+ ),
+ pytest.param(
+ "def dummy_task(x, y=5, {ctx0}=None): return (x, y)",
+ {"x": 1},
+ id="context_key_after_regular_default",
+ ),
+ pytest.param(
+ "def dummy_task(a, {ctx0}=None, b=7, {ctx1}=None): return (a,
b)",
+ {"a": 1},
+ id="multiple_context_keys_mixed_with_regular_defaults",
+ ),
+ pytest.param(
+ "def dummy_task({ctx0}, x, y=10): return (x, y)",
+ {"x": 42},
+
id="required_param_between_context_key_and_regular_default_gets_none",
+ ),
+ pytest.param(
+ "def dummy_task({ctx0}=None, {ctx1}=None, {ctx2}=None): return
True",
+ {},
+ id="context_key_only_signature",
+ ),
+ ],
+ )
+ def test_context_key_construction_succeeds(self, func_src, op_kwargs):
+ """All context-key signature shapes must construct without raising."""
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
Review Comment:
Move the `KNOWN_CONTEXT_KEYS` import to module scope (and reuse it) instead
of importing inside the test function. This file currently uses top-level
imports, and the repeated local imports add noise and can trip
import-order/lint rules.
##########
task-sdk/tests/task_sdk/bases/test_decorator.py:
##########
@@ -69,6 +69,160 @@ def
test_get_python_source_strips_decorator_and_comment(self, tmp_path: Path):
assert cleaned.lstrip().splitlines()[0].startswith("def a_task")
+class DummyDecoratedOperator(DecoratedOperator):
+ custom_operator_name = "@task.dummy"
+
+ def execute(self, context):
+ return self.python_callable(*self.op_args, **self.op_kwargs)
+
+
+class TestDefaultFillingLogic:
+ @pytest.mark.parametrize(
+ ("func", "kwargs", "args"),
+ [
+ pytest.param(
+ lambda: 42,
+ {},
+ [],
+ id="no_params",
+ ),
+ pytest.param(
+ lambda x, y=99: (x, y),
+ {"x": 1},
+ [],
+ id="param_after_first_default_is_given_none",
+ ),
+ pytest.param(
+ lambda a, b=5, c=None: (a, b, c),
+ {"a": 1},
+ [],
+ id="all_params_after_first_default_already_have_defaults",
+ ),
+ pytest.param(
+ lambda a, b, c=99: (a, b, c),
+ {},
+ [1, 2],
+ id="single_trailing_optional",
+ ),
+ ],
+ )
+ def test_construction_succeeds(self, func, kwargs, args):
+ op = make_op(func, op_kwargs=kwargs, op_args=args)
+ assert op is not None
+
+ def test_construction_succeeds_with_context_key_params(self):
+ def foo(ds, my_data):
+ return my_data
+
+ assert make_op(foo, op_args=[None, None]) is not None
+
+ def test_context_key_default_none_does_not_raise(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
+ ctx_key = next(iter(sorted(KNOWN_CONTEXT_KEYS)))
+ f = _make_func(f"def dummy_task(x, {ctx_key}=None): return x")
+ assert make_op(f, op_kwargs={"x": 1}) is not None
+
+ def test_context_key_with_non_none_default_raises(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
+ ctx_key = next(iter(sorted(KNOWN_CONTEXT_KEYS)))
+ f = _make_func(f"def dummy_task(x, {ctx_key}='bad_default'): return x")
+ with pytest.raises(ValueError, match="can't have a default other than
None"):
+ make_op(f, op_kwargs={"x": 1})
+
+ @pytest.mark.parametrize(
+ ("func_src", "op_kwargs"),
+ [
+ pytest.param(
+ "def dummy_task({ctx0}, x, y=10): return (x, y)",
+ {"x": 1},
+ id="context_key_before_first_default_shifts_boundary",
+ ),
+ pytest.param(
+ "def dummy_task(x, y=5, {ctx0}=None): return (x, y)",
+ {"x": 1},
+ id="context_key_after_regular_default",
+ ),
+ pytest.param(
+ "def dummy_task(a, {ctx0}=None, b=7, {ctx1}=None): return (a,
b)",
+ {"a": 1},
+ id="multiple_context_keys_mixed_with_regular_defaults",
+ ),
+ pytest.param(
+ "def dummy_task({ctx0}, x, y=10): return (x, y)",
+ {"x": 42},
+
id="required_param_between_context_key_and_regular_default_gets_none",
+ ),
+ pytest.param(
+ "def dummy_task({ctx0}=None, {ctx1}=None, {ctx2}=None): return
True",
+ {},
+ id="context_key_only_signature",
+ ),
+ ],
+ )
+ def test_context_key_construction_succeeds(self, func_src, op_kwargs):
+ """All context-key signature shapes must construct without raising."""
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
+ ctx_keys = sorted(KNOWN_CONTEXT_KEYS)
+ src = func_src.format(
+ ctx0=ctx_keys[0],
+ ctx1=ctx_keys[1] if len(ctx_keys) > 1 else ctx_keys[0],
+ ctx2=ctx_keys[2] if len(ctx_keys) > 2 else ctx_keys[0],
+ )
+ op = make_op(_make_func(src), op_kwargs=op_kwargs)
+ assert op is not None
+
+ def test_non_context_param_after_context_key_gets_none_injected(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
Review Comment:
Move the `KNOWN_CONTEXT_KEYS` import to module scope (and reuse it) instead
of importing inside the test function. This file currently uses top-level
imports, and the repeated local imports add noise and can trip
import-order/lint rules.
##########
task-sdk/tests/task_sdk/bases/test_decorator.py:
##########
@@ -69,6 +69,160 @@ def
test_get_python_source_strips_decorator_and_comment(self, tmp_path: Path):
assert cleaned.lstrip().splitlines()[0].startswith("def a_task")
+class DummyDecoratedOperator(DecoratedOperator):
+ custom_operator_name = "@task.dummy"
+
+ def execute(self, context):
+ return self.python_callable(*self.op_args, **self.op_kwargs)
+
+
+class TestDefaultFillingLogic:
+ @pytest.mark.parametrize(
+ ("func", "kwargs", "args"),
+ [
+ pytest.param(
+ lambda: 42,
+ {},
+ [],
+ id="no_params",
+ ),
+ pytest.param(
+ lambda x, y=99: (x, y),
+ {"x": 1},
+ [],
+ id="param_after_first_default_is_given_none",
+ ),
+ pytest.param(
+ lambda a, b=5, c=None: (a, b, c),
+ {"a": 1},
+ [],
+ id="all_params_after_first_default_already_have_defaults",
+ ),
+ pytest.param(
+ lambda a, b, c=99: (a, b, c),
+ {},
+ [1, 2],
+ id="single_trailing_optional",
+ ),
+ ],
+ )
+ def test_construction_succeeds(self, func, kwargs, args):
+ op = make_op(func, op_kwargs=kwargs, op_args=args)
+ assert op is not None
+
+ def test_construction_succeeds_with_context_key_params(self):
+ def foo(ds, my_data):
+ return my_data
+
+ assert make_op(foo, op_args=[None, None]) is not None
+
+ def test_context_key_default_none_does_not_raise(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
Review Comment:
Move the `KNOWN_CONTEXT_KEYS` import to module scope (and reuse it) instead
of importing inside the test function. This file currently uses top-level
imports, and the repeated local imports add noise and can trip
import-order/lint rules.
##########
task-sdk/tests/task_sdk/bases/test_decorator.py:
##########
@@ -69,6 +69,160 @@ def
test_get_python_source_strips_decorator_and_comment(self, tmp_path: Path):
assert cleaned.lstrip().splitlines()[0].startswith("def a_task")
+class DummyDecoratedOperator(DecoratedOperator):
+ custom_operator_name = "@task.dummy"
+
+ def execute(self, context):
+ return self.python_callable(*self.op_args, **self.op_kwargs)
+
+
+class TestDefaultFillingLogic:
+ @pytest.mark.parametrize(
+ ("func", "kwargs", "args"),
+ [
+ pytest.param(
+ lambda: 42,
+ {},
+ [],
+ id="no_params",
+ ),
+ pytest.param(
+ lambda x, y=99: (x, y),
+ {"x": 1},
+ [],
+ id="param_after_first_default_is_given_none",
+ ),
+ pytest.param(
+ lambda a, b=5, c=None: (a, b, c),
+ {"a": 1},
+ [],
+ id="all_params_after_first_default_already_have_defaults",
+ ),
+ pytest.param(
+ lambda a, b, c=99: (a, b, c),
+ {},
+ [1, 2],
+ id="single_trailing_optional",
+ ),
+ ],
+ )
+ def test_construction_succeeds(self, func, kwargs, args):
+ op = make_op(func, op_kwargs=kwargs, op_args=args)
+ assert op is not None
+
+ def test_construction_succeeds_with_context_key_params(self):
+ def foo(ds, my_data):
+ return my_data
+
+ assert make_op(foo, op_args=[None, None]) is not None
+
+ def test_context_key_default_none_does_not_raise(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
+ ctx_key = next(iter(sorted(KNOWN_CONTEXT_KEYS)))
+ f = _make_func(f"def dummy_task(x, {ctx_key}=None): return x")
+ assert make_op(f, op_kwargs={"x": 1}) is not None
+
+ def test_context_key_with_non_none_default_raises(self):
+ from airflow.sdk.bases.decorator import KNOWN_CONTEXT_KEYS
+
Review Comment:
Move the `KNOWN_CONTEXT_KEYS` import to module scope (and reuse it) instead
of importing inside the test function. This file currently uses top-level
imports, and the repeated local imports add noise and can trip
import-order/lint rules.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
