Skip to content

Comments

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

Merged
potiuk merged 1 commit intoapache:mainfrom
potiuk:future_annotations_2
Sep 14, 2022
Merged

Apply PEP-563 (Postponed Evaluation of Annotations) to core airflow#26290
potiuk merged 1 commit intoapache:mainfrom
potiuk:future_annotations_2

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Sep 10, 2022 •
edited
Loading

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.

@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Sep 10, 2022
@potiuk potiuk force-pushed the future_annotations_2 branch from 441986d to 69b907c Compare September 10, 2022 20:54
This was referenced Sep 13, 2022
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@potiuk potiuk mentioned this pull request Sep 13, 2022
Closed
@potiuk potiuk removed the area:API Airflow's REST/HTTP API label Sep 13, 2022
@potiuk potiuk force-pushed the future_annotations_2 branch from 69b907c to 9ed9125 Compare September 13, 2022 12:08
@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 mentioned this pull request Sep 13, 2022
Merged
@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 ?

@jedcunningham jedcunningham mentioned this pull request Sep 14, 2022
Closed
@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

uranusjr commented Sep 14, 2022

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

@potiuk
Apply PEP-563 (Postponed Evaluation of Annotations) to core airflow
8e8fc6f 
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 force-pushed the future_annotations_2 branch from 4a87013 to 8e8fc6f Compare September 14, 2022 06:54
@potiuk potiuk merged commit d67ac59 into apache:main Sep 14, 2022
@potiuk potiuk deleted the future_annotations_2 branch September 14, 2022 06:55
@uranusjr uranusjr mentioned this pull request Sep 14, 2022
Merged
potiuk added a commit that referenced this pull request Sep 14, 2022
@potiuk
Apply PEP-563 (Postponed Evaluation of Annotations) to core airflow (#…
14b025a 
…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
@potiuk @ephraimbuddy
Apply PEP-563 (Postponed Evaluation of Annotations) to core airflow (#…
e99a518 
…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)
@rajaths010494 rajaths010494 mentioned this pull request Sep 22, 2022
Merged
@pierrejeambrun pierrejeambrun mentioned this pull request Oct 4, 2022
Merged
3 tasks
@BasPH BasPH mentioned this pull request Nov 4, 2022
Merged
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Nov 16, 2022
@potiuk potiuk mentioned this pull request May 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashb ashb ashb approved these changes

@mik-laj mik-laj Awaiting requested review from mik-laj

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy ephraimbuddy is a code owner

@uranusjr uranusjr Awaiting requested review from uranusjr

@dstandish dstandish Awaiting requested review from dstandish

@josh-fell josh-fell Awaiting requested review from josh-fell

@kaxil kaxil Awaiting requested review from kaxil kaxil is a code owner

@jedcunningham jedcunningham Awaiting requested review from jedcunningham jedcunningham is a code owner

@turbaszek turbaszek Awaiting requested review from turbaszek

@jhtimmins jhtimmins Awaiting requested review from jhtimmins

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton

@bbovenzi bbovenzi Awaiting requested review from bbovenzi

@XD-DENG XD-DENG Awaiting requested review from XD-DENG

Assignees

No one assigned

Labels

future-annotations type:improvement Changelog: Improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@potiuk @uranusjr @ashb @ephraimbuddy