-
Notifications
You must be signed in to change notification settings - Fork 16.3k
fix mypy error in airflow-core/src/airflow/models/trigger.py #58753
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.
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
Conversation
|
|
||
| # Get the next kwargs of the task instance, or an empty dictionary if it doesn't exist | ||
| next_kwargs = task_instance.next_kwargs or {} | ||
| next_kwargs = cast("dict[str, Any]", task_instance.next_kwargs or {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the error here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there’s a potential bug; task_instance.next_kwargs can be a str; see discussion here #58728 (comment)
This shouldn’t be ignored with a cast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanations above!
I just want to double-check the current situation:
From the discussion, it seems that task_instance.next_kwargs can be either a dict or a serialized/encrypted str, and the correct handling is still unclear.
Given this, should I put this PR aside for now and wait for a decision on how next_kwargs is expected to behave?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a similar case where next_kwargs is deserialized first if it's a str.
airflow/airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py
Lines 513 to 519 in 3928c9d
| # This is slightly inefficient as we deserialize it to then right again serialize it in the sqla | |
| # TypeAdapter. | |
| next_kwargs = None | |
| if ti_patch_payload.next_kwargs: | |
| from airflow.serialization.serialized_objects import BaseSerialization | |
| next_kwargs = BaseSerialization.deserialize(ti_patch_payload.next_kwargs) |
Perhaps the solution would be something like this?
| next_kwargs = cast("dict[str, Any]", task_instance.next_kwargs or {}) | |
| next_kwargs = task_instance.next_kwargs or {} | |
| if isinstance(next_kwargs, str): | |
| from airflow.serialization.serialized_objects import BaseSerialization | |
| next_kwargs = BaseSerialization.deserialize(next_kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See some additional suggestions in this discussion (incl. the collapsed session in my last comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
No problem, thanks for the clarification! |
|
All mypy errors have been fixed. Closing it |

related #56735
fix mypy error in airflow-core/src/airflow/models/trigger.py
^ 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.