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

Add get_airflow_version helper #44607

Closed

Conversation

dstandish
Copy link
Contributor

@dstandish dstandish commented Dec 3, 2024

The point of this helper is to make it simpler to figure out what airflow version is installed, in particular to implement conditional logic in providers.

Currently the logic is too complicated and this has resulted in the proliferation of sort of redundant constants. Here's an example:

from airflow import __version__ as AIRFLOW_VERSION

AIRFLOW_V_3_0_PLUS = Version(Version(AIRFLOW_VERSION).base_version) >= Version("3.0.0")
if AIRFLOW_V_3_0_PLUS:
    from airflow.sdk.definitions.asset import Asset

With this helper, we can do this instead:

from airflow import get_airflow_version

if get_airflow_version() > Version("3.0.0"):
    from airflow.sdk.definitions.asset import Asset

We can't make use of this helper in providers until they are bumped to min airflow version == 2.11.0.

@dstandish dstandish requested a review from potiuk December 3, 2024 13:46
@dstandish dstandish added this to the Airflow 2.11.0 milestone Dec 3, 2024
from airflow import configuration, settings


Copy link
Member

Choose a reason for hiding this comment

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

I think - as discussed in slack - this is a nice proposal. that we might be able to use in some 8 months (if we back-port it and release in 2.11, but we need to figure out - as part of it - what we do now and how we do it for the rest of the code.

I proposed a solution there (this needs to be fixed) - #43773 (comment) - not sure if we can figure out something better. I think it's a good idea to discuss it there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so the only real material problem with the constants is if someone imports one of these constants across providers. otherwise, it's mostly a non-issue. in any case this will be a generally helpful helper func.

if we want to reduce the likelihood of cross provider imports of these kinds of version constants, we could remove the version_references module from the standard provider. i've done that in latest commit. take a look and lemme know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, after thinking a bit and seeing it - I think we can have cale And wat it too. Maybe we can generate the constants via __init__.py template in all providers?

This way each procider could have automatically generated own set of constant (possibly even named differently to avoid confusion where to importit from.

I am at family funeral today with my mum and two aunt's but I can propose a PR when I am back if that seems appealing

Copy link
Member

Choose a reason for hiding this comment

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

See #44686

@potiuk
Copy link
Member

potiuk commented Dec 5, 2024

This should be not needed any more (at least for providers) after the better and more complete fix ("eat cake and have it too") in #44686

@dstandish dstandish force-pushed the add-get-airflow-version-helper branch from bdd3e60 to d0d0e31 Compare December 6, 2024 19:18
The point of this helper is to make it simpler to figure out what airflow version is installed, in particular to implement conditional logic in providers.

Currently the logic is too complicated and this has resulted in the proliferation of sort of redundant constants.  Here's an example:

```
from airflow import __version__ as AIRFLOW_VERSION

AIRFLOW_V_3_0_PLUS = Version(Version(AIRFLOW_VERSION).base_version) >= Version("3.0.0")
if AIRFLOW_V_3_0_PLUS:
    from airflow.sdk.definitions.asset import Asset
```

With this helper, we can do this instead:

```
from airflow import get_airflow_version

if get_airflow_version() > Version("3.0.0"):
    from airflow.sdk.definitions.asset import Asset
```
@dstandish dstandish force-pushed the add-get-airflow-version-helper branch from d0d0e31 to 96e5fe8 Compare December 6, 2024 19:18
from airflow import configuration, settings


def get_airflow_version() -> Version:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get_airflow_version() -> Version:
def get_base_airflow_version() -> Version:


def get_airflow_version() -> Version:
"""Return packaging Version object representing the base version."""
return Version(Version(__version__).base_version)
Copy link
Member

Choose a reason for hiding this comment

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

Can we make sure (add pre-commit?) that this method is not used until min-airflow-version is set to 2.11 in all providers?

Copy link
Member

@potiuk potiuk Dec 7, 2024

Choose a reason for hiding this comment

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

Likely it could just be a RUFF https://docs.astral.sh/ruff/settings/#lint_flake8-import-conventions_banned-from if instead adding it to airflow/__init__.py you will move it to airflow/version_compat.py module for example (following what I am proposing in #44686 as a solution until 2.11 is set as minimum version.

Implementing identical module will make it as simple as removing all version_compat.py from providers and replacing local provider's imports with imports from airflow.version_compat.

potiuk added a commit to potiuk/airflow that referenced this pull request Dec 7, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 7, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 7, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 7, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 7, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 7, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 7, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 7, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 8, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 8, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 8, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 8, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit to potiuk/airflow that referenced this pull request Dec 8, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
potiuk added a commit that referenced this pull request Dec 10, 2024
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via #44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jan 29, 2025
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
This PR introduces consistent way of checking version of Airflow
by Airflow providers. So far there were about 6 different ways on
how Providers checked for Airflow version - this PR aims to unify
this approach for now and in the future - at least until minimum
version of Airflow set to 2.11 where we are likely to introduce
a simpler check via apache#44607. Until then all providers are going
to have `version_references.py` module copied in their sources
that they will be importing the constants from.

This PR also adds pre-commit that checks if the
``version_compat.py`` module is imported from local package copy
or maybe from another provider or test code - both causing
unneeded dependencies from the provider - to another package or
to test code respectively.
@github-actions github-actions bot closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale PRs per the .github/workflows/stale.yml policy file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants