Skip to content

remove params.validate call in validate_params #59653

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.

Sign up for GitHub

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

Open
henry3260 wants to merge 1 commit into
base: main
Choose a base branch
from
+16 −14
Open

remove params.validate call in validate_params #59653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion providers/standard/src/airflow/providers/standard/operators/hitl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ def validate_params(self) -> None:
Raises:
ValueError: If `"_options"` key is present in `params`, which is not allowed.
"""
self.params.validate()
Copy link
Contributor

@sjyangkevin sjyangkevin Jan 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to share an observation that may help understand the things happen behind. When the DAG is executed, the params will be written to the metadata store. The self.params.validate() include a step checking whether the value is json-serializable.

Here, it is called in the __init__, and the DB write not yet happens, so it may be a little over strict here, but I am interested to understand different views on why we should call this validation here.

Screenshot from 2026-01-08 20-01-07

if "_options" in self.params:
raise ValueError('"_options" is not allowed in params')

Expand Down
29 changes: 16 additions & 13 deletions providers/standard/tests/unit/standard/operators/test_hitl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,21 @@ def test_validate_options(self) -> None:
)
hitl_op.validate_options()

def test_init_allow_no_default_param(self):
try:
op = HITLOperator(
task_id="wait_for_human",
subject="Please input data",
options=["Submit"],
params={"user_input_data": Param(type="string", description="No default here!")},
)
except Exception as e:
pytest.fail(f"Initialization failed with exception: {e}")

param_obj = op.params.get_param("user_input_data")
assert isinstance(param_obj, Param)
assert "user_input_data" in op.params

def test_validate_options_with_empty_options(self) -> None:
# validate_options is called during initialization
with pytest.raises(ValueError, match='"options" cannot be empty.'):
Expand All @@ -138,19 +153,7 @@ def test_validate_options_with_empty_options(self) -> None:

@pytest.mark.parametrize(
("params", "exc", "error_msg"),
(
(ParamsDict({"_options": 1}), ValueError, '"_options" is not allowed in params'),
(
ParamsDict({"param": Param("", type="integer")}),
ParamValidationError,
(
"Invalid input for param param: '' is not of type 'integer'\n\n"
"Failed validating 'type' in schema:\n"
" {'type': 'integer'}\n\n"
"On instance:\n ''"
),
),
),
Comment on lines -144 to -153
Copy link
Contributor

@sjyangkevin sjyangkevin Jan 9, 2026

Choose a reason for hiding this comment

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

Thanks for proposing the change. Wonder if this test case is removed. I think it is checking for the situation "when a default value is provided for the param, the default value should be valid for the type".

the proposed change seems to make the default value optional, but we should probably keep this test case for the situation that default is provided.

((ParamsDict({"_options": 1}), ValueError, '"_options" is not allowed in params'),),
)
def test_validate_params(
self, params: ParamsDict, exc: type[ValueError | ParamValidationError], error_msg: str
Expand Down
Loading