Skip to content
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

Apply PEP-563 (Postponed Evaluation of Annotations) to core airflow #26290

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Sep 10, 2022

This PR applies PEP-563 to all core airflow Python files - i.e.
those that we want to cherry-pick to v2-* branches in order
to make cherry-picking for the upcoming 2.4* releases easier. There was
a separate PR (#26289) with non-core changes that are not going to be
cherry-picked..

This PR is a result of combining some of the 47 PRs reviewed
and approved separately (otherwise it would have been unreviewable)

The history of those PRs can be changed in:
https://github.com/apache/airflow/pulls?q=is%3Apr+label%3Afuture-annotations+is%3Aopen

Relevant discussion: https://lists.apache.org/thread/81fr042s5d3v17v83bpo24tnrr2pp0fp
Lazy consensus call: https://lists.apache.org/thread/l74nvjh8tgbtojllhwkcn7f8mfnlz4jq


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk potiuk removed the area:API Airflow's REST/HTTP API label Sep 13, 2022
@potiuk potiuk changed the title Convert airflow/api to __future__.annotations Apply PEP-563 (Postponed Evaluation of Annotations) to core airflow Sep 13, 2022
@potiuk potiuk force-pushed the future_annotations_2 branch 3 times, most recently from e1f2ee7 to 4a87013 Compare September 13, 2022 22:02
@potiuk
Copy link
Member Author

potiuk commented Sep 13, 2022

Hey @ashb @uranusjr (also @kaxil - I know you were looking at cattrs/atts before) -> I think, before merging that one I need one more opinion/comment/advice. I have not merged it yet as I was trying to figure out why we had exactly ONE test failing after I merged all the PRs.

I had this one test failing after I merged all the PEP-563 changes (strangely enough it was not failing in the PR where "baseoperator" changes were separated into a separate PR).

Example failure here: https://github.com/apache/airflow/actions/runs/3047099374/jobs/4911094396#step:9:8804

airflow.exceptions.SerializationError: Failed to serialize DAG 'test-bigquery-operators': 'type' object is not subscriptable

I did some bisecting and pin-pointed the exact line which is the culprit:

https://github.com/apache/airflow/pull/26290/files#diff-848f325ace55b3504e8052fecdb53c0f295c891b67a6d90e9341cbe79cc545fbL1748

It is super-reproducible with :

 pytest  tests/serialization/test_dag_serialization.py::TestStringifiedDAGs::test_extra_serialized_field_and_operator_links

It's as triggered by replacing those lines:

@attr.s(auto_attribs=True)
class BaseOperatorLink(metaclass=ABCMeta):
    """Abstract base class that defines how we get an operator link."""

    operators: ClassVar[List[Type[BaseOperator]]] = []

with those:

@attr.s(auto_attribs=True)
class BaseOperatorLink(metaclass=ABCMeta):
    """Abstract base class that defines how we get an operator link."""

    operators: ClassVar[list[type[BaseOperator]]] = []

This is what pyupgrade does. All other changes in "baseoperator" are fine (and I left them in in the current version).

Basically seems that changing from the "classic" typing to PEP-563 breaks some of the "attrs" magic in this SINGLE case.
I looked for similar issues and it seems very similar to python-attrs/cattrs#80, but it's not the same. And it does look like attrs is not able to handle the type correctly, when it is defined with PEP-563.

For now I just reverted that single line (and List/Type) from PyUpgrade and added the baseoperator to be skipped from pyugprade. The test is passing locally.

But I wonder If this is something that you might have a better idea how to solve ?

@potiuk
Copy link
Member Author

potiuk commented Sep 14, 2022

BTW. Tests are passing. I will merge that one (with better commend in pyupgrade disable) and I think we can deal with this afterwards.

@uranusjr
Copy link
Member

The error looks like python-attrs/cattrs#279. Translation: It does not work, no way around it.

This PR applies PEP-563 to all core airflow Python files - i.e.
those that we want to cherry-pick to v2-* branches in order
to make cherry-picking for the upcoming 2.4* releases easier. There was
a separate PR (apache#26289) with non-core changes that are not going to be
cherry-picked..

This PR is a result of combining some of the 47 PRs reviewed
and approved separately (otherwise it would have been unreviewable)

The history of those PRs can be changed in:
https://github.com/apache/airflow/pulls?q=is%3Apr+label%3Afuture-annotations+is%3Aopen

Relevant discussion: https://lists.apache.org/thread/81fr042s5d3v17v83bpo24tnrr2pp0fp
Lazy consensus call: https://lists.apache.org/thread/l74nvjh8tgbtojllhwkcn7f8mfnlz4jq
@potiuk potiuk merged commit d67ac59 into apache:main Sep 14, 2022
@potiuk potiuk deleted the future_annotations_2 branch September 14, 2022 06:55
potiuk added a commit that referenced this pull request Sep 14, 2022
…26290)

This PR applies PEP-563 to all core airflow Python files - i.e.
those that we want to cherry-pick to v2-* branches in order
to make cherry-picking for the upcoming 2.4* releases easier. There was
a separate PR (#26289) with non-core changes that are not going to be
cherry-picked..

This PR is a result of combining some of the 47 PRs reviewed
and approved separately (otherwise it would have been unreviewable)

The history of those PRs can be changed in:
https://github.com/apache/airflow/pulls?q=is%3Apr+label%3Afuture-annotations+is%3Aopen

Relevant discussion: https://lists.apache.org/thread/81fr042s5d3v17v83bpo24tnrr2pp0fp
Lazy consensus call: https://lists.apache.org/thread/l74nvjh8tgbtojllhwkcn7f8mfnlz4jq

(cherry picked from commit d67ac59)
ephraimbuddy pushed a commit that referenced this pull request Sep 14, 2022
…26290)

This PR applies PEP-563 to all core airflow Python files - i.e.
those that we want to cherry-pick to v2-* branches in order
to make cherry-picking for the upcoming 2.4* releases easier. There was
a separate PR (#26289) with non-core changes that are not going to be
cherry-picked..

This PR is a result of combining some of the 47 PRs reviewed
and approved separately (otherwise it would have been unreviewable)

The history of those PRs can be changed in:
https://github.com/apache/airflow/pulls?q=is%3Apr+label%3Afuture-annotations+is%3Aopen

Relevant discussion: https://lists.apache.org/thread/81fr042s5d3v17v83bpo24tnrr2pp0fp
Lazy consensus call: https://lists.apache.org/thread/l74nvjh8tgbtojllhwkcn7f8mfnlz4jq

(cherry picked from commit d67ac59)
@pierrejeambrun pierrejeambrun mentioned this pull request Oct 4, 2022
3 tasks
@BasPH BasPH mentioned this pull request Nov 4, 2022
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants