remove params.validate call in validate_params #59653
Conversation
henry3260
force-pushed
the
fix-hitlop
branch
2 times, most recently
from
6cd55e6 to
5983b44
Compare
henry3260
changed the title
henry3260
force-pushed
the
fix-hitlop
branch
from
5983b44 to
34dd860
Compare
suii2210
left a comment
suii2210
left a comment
There was a problem hiding this comment.
hello @henry3260
I reviewed your PR and this looks good to me.
test_init_allow_no_default_param clearly captures the reported regression and ensures HITLOperator can be initialized with params lacking defaults, which matches expected HITL behavior.
The test scope is appropriate (init-time only), and removing the previous ParamValidationError case here makes sense since validation is intentionally deferred.
|
Optional follow-up: we could consider adding runtime validation tests in a separate PR if we want extra coverage, but this looks good to merge as-is. |
Do you want to open a pr for this? |
I think no need of this, your PR is already covering all things . |
Feel free to use the 'Suggested Changes' feature for any code improvements. I'll be happy to apply them directly! |
Ok then, Lets do it !! |
| Raises: | ||
| ValueError: If `"_options"` key is present in `params`, which is not allowed. | ||
| """ | ||
| self.params.validate() |
There was a problem hiding this 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 different views on why we should call this validation here.
| 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 ''" | ||
| ), | ||
| ), | ||
| ), |
There was a problem hiding this 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.
3 participants
closes: #59551
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.