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.

@potiuk
Copy link
Member

@potiuk 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

@uranusjr uranusjr commented May 20, 2021

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 uranusjr force-pushed the shim-cached-property branch from b6eb9f1 to e3cd453 May 20, 2021
@uranusjr
Copy link
Member Author

@uranusjr uranusjr commented May 20, 2021

I've put airflow.utils.yaml back.

@potiuk
Copy link
Member

@potiuk 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 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

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

@ashb
Copy link
Member

@ashb ashb commented May 20, 2021

Eek! Good spot Jarek.

airflow/configuration.py Show resolved Hide resolved
@uranusjr uranusjr force-pushed the shim-cached-property branch from e3cd453 to 1a5114f May 20, 2021
@uranusjr
Copy link
Member Author

@uranusjr uranusjr commented May 20, 2021

I've reverted all changes to airflow/providers.

potiuk
potiuk approved these changes May 20, 2021
@github-actions
Copy link

@github-actions github-actions bot commented May 20, 2021

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
This module shims 'cached_property' and 'cache' so modules don't need to
all do their own ad-hoc try-except ImportError.
@uranusjr uranusjr force-pushed the shim-cached-property branch from 1a5114f to 814484e May 25, 2021
ashb
ashb approved these changes May 25, 2021
@ashb ashb merged commit 3db347e into apache:master May 25, 2021
39 of 41 checks passed
@uranusjr uranusjr deleted the shim-cached-property branch May 28, 2021
andormarkus pushed a commit to andormarkus/airflow that referenced this issue Jun 5, 2021
This module shims 'cached_property' and 'cache' so modules don't need to
all do their own ad-hoc try-except ImportError.
jhtimmins added a commit to astronomer/airflow that referenced this issue 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 added a commit that referenced this issue 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 added a commit to astronomer/airflow that referenced this issue 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 added a commit that referenced this issue 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 added a commit to astronomer/airflow that referenced this issue 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