You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "kevinalh (via GitHub)" <gi...@apache.org> on 2024/03/10 07:34:28 UTC
[PR] Context parameters fix [airflow]
kevinalh opened a new pull request, #38015:
URL: https://github.com/apache/airflow/pull/38015
<!--
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/
-->
This PR does two things for the Python task decorator:
1. Don't allow a default parameter other than None for context parameters, since this is always replaced by None in 2.8. Keeping this behavior allowed would only bring confusion as it's ambiguous. Suggested by @potiuk in https://github.com/apache/airflow/pull/38007#pullrequestreview-1926201627.
2. Improve the error message for cases where the replacement of default values breaks the function signature. For this I opted for a simple try-catch where we let the Python traceback provide the exact nature of the failure.
Closes: #38006
<!-- 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 [newsfragments](https://github.com/apache/airflow/tree/main/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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #38015:
URL: https://github.com/apache/airflow/pull/38015#discussion_r1519372994
##########
airflow/decorators/base.py:
##########
@@ -208,11 +208,36 @@ def __init__(
# since values for those will be provided when the task is run. Since
# we're not actually running the function, None is good enough here.
signature = inspect.signature(python_callable)
+
+ # Don't allow context argument defaults other than None to avoid ambiguities.
+ if any(
+ param.name in KNOWN_CONTEXT_KEYS and param.default not in (None, inspect.Parameter.empty)
+ for param in signature.parameters.values()
+ ):
+ raise ValueError("Context key parameters can't have a default other than None")
+
parameters = [
param.replace(default=None) if param.name in KNOWN_CONTEXT_KEYS else param
for param in signature.parameters.values()
]
- signature = signature.replace(parameters=parameters)
+ try:
+ signature = signature.replace(parameters=parameters)
+ except ValueError as err:
+ raise ValueError(
+ textwrap.dedent(
+ f"""
+ The function signature broke while assigning defaults to context key parameters.
+
+ The decorator is replacing the signature
+ > {python_callable.__name__}({', '.join(str(param) for param in signature.parameters.values())})
+
+ with
+ > {python_callable.__name__}({', '.join(str(param) for param in parameters)})
+
+ which isn't valid: {err}
+ """
+ )
Review Comment:
Smal nit. I would rather put this into the variable and use it as exception message, it improve readability of the traceback.
eg.
```python
msg = textwrap.dedent
...
)
raise ValueError(msg)
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #38015:
URL: https://github.com/apache/airflow/pull/38015
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #38015:
URL: https://github.com/apache/airflow/pull/38015#discussion_r1521946748
##########
airflow/decorators/base.py:
##########
@@ -208,11 +208,35 @@ def __init__(
# since values for those will be provided when the task is run. Since
# we're not actually running the function, None is good enough here.
signature = inspect.signature(python_callable)
+
+ # Don't allow context argument defaults other than None to avoid ambiguities.
+ if any(
+ param.name in KNOWN_CONTEXT_KEYS and param.default not in (None, inspect.Parameter.empty)
+ for param in signature.parameters.values()
+ ):
+ raise ValueError("Context key parameters can't have a default other than None")
+
parameters = [
param.replace(default=None) if param.name in KNOWN_CONTEXT_KEYS else param
for param in signature.parameters.values()
]
- signature = signature.replace(parameters=parameters)
+ try:
+ signature = signature.replace(parameters=parameters)
+ except ValueError as err:
+ message = textwrap.dedent(
+ f"""
+ The function signature broke while assigning defaults to context key parameters.
+
+ The decorator is replacing the signature
+ > {python_callable.__name__}({', '.join(str(param) for param in signature.parameters.values())})
+
+ with
+ > {python_callable.__name__}({', '.join(str(param) for param in parameters)})
+
+ which isn't valid: {err}
+ """
+ )
+ raise ValueError(message)
Review Comment:
```suggestion
raise ValueError(message) from err
```
Cleaener traceback
##########
airflow/decorators/base.py:
##########
@@ -208,11 +208,35 @@ def __init__(
# since values for those will be provided when the task is run. Since
# we're not actually running the function, None is good enough here.
signature = inspect.signature(python_callable)
+
+ # Don't allow context argument defaults other than None to avoid ambiguities.
+ if any(
+ param.name in KNOWN_CONTEXT_KEYS and param.default not in (None, inspect.Parameter.empty)
+ for param in signature.parameters.values()
+ ):
+ raise ValueError("Context key parameters can't have a default other than None")
+
parameters = [
param.replace(default=None) if param.name in KNOWN_CONTEXT_KEYS else param
for param in signature.parameters.values()
]
- signature = signature.replace(parameters=parameters)
+ try:
+ signature = signature.replace(parameters=parameters)
+ except ValueError as err:
+ message = textwrap.dedent(
+ f"""
+ The function signature broke while assigning defaults to context key parameters.
+
+ The decorator is replacing the signature
+ > {python_callable.__name__}({', '.join(str(param) for param in signature.parameters.values())})
+
+ with
+ > {python_callable.__name__}({', '.join(str(param) for param in parameters)})
+
+ which isn't valid: {err}
+ """
+ )
+ raise ValueError(message)
Review Comment:
```suggestion
raise ValueError(message) from err
```
Cleaner traceback
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38015:
URL: https://github.com/apache/airflow/pull/38015#discussion_r1518758684
##########
airflow/decorators/base.py:
##########
@@ -208,11 +208,25 @@ def __init__(
# since values for those will be provided when the task is run. Since
# we're not actually running the function, None is good enough here.
signature = inspect.signature(python_callable)
+
+ # Don't allow context argument defaults other than None to avoid ambiguities.
+ if any(
+ param.name in KNOWN_CONTEXT_KEYS and param.default not in (None, inspect.Parameter.empty)
+ for param in signature.parameters.values()
+ ):
+ raise ValueError("Context key parameters can't have a default other than None")
+
parameters = [
param.replace(default=None) if param.name in KNOWN_CONTEXT_KEYS else param
for param in signature.parameters.values()
]
- signature = signature.replace(parameters=parameters)
+ try:
+ signature = signature.replace(parameters=parameters)
+ except ValueError:
+ raise ValueError(
+ "The function signature broke while assigning defaults to context key parameters. "
+ "You need to change their position in the parameter list"
Review Comment:
I think we need a little more explicit message here. Maybe a wrong examples and what the user is expected to do ?
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #38015:
URL: https://github.com/apache/airflow/pull/38015#issuecomment-2041529236
> If we can add the newsfragtment on the docs this woul dbe great for others potentially stumbling into it.
Agree :)
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "kevinalh (via GitHub)" <gi...@apache.org>.
kevinalh commented on code in PR #38015:
URL: https://github.com/apache/airflow/pull/38015#discussion_r1527170374
##########
airflow/decorators/base.py:
##########
@@ -208,11 +208,35 @@ def __init__(
# since values for those will be provided when the task is run. Since
# we're not actually running the function, None is good enough here.
signature = inspect.signature(python_callable)
+
+ # Don't allow context argument defaults other than None to avoid ambiguities.
+ if any(
+ param.name in KNOWN_CONTEXT_KEYS and param.default not in (None, inspect.Parameter.empty)
+ for param in signature.parameters.values()
+ ):
+ raise ValueError("Context key parameters can't have a default other than None")
Review Comment:
New error message:
```
ValueError: Context key parameter logical_date can't have a default other than None
```
Definitely helps debugging. We could also make it print all (or a subset) of distinct faulty context key parameters, but this should be enough in my opinion. What do you think?
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "kevinalh (via GitHub)" <gi...@apache.org>.
kevinalh commented on code in PR #38015:
URL: https://github.com/apache/airflow/pull/38015#discussion_r1518928827
##########
airflow/decorators/base.py:
##########
@@ -208,11 +208,25 @@ def __init__(
# since values for those will be provided when the task is run. Since
# we're not actually running the function, None is good enough here.
signature = inspect.signature(python_callable)
+
+ # Don't allow context argument defaults other than None to avoid ambiguities.
+ if any(
+ param.name in KNOWN_CONTEXT_KEYS and param.default not in (None, inspect.Parameter.empty)
+ for param in signature.parameters.values()
+ ):
+ raise ValueError("Context key parameters can't have a default other than None")
+
parameters = [
param.replace(default=None) if param.name in KNOWN_CONTEXT_KEYS else param
for param in signature.parameters.values()
]
- signature = signature.replace(parameters=parameters)
+ try:
+ signature = signature.replace(parameters=parameters)
+ except ValueError:
+ raise ValueError(
+ "The function signature broke while assigning defaults to context key parameters. "
+ "You need to change their position in the parameter list"
Review Comment:
What about this message? Made it print the signatures of both the current and replaced signatures so that users can see what's going on, plus the Python-generated error message (in case there's another reason this may break in the future, although I can't think of one at the moment).
```
The function signature broke while assigning defaults to context key parameters.
The decorator is replacing the signature
> run_task(input: 'str', logical_date: 'str | None', logical_date_2: 'str', *args, **kwargs)
with
> run_task(input: 'str', logical_date: 'str | None' = None, logical_date_2: 'str', *args, **kwargs)
which isn't valid: non-default argument follows default argument
```
Hopefully this isn't too verbose!
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #38015:
URL: https://github.com/apache/airflow/pull/38015#discussion_r1521948279
##########
airflow/decorators/base.py:
##########
@@ -208,11 +208,35 @@ def __init__(
# since values for those will be provided when the task is run. Since
# we're not actually running the function, None is good enough here.
signature = inspect.signature(python_callable)
+
+ # Don't allow context argument defaults other than None to avoid ambiguities.
+ if any(
+ param.name in KNOWN_CONTEXT_KEYS and param.default not in (None, inspect.Parameter.empty)
+ for param in signature.parameters.values()
+ ):
+ raise ValueError("Context key parameters can't have a default other than None")
Review Comment:
Would it be helpful if this shows which argument is at fault? Or is it unnecessary?
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #38015:
URL: https://github.com/apache/airflow/pull/38015#issuecomment-2015977471
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "kevinalh (via GitHub)" <gi...@apache.org>.
kevinalh commented on code in PR #38015:
URL: https://github.com/apache/airflow/pull/38015#discussion_r1520710330
##########
airflow/decorators/base.py:
##########
@@ -208,11 +208,36 @@ def __init__(
# since values for those will be provided when the task is run. Since
# we're not actually running the function, None is good enough here.
signature = inspect.signature(python_callable)
+
+ # Don't allow context argument defaults other than None to avoid ambiguities.
+ if any(
+ param.name in KNOWN_CONTEXT_KEYS and param.default not in (None, inspect.Parameter.empty)
+ for param in signature.parameters.values()
+ ):
+ raise ValueError("Context key parameters can't have a default other than None")
+
parameters = [
param.replace(default=None) if param.name in KNOWN_CONTEXT_KEYS else param
for param in signature.parameters.values()
]
- signature = signature.replace(parameters=parameters)
+ try:
+ signature = signature.replace(parameters=parameters)
+ except ValueError as err:
+ raise ValueError(
+ textwrap.dedent(
+ f"""
+ The function signature broke while assigning defaults to context key parameters.
+
+ The decorator is replacing the signature
+ > {python_callable.__name__}({', '.join(str(param) for param in signature.parameters.values())})
+
+ with
+ > {python_callable.__name__}({', '.join(str(param) for param in parameters)})
+
+ which isn't valid: {err}
+ """
+ )
Review Comment:
Traceback looks much better with this change, thanks for the suggestion
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #38015:
URL: https://github.com/apache/airflow/pull/38015#discussion_r1521947442
##########
tests/decorators/test_python.py:
##########
@@ -259,6 +259,16 @@ def add_number(num: int) -> int:
add_number()
add_number("test")
+ def test_fails_context_parameter_other_than_none(self):
+ """Fail if a context parameter has a default and it's not None."""
+ with pytest.raises(ValueError):
Review Comment:
Should also check the exception value.
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38015:
URL: https://github.com/apache/airflow/pull/38015#discussion_r1518758684
##########
airflow/decorators/base.py:
##########
@@ -208,11 +208,25 @@ def __init__(
# since values for those will be provided when the task is run. Since
# we're not actually running the function, None is good enough here.
signature = inspect.signature(python_callable)
+
+ # Don't allow context argument defaults other than None to avoid ambiguities.
+ if any(
+ param.name in KNOWN_CONTEXT_KEYS and param.default not in (None, inspect.Parameter.empty)
+ for param in signature.parameters.values()
+ ):
+ raise ValueError("Context key parameters can't have a default other than None")
+
parameters = [
param.replace(default=None) if param.name in KNOWN_CONTEXT_KEYS else param
for param in signature.parameters.values()
]
- signature = signature.replace(parameters=parameters)
+ try:
+ signature = signature.replace(parameters=parameters)
+ except ValueError:
+ raise ValueError(
+ "The function signature broke while assigning defaults to context key parameters. "
+ "You need to change their position in the parameter list"
Review Comment:
I think we need a little more explicit message here. Maybe a wrong example and what the user is expected to do ?
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "kevinalh (via GitHub)" <gi...@apache.org>.
kevinalh commented on code in PR #38015:
URL: https://github.com/apache/airflow/pull/38015#discussion_r1518928827
##########
airflow/decorators/base.py:
##########
@@ -208,11 +208,25 @@ def __init__(
# since values for those will be provided when the task is run. Since
# we're not actually running the function, None is good enough here.
signature = inspect.signature(python_callable)
+
+ # Don't allow context argument defaults other than None to avoid ambiguities.
+ if any(
+ param.name in KNOWN_CONTEXT_KEYS and param.default not in (None, inspect.Parameter.empty)
+ for param in signature.parameters.values()
+ ):
+ raise ValueError("Context key parameters can't have a default other than None")
+
parameters = [
param.replace(default=None) if param.name in KNOWN_CONTEXT_KEYS else param
for param in signature.parameters.values()
]
- signature = signature.replace(parameters=parameters)
+ try:
+ signature = signature.replace(parameters=parameters)
+ except ValueError:
+ raise ValueError(
+ "The function signature broke while assigning defaults to context key parameters. "
+ "You need to change their position in the parameter list"
Review Comment:
What about this message? Made it print the signatures of both the current and replaced signatures so that users can see what's going on, plus the Python-generated error message (in case there's another reason this may break in the future, although I can't think of one at the moment). I think we can assume the Python-generated message is enough guidance.
```
The function signature broke while assigning defaults to context key parameters.
The decorator is replacing the signature
> run_task(input: 'str', logical_date: 'str | None', logical_date_2: 'str', *args, **kwargs)
with
> run_task(input: 'str', logical_date: 'str | None' = None, logical_date_2: 'str', *args, **kwargs)
which isn't valid: non-default argument follows default argument
```
Hopefully this isn't too verbose!
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #38015:
URL: https://github.com/apache/airflow/pull/38015#issuecomment-2041348474
Uh, oh, late to the party. I noticed this change which - after some reading - really looks understandable. But when testing 2.9.0RC2 I had a lot of broken DAGs and it felt to be like a breaking change.
Therefore I raised a PR to add a newsfragment such that we have less support issues and user compliants. Still I see it a bit problematic... in our cases the DAGs just broke because we have strong mypy/pylint checks and want to ensure we have typed code...
I our case we had a DAG and wanted to have the DAG params being typed. Imageine something like
```
with ... as dag:
@task
def get_file_list(params):
"""Prepares the file list to be re-computed based on the passed parameters."""
return params["files"]
@task(retries=2)
def recompute_batch(file) -> None:
... # do something with the file
recompute_batch.expand(file=get_file_list())
```
... so here the `get_file_list(params)` is not having types defined which is not nice...
Then changing this to `get_file_list(params: ParamsDict)` is typed - but to add the task in the DAG we need to call the function `get_file_list()` in the code for the expansion. Required argument then is missing.
So we had the option to use either `get_file_list(params: ParamsDict|None)` which then requires a check against `None` in the code in order to satisfy mypy.
That is why in many DAGs we use the signature like `def get_file_list(params: ParamsDict = dag.params) -> List[str]:`.
With this change being now on the main line. What is the recommended way for key context variables and parallel type checks and demand for typed context?
The full example we use typed today, broken in 2.9.0rc2
```
with ... as dag:
@task
def get_file_list(params: ParamsDict = dag.params) -> List[str]:
"""Prepares the file list to be re-computed based on the passed parameters."""
result: List[str] = params["files"]
return result
@task(retries=2)
def recompute_batch(file: str) -> None:
...
recompute_batch.expand(file=get_file_list())
```
--
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] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #38015:
URL: https://github.com/apache/airflow/pull/38015#issuecomment-2041517914
I think a little bit more context is needed on that one, becuase this is a follow-up and really fix of another problem introdued in 2.8 in https://github.com/apache/airflow/pull/33430 (where we allowed context params to not have defaults at all).
This change is really codifying what has been explained for quite some time - since 2.7.0. I think we explicitly explained that `| None` should be used for typing
From https://airflow.apache.org/docs/apache-airflow/2.8.0/core-concepts/taskflow.html#context :
```python
@task
def print_ti_info(task_instance: TaskInstance | None = None, dag_run: DagRun | None = None):
print(f"Run ID: {task_instance.run_id}") # Run ID: scheduled__2023-08-09T00:00:00+00:00
print(f"Duration: {task_instance.duration}") # Duration: 0.972019
print(f"DAG Run queued at: {dag_run.queued_at}") # 2023-08-10 00:00:01+02:20
```
This is how we explain that we expect None in those parameters used by the context. That's the "canonical" way I think we suggest people doing it since 2.7. In 2.8 https://github.com/apache/airflow/pull/33430 introduced the possibility of "NOT" having the defaults.
Technically speaking - `params = dag.params` is just a `mypy` hack, because the value set as default will be anyhow overwritten with the params from context. And it might be misleading because people my expect it to be set. And it's a hack that you can do for only subset of arguments - like params in a reasonable/easy way IMHO.
Say - for TaskInstance or DagRun - how would you provide defaults there? I think limiting only to params being set by dag.params as default is even more hack'y and confusing in this context.
There is a class of difficult to spot errors that is avoided with this fix - for example this will not work as you think it will work (if you don't know that run_id will be injected by task_flow):
```python
@task
def print_ti_info(some_value="x", run_id: str = "my_run_id"):
print(some_value)
print(run_id)
print_ti_info():
```
In example above you might simply miss that the context param has the `run_id` parameter injected, there are other name clashes possible too - especially for task names overriding the context names accidentally. There is quite some ambiguity today and this change is going to address it.
And there is another case - it's not `academic` problem either actually (See https://github.com/apache/airflow/issues/38006) the change was not really implemented "because we thought it is a good idea" - it is solving a real issue our user had - when https://github.com/apache/airflow/pull/33430 was added - for some of our users they could see "default value cannot follow non-default one) - introduced by https://github.com/apache/airflow/pull/33430 - where we actually allowed context values not to provide defaults at all. This was when user used a parameter (`logical_date`) that was also accidentally a context parameter. So it detected a mistake the user could miss before.
However yes - I agree that this change might have much bigger effect than we initially anticipated. Others **could** have used it for similar purpose to yours. We might want to make some other follow-ups after that if we see that this is a problem for others.
The case you have @jscheffl is really a hack, but one that is likely to be used. I don't think it is strong enough reason to make RC4 - don't think so, while I did not get approval from @uranusjr and @Taragolis - we all thought it's a reasonable one, I think (I'd love to hear your thought here as well).
But answering the typing question - one of the solutions (and a simple one) is:
```python
@task
def print_ti_info(params: ParamsDict | None = None, task_instance: TaskInstance | None = None, dag_run: DagRun | None = None):
assert params and task_instance and dag_run
...
```
A bit ugly, yes, but will work, and I personally think it's much more "straightforward" than smth like that:
```python
def print_ti_info(params: ParamsDict = dag.param, task_instance: TaskInstance = TaskInstance(), dag_run: DagRun = DagRun()):
...
```
Ideally (and maybe that should be a follow-up) we should be able to have (and mybe even publish) Airflow MyPy plugin - someone even requested one for default args #38796 recently https://github.com/apache/airflow/tree/main/dev/mypy/plugin (the ones we have work with default args and outputs). Maybe it is possible - we could write a plugin that could lilely be used to also allow this case (though @uranusjr in https://github.com/apache/airflow/pull/33430 hinted that it might be impossible).
```python
def print_ti_info(params: ParamsDict, task_instance: TaskInstance, dag_run: DagRun):
....
print_ti_info()
```
And maybe that's the best idea to actually do so for anyone who wants to use MyPy in this 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
Re: [PR] Don't allow defaults other than None in context parameters, and improve error message [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #38015:
URL: https://github.com/apache/airflow/pull/38015#issuecomment-2041524410
Thanks @potiuk for the detailled background. Convinced.
And believe me, I did not want to revert and call for an RC4 - if we can add the newsfragtment on the docs this woul dbe great for others potentially stumbling into it.
Of course I'll start adjusting our DAGs now...
--
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