-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Enhance multiple_outputs
inference of dict typing
#19608
Enhance multiple_outputs
inference of dict typing
#19608
Conversation
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
656e595
to
fdaf89a
Compare
fdaf89a
to
b92f49a
Compare
341740c
to
3cc024d
Compare
3cc024d
to
a7a6df1
Compare
@uranusjr I noticed in another PR that there were other references to tuples and lists in |
Looks like a new static check was added in #20501 which checks syntax for 3.7+ and is attempting to remove the check for Python 3.6 in the typing inference logic here. Is dropping Python 3.6 support slated for 2.2.4? Or should I go ahead and remove the Python 3.6 check and assume it's no longer supported even though #20467 is not part of a release yet? |
Interesting one that we have not foreseen. I think we should disable the pyupgrade for now and re-enable it when we release 2.3.0. |
@potiuk Would you like me to open a PR to disable the pyupgrade check or were there other conversations on handling this edge case until the 2.3 release? |
@task_decorator | ||
def identity_dict(x: int, y: int) -> eval(test_return_annotation): | ||
return {"x": x, "y": y} |
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.
Would it be possible/easy to also handle the stringified annotation, e.g.
@task_decorator
def identity_dict(x: int, y: int) -> "dict":
return {}
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.
Yeah I don't see why not. I'll dig in.
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 typing.get_type_hints()
does the eval automatically and can be useful here (to replace inspect.signature
).
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.
From what I can tell we eventually get into the same __origin__
vs __extra__
attribute situation for Python 3.6 unfortunately. We can definitely add some tests for the stringified dict typing though.
Also looks like the recent refactor of airflow/decorators/base.py
uses inspect.signature
a little more extensively too.
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.
Python 3.6
Haven’t we dropped it already?
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.
Technically it won't be dropped until 2.3 but this PR is slated for 2.2.4.
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.
Would it be viable to do de-stringification if 3.6 is not considered? We can add this in a separate PR that targets only 2.3+ if so.
Disabling - no but changing it back to 3.6 syntax (you can do it in this PR) with a note that it should be bumed to 3.7 when we release 2.3.0 |
0b036ea
to
d7378a9
Compare
(cherry picked from commit 4198550)
Closes: #19538
There are a few cases in which the existing
multiple_outputs
inference in the TaskFlow API doesn't automatically unfurl a returned dictionary into separateXComs
:dict
typemultiple_outputs
to True due to use of an incorrect attrThis PR fixes the inference on a simple
dict
return type annotation and adds a condition for Python 3.6 use.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.