You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "WangzJi (via GitHub)" <gi...@apache.org> on 2025/06/22 12:13:07 UTC
[PR] Fix ParamsDict.get() ignoring default values for None parameters [airflow]
WangzJi opened a new pull request, #52019:
URL: https://github.com/apache/airflow/pull/52019
- Add custom get() method to ParamsDict class that returns default value when resolved parameter is None
- Fix issue where dag.params.get('key', default) would return None instead of default for parameters with default=None
- Add comprehensive test cases for ParamsDict.get() method behavior
- Update documentation with usage examples for params.get() with default values
Closes #51977
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ParamsDict.get() ignoring default values for None parameters [airflow]
Posted by "WangzJi (via GitHub)" <gi...@apache.org>.
WangzJi closed pull request #52019: Fix ParamsDict.get() ignoring default values for None parameters
URL: https://github.com/apache/airflow/pull/52019
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ParamsDict.get() ignoring default values for None parameters [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #52019:
URL: https://github.com/apache/airflow/pull/52019#issuecomment-2994220840
> I think this behavior change needs a bit more discussion as it changes the behavior of the handling of defaults in params which might affect a lot of existing Dags of users. I do not see this as a bug.
Agree. We can decide to stay with the current behaviour and document it as well - possibly worth several people's opinion.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ParamsDict.get() ignoring default values for None parameters [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #52019:
URL: https://github.com/apache/airflow/pull/52019#discussion_r2160340075
##########
task-sdk/src/airflow/sdk/definitions/param.py:
##########
@@ -218,6 +218,27 @@ def __getitem__(self, key: str) -> Any:
param = self.__dict[key]
return param.resolve(suppress_exception=self.suppress_exception)
+ def get(self, key: str, default: Any = None) -> Any:
+ """
+ Get the value for a key, returning default if the key is not found OR if the resolved value is None.
+
+ This method properly handles the case where a parameter exists but has a None value,
+ which should be treated as if the key doesn't exist when a default is provided.
+
+ :param key: The key to fetch
+ :param default: The default value to return if key is not found or resolved value is None
+ """
+ try:
+ param = self.__dict[key]
+ resolved_value = param.resolve(suppress_exception=self.suppress_exception)
+ # If the resolved value is None and a default is provided, return the default
+ # This matches the expected behavior described in GitHub issue #51977
+ if resolved_value is None and default is not None:
+ return default
Review Comment:
I dis-agree to this behavior change.
If a param is defined using `None`as default by the Dag author, why should a method using define another default that overrides the previous.
There might be good reasons having a parameter as `None` and also empty strings are passed as `None` and assuning a `.get()` provides another level of default potentially over-riding the explicitly passed `None` is wrong in my view.
For me this is a different behavior and if somebody needs it it can be well achieved with other Python options like `params['key'] or 'default'` which make clear that you want to override the params default.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ParamsDict.get() ignoring default values for None parameters [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #52019:
URL: https://github.com/apache/airflow/pull/52019#discussion_r2160340075
##########
task-sdk/src/airflow/sdk/definitions/param.py:
##########
@@ -218,6 +218,27 @@ def __getitem__(self, key: str) -> Any:
param = self.__dict[key]
return param.resolve(suppress_exception=self.suppress_exception)
+ def get(self, key: str, default: Any = None) -> Any:
+ """
+ Get the value for a key, returning default if the key is not found OR if the resolved value is None.
+
+ This method properly handles the case where a parameter exists but has a None value,
+ which should be treated as if the key doesn't exist when a default is provided.
+
+ :param key: The key to fetch
+ :param default: The default value to return if key is not found or resolved value is None
+ """
+ try:
+ param = self.__dict[key]
+ resolved_value = param.resolve(suppress_exception=self.suppress_exception)
+ # If the resolved value is None and a default is provided, return the default
+ # This matches the expected behavior described in GitHub issue #51977
+ if resolved_value is None and default is not None:
+ return default
Review Comment:
I dis-agree to this behavior change.
If a param is defined using `None`as default by the Dag author, why should a method using define another default that overrides the previous.
There might be good reasons having a parameter as `None` and also empty strings are passed as `None` and assuning a `.get()` provides another level of default potentially over-riding the explicitly passed `None` is wrong in my view.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ParamsDict.get() ignoring default values for None parameters [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #52019:
URL: https://github.com/apache/airflow/pull/52019#discussion_r2160339203
##########
task-sdk/src/airflow/sdk/definitions/param.py:
##########
@@ -218,6 +218,27 @@ def __getitem__(self, key: str) -> Any:
param = self.__dict[key]
return param.resolve(suppress_exception=self.suppress_exception)
+ def get(self, key: str, default: Any = None) -> Any:
+ """
+ Get the value for a key, returning default if the key is not found OR if the resolved value is None.
+
+ This method properly handles the case where a parameter exists but has a None value,
+ which should be treated as if the key doesn't exist when a default is provided.
+
+ :param key: The key to fetch
+ :param default: The default value to return if key is not found or resolved value is None
+ """
+ try:
+ param = self.__dict[key]
Review Comment:
Correct would be __dict__
```suggestion
param = self.__dict__[key]
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ParamsDict.get() ignoring default values for None parameters [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #52019:
URL: https://github.com/apache/airflow/pull/52019#issuecomment-2994221284
But my opinion is that overriding default with get is good idea :D
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ParamsDict.get() ignoring default values for None parameters [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #52019:
URL: https://github.com/apache/airflow/pull/52019#discussion_r2160615985
##########
task-sdk/src/airflow/sdk/definitions/param.py:
##########
@@ -218,6 +218,27 @@ def __getitem__(self, key: str) -> Any:
param = self.__dict[key]
return param.resolve(suppress_exception=self.suppress_exception)
+ def get(self, key: str, default: Any = None) -> Any:
+ """
+ Get the value for a key, returning default if the key is not found OR if the resolved value is None.
+
+ This method properly handles the case where a parameter exists but has a None value,
+ which should be treated as if the key doesn't exist when a default is provided.
+
+ :param key: The key to fetch
+ :param default: The default value to return if key is not found or resolved value is None
+ """
+ try:
+ param = self.__dict[key]
+ resolved_value = param.resolve(suppress_exception=self.suppress_exception)
+ # If the resolved value is None and a default is provided, return the default
+ # This matches the expected behavior described in GitHub issue #51977
+ if resolved_value is None and default is not None:
+ return default
Review Comment:
I agree, if the resolved value is None, I should be able to get None. That’s how dict works in Python.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ParamsDict.get() ignoring default values for None parameters [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #52019:
URL: https://github.com/apache/airflow/pull/52019#issuecomment-2994222613
But also I agree I have a hard time coming with a use case where such override would be useful.
Maybe someone can come up with realistic case ?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org