Navigation Menu

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

Introduce subpackage airflow.compat #15969

Merged
merged 1 commit into from May 25, 2021
Merged

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented May 20, 2021

Spawned from #15397 (comment)

This introduces a shim subpackage airflow.compat for all code to import from, instead of ad-hoc try-except.

  • airflow.compat.functools shims functools.cached_property (using the third-party cached_property package) and 'functools.cache' (with functools.lru_cache).
  • airflow.compat.yaml was moved from airflow.utils.yaml and improved so we can always use it instead of importing yaml directly. Reverted

^ 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.

@boring-cyborg boring-cyborg bot added area:CLI area:core-operators Operators, Sensors and hooks within Core Airflow provider:cncf-kubernetes Kubernetes provider related issues area:logging area:providers area:secrets provider:amazon-aws AWS/Amazon - related issues provider:google Google (including GCP) related issues labels May 20, 2021
@potiuk
Copy link
Member

potiuk commented May 20, 2021

Would not that require releasing a new version of Airlfow and having all providers depend on that version? We are already going to release a new version of providers that are going to depend on Airlfow 2.1+, but as I see that will be another similar change and it will require another compatibility-breaking release of providers?

@uranusjr
Copy link
Member Author

Would not that require releasing a new version of Airlfow and having all providers depend on that version?

Err, right, I didn't consider that. I'll revert the airflow.compat.yaml one.

The functools ones don't require provider releases since the old imports will still work, the new Airflow version will only add a shortcut to do the same thing.

@uranusjr
Copy link
Member Author

I've put airflow.utils.yaml back.

@potiuk
Copy link
Member

potiuk commented May 20, 2021

The functools ones don't require provider releases since the old imports will still work, the new Airflow version will only add a shortcut to do the same thing.

Not really. It also means that when we release new providers (they are always released from master) they will not work with the old versions of airflow that have no airflow.compat package.

The only sane way of doing it is making those changes first (adding compat) to airflow and then at some point of time when releasing providers switching to it with >= <airflow_version_with_compat>. Which I think we should do.

But I'd reserve it for major versions of Airflow.

As soon as we release 2.1 version of airflow we will add-back the check where all providers are built and installed with old version of airlflow (we temporarily removed it before releasing 2.1 and adding "apply_default" incompatibility in the same way), but it will be 2.1 now) - and in this case you will get a failing job. This way we will prevent from accidental usage of this compat in providers (until we decide new wave of providers is >=2.2 for example).

@potiuk
Copy link
Member

potiuk commented May 20, 2021

I even think that (unlike apply_providers) the old imports in providers should stay until 3.0 . There is rather little value in this comparing to the lost backwards-compatibiity with already released (and used) versions of Airflow.

But this is perfectly ok to change it for all the "core" parts

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to sort out provider compatibility approach for that one.

@ashb
Copy link
Member

ashb commented May 20, 2021

Eek! Good spot Jarek.

@uranusjr
Copy link
Member Author

I've reverted all changes to airflow/providers.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label May 20, 2021
@github-actions
Copy link

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 master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@uranusjr uranusjr requested a review from ashb May 25, 2021 11:02
This module shims 'cached_property' and 'cache' so modules don't need to
all do their own ad-hoc try-except ImportError.
@ashb ashb merged commit 3db347e into apache:master May 25, 2021
@uranusjr uranusjr deleted the shim-cached-property branch May 28, 2021 06:54
jhtimmins pushed a commit that referenced this pull request Jul 8, 2021
This module shims 'cached_property' and 'cache' so modules don't need to
all do their own ad-hoc try-except ImportError.

(cherry picked from commit 3db347e)
jhtimmins pushed a commit to astronomer/airflow that referenced this pull request Jul 9, 2021
This module shims 'cached_property' and 'cache' so modules don't need to
all do their own ad-hoc try-except ImportError.

(cherry picked from commit 3db347e)
jhtimmins pushed a commit that referenced this pull request Jul 9, 2021
This module shims 'cached_property' and 'cache' so modules don't need to
all do their own ad-hoc try-except ImportError.

(cherry picked from commit 3db347e)
kaxil pushed a commit to astronomer/airflow that referenced this pull request Jul 13, 2021
This module shims 'cached_property' and 'cache' so modules don't need to
all do their own ad-hoc try-except ImportError.

(cherry picked from commit 3db347e)
(cherry picked from commit 1aa1bbd)
@ashb ashb added this to the Airflow 2.2.0 milestone Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI area:core-operators Operators, Sensors and hooks within Core Airflow area:logging area:providers area:secrets full tests needed We need to run full set of tests for this PR to merge provider:amazon-aws AWS/Amazon - related issues provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants