sjyangkevin commented on code in PR #59653:
URL: https://github.com/apache/airflow/pull/59653#discussion_r2674417720
##########
providers/standard/tests/unit/standard/operators/test_hitl.py:
##########
@@ -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'),
- (
Review Comment:
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.
##########
providers/standard/src/airflow/providers/standard/operators/hitl.py:
##########
@@ -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()
Review Comment:
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 for
different views on why we should call this validation here.
<img width="944" height="105" alt="Screenshot from 2026-01-08 20-01-07"
src="https://github.com/user-attachments/assets/14562c3f-b594-4869-9aeb-c981a75c25b0";
/>
--
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]
