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

fix(proivders/common/compat): move airflow versions out of __init__.py #44610

Closed

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Dec 3, 2024

Why

#43773 (comment)

What

compare airflow versions in each modules


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

@@ -0,0 +1,26 @@
# Licensed to the Apache Software Foundation (ASF) under one
Copy link
Member

@potiuk potiuk Dec 3, 2024

Choose a reason for hiding this comment

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

This is, unfortunately, not a complete solution.

If we want to move those thing to "versions" in common.compat we also need to import those in all providers that use it, and add provider.yaml dependency to this new (not yet released) feature of common.compat.

and after discussion with @dstandish - I am not sure we want to do it. I think we should agree what to do and how to approach it consistently (and also have it described somewhere - how we are doing it.

Daniel in slack discussion raised concerns about dependencies introduced this way - for example this one introduced dependency on specific common-compat version in providers using it (which BTW. should be added in provider.yaml if we want to depend on this common.compat package.)

And yes I agree having just import of the constant requiring to have "common.compat" dependency is excessive.

So I think it would be great if we agree @dstandish @uranusjr @Lee-W @eladkal - from people involved in the discussiosn what we do - before we do it.

Because it has some questions/ answers:

  • tests_common has currently the same named constants and it already caused confusion - worth renaming those ?
  • we miss common.compat dependency to the upcoming version of common.compat - each provider using those constants has to have common.compat >= NEW_VERSION ?
  • are we going to have 2.11 as well in the list of constants below for the future?
  • maybe it's better to copy*paste the - currently DRY - code to all providers rather than have this common code at all?

Those were all the questions that need some answers and consensus, so that we are all on the same page and can guide contributors and commiters how to add this compatiblity code.

Copy link
Member

Choose a reason for hiding this comment

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

Also see #44607 as potential long-term solution (some 8 months from now).

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah i think ultimately these constants to more harm than good.

i think we should just chop em. it's more readable to just do something like this example

        if _get_airflow_version() >= Version("2.11.0"):
            timeout = self.timeout
        else:
            timeout = timedelta(seconds=self.timeout)

it's simple, it's explicit, it does not require any constants to be defined.

wherever we have constants like this defined, we can add a single private helper function

def _get_airflow_version():
    from airflow import __version__ as airflow_version

    return Version(Version(airflow_version).base_version)

then when the min airflow version of the provider reaches 2.11, we can use the helper from core introduced in #44607

wdyt

Copy link
Contributor

@dstandish dstandish Dec 3, 2024

Choose a reason for hiding this comment

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

and to clarify, i think it's fine to have these defined in test utils --- that i think doesn't really matter because there's no cross-provider dep issue. but for non-test code i think we do not add such constants, except possibly locally / privately for convenience -- in which case doesn't really bother me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. Yep, sounds good to me! I thought we needed a hotfix for it. I'll change it later today!

Copy link
Member Author

Choose a reason for hiding this comment

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

I just tried to change it to a similar approach. we probably will need it before min airflow version set to 2.11

Copy link
Member

Choose a reason for hiding this comment

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

I will come up with alternative proposal soon :)

@Lee-W Lee-W marked this pull request as draft December 4, 2024 02:15
@Lee-W Lee-W force-pushed the move-verions-out-of-compat-init branch from 14984e8 to fc0caf8 Compare December 4, 2024 03:52
@Lee-W Lee-W requested review from dstandish and potiuk December 4, 2024 03:52
@Lee-W Lee-W marked this pull request as ready for review December 4, 2024 03:52
@Lee-W Lee-W changed the title fix(porivders/common/compat): move airflow verions out of __init__.py fix(proivders/common/compat): move airflow versions out of __init__.py Dec 4, 2024
@potiuk
Copy link
Member

potiuk commented Dec 5, 2024

Better and more complete ("eat cake and have it too") in #44686

@Lee-W Lee-W closed this Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants