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

Implement per-provider tests with lowest-direct dependency resolution #39946

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented May 30, 2024

With this change we are running tests of downgrading Airflow
dependencies to "lowest-direct" ones - separately for "core" tests
and for each provider (and run corresponding tests with it). This
should allows us to determine what are the lowest bounds for the
dependencies - for Airflow and for individual providers and continue
doing it while Airflow evolves in the future.

Fixes: #35549

Related: #39100


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

@potiuk potiuk marked this pull request as draft May 30, 2024 11:58
@mobuchowski mobuchowski self-requested a review May 30, 2024 12:12
@potiuk potiuk force-pushed the add-lowest-dependency-resolution-tests branch from e68fbed to f6c5987 Compare May 30, 2024 13:01
@JDarDagran
Copy link
Contributor

JDarDagran commented May 30, 2024

Does this PR assume all cross-provider dependencies should be expressed as extras (in terms of python packaging, added in additional-extras for each provider)?
Not saying this is a bad approach, just making sure as e.g. OpenLineage provider exists as extra dep only for DBT provider and probably needs manual work to add it as well (or at least have a check in pre-commit if such extra exists, there's already check for affected providers).
lowest-direct resolution is a great approach though 👍

Edit: While the initial checks cover transitive dependencies, they don't account for potential issues that may arise when using different providers with different versions of OpenLineage. For instance, consider a scenario where:

  • In release A.X -> A.Y, a breaking change is introduced in the OpenLineage provider, which also affects the version of openlineage-python support.
  • In release B.X -> B.Y, provider B addresses this change by bumping the lowest version in the [openlineage] extra.
  • However, the proposed tests only verify that the lowest OpenLineage provider and highest openlineage-python version still work together with provider B.

This leaves a gap in testing, as it doesn't ensure that provider B version B.Y can seamlessly integrate with the older OpenLineage provider A.X, unless we assume that users would always install provider B with the [openlineage] extra.

@potiuk
Copy link
Member Author

potiuk commented May 30, 2024

Does this PR assume all cross-provider dependencies should be expressed as extras (in terms of python packaging, added in additional-extras for each provider)?

Unfortunately the way it works in Python, those "extra" limits are not enforceable, and It makes little sense to add them (and verify). There is really no good way to speciffy "if you want to install this optional dependency, it has to follow those limits" - there are multiple scenarios where even with those limits set on extra, user will be able to perform a sequence of upgrades to break it, and at any point in their process even pip check will show that everything is fine. So I am not sure we even should attempt to do such a check.

@potiuk potiuk force-pushed the add-lowest-dependency-resolution-tests branch from f6c5987 to 71fab0d Compare May 30, 2024 21:27
@potiuk potiuk added the default versions only When assigned to PR - only default python version is used for CI tests label May 30, 2024
@potiuk potiuk force-pushed the add-lowest-dependency-resolution-tests branch 2 times, most recently from ed99adc to 8eb947e Compare May 30, 2024 21:31
@potiuk potiuk added the use self-hosted runners When set on a PR run self-hosted runners will be used by default label May 30, 2024
@potiuk potiuk force-pushed the add-lowest-dependency-resolution-tests branch 4 times, most recently from f7e1e1c to 0bbe8b9 Compare May 31, 2024 06:34
@JDarDagran
Copy link
Contributor

Does this PR assume all cross-provider dependencies should be expressed as extras (in terms of python packaging, added in additional-extras for each provider)?

Unfortunately the way it works in Python, those "extra" limits are not enforceable, and It makes little sense to add them (and verify). There is really no good way to speciffy "if you want to install this optional dependency, it has to follow those limits" - there are multiple scenarios where even with those limits set on extra, user will be able to perform a sequence of upgrades to break it, and at any point in their process even pip check will show that everything is fine. So I am not sure we even should attempt to do such a check.

Fully agreed. Do you still think that would be worth adding different kind of tests too?
I'll stick to OpenLineage as there have been cases for not keeping backwards-compatibility we did not detect. #39530 also brings such and would love to have something that runs checks against this. My initial idea is to maintain a static list of providers that can be downgraded to the latest released version, similar to how Pydantic and SQLAlchemy have special tests for downgrading. This way, if changes are made to a provider that affect compatibility with OpenLineage, tests will fail if the changes are not backwards-compatible.

@potiuk
Copy link
Member Author

potiuk commented May 31, 2024

Sure - we can think of next steps and once we get this one merged, you could add a similar follow-up PR with those checks :)

@potiuk potiuk force-pushed the add-lowest-dependency-resolution-tests branch 4 times, most recently from 2bbb5be to 8ad1ea7 Compare June 1, 2024 08:46
@potiuk potiuk marked this pull request as ready for review June 1, 2024 08:47
@potiuk
Copy link
Member Author

potiuk commented Jun 1, 2024

cc: @charliermarsh -> thanks to uv's lowest-direct we finally could automate bumping the lower bounding of direct dependencies for Airflow and it's ~90 providers

Also @notatallshaw -> this is probably as close as it can get to the #39100 you opened and likely will make your apache-airflow[all] installations quite a bit faster. UPDATE: Actually you wanted to put limits for airflow -> provider dependencies, so it's a bit differen (those are limits on airlfow and provider's own dependencies) - but let's see where it can get us.

@potiuk potiuk requested a review from jscheffl June 1, 2024 08:54
@potiuk potiuk requested a review from mik-laj as a code owner June 3, 2024 04:47
@potiuk potiuk force-pushed the add-lowest-dependency-resolution-tests branch 3 times, most recently from d01e164 to 1f17772 Compare June 3, 2024 12:29
With this change we are running tests of downgrading Airflow
dependencies to "lowest-direct" ones - separately for "core" tests
and for each provider (and run corresponding tests with it). This
should allows us to determine what are the lowest bounds for the
dependencies - for Airflow and for individual providers and continue
doing it while Airflow evolves in the future.

Fixes: apache#35549
Related: apache#39100
@potiuk potiuk force-pushed the add-lowest-dependency-resolution-tests branch from 1f17772 to 986ca14 Compare June 3, 2024 13:02
@potiuk potiuk merged commit c0f2709 into apache:main Jun 4, 2024
107 checks passed
@potiuk potiuk deleted the add-lowest-dependency-resolution-tests branch June 4, 2024 04:20
@potiuk potiuk added this to the Airflow 2.9.2 milestone Jun 4, 2024
@potiuk
Copy link
Member Author

potiuk commented Jun 4, 2024

And merged :)

@potiuk
Copy link
Member Author

potiuk commented Jun 4, 2024

BTW. When it comes to --lowest strategy, we might want to wait to try it when astral-sh/uv#4006 gets merged - then we will see clearer which packages might cause problems there - but I am afraid it will get things "too much" to the floor, and we will be fixing someone else's problems this way.

@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jun 4, 2024
@ephraimbuddy ephraimbuddy modified the milestone: Airflow 2.9.2 Jun 4, 2024
fdemiane pushed a commit to fdemiane/airflow that referenced this pull request Jun 6, 2024
…apache#39946)

With this change we are running tests of downgrading Airflow
dependencies to "lowest-direct" ones - separately for "core" tests
and for each provider (and run corresponding tests with it). This
should allows us to determine what are the lowest bounds for the
dependencies - for Airflow and for individual providers and continue
doing it while Airflow evolves in the future.

Fixes: apache#35549
Related: apache#39100
syedahsn pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Jun 7, 2024
…apache#39946)

With this change we are running tests of downgrading Airflow
dependencies to "lowest-direct" ones - separately for "core" tests
and for each provider (and run corresponding tests with it). This
should allows us to determine what are the lowest bounds for the
dependencies - for Airflow and for individual providers and continue
doing it while Airflow evolves in the future.

Fixes: apache#35549
Related: apache#39100
potiuk added a commit to potiuk/airflow that referenced this pull request Jun 12, 2024
The output of parallel tests especially for lowest-direct tests
is very long because we are printing state every 10 seconds and there
are many paralell test types (90+ for lowest direct). We do not need
to print progress that often, and it has already been added in apache#39946
but one place to add it was missing - context manager still had
the default 10 seconds refresh time.

After this change the output will be printed every 20 seconds in
the regular tests and every 2 minutes in "lowest-direct" tests
(controlled by env variable) so the output should be much easier
to find reasons for issues.
potiuk added a commit that referenced this pull request Jun 12, 2024
The output of parallel tests especially for lowest-direct tests
is very long because we are printing state every 10 seconds and there
are many paralell test types (90+ for lowest direct). We do not need
to print progress that often, and it has already been added in #39946
but one place to add it was missing - context manager still had
the default 10 seconds refresh time.

After this change the output will be printed every 20 seconds in
the regular tests and every 2 minutes in "lowest-direct" tests
(controlled by env variable) so the output should be much easier
to find reasons for issues.
@simonprydden
Copy link
Contributor

But on a serious note - I really want to hear from Amazon team, because I think the watchtower change might be problematic (can't remember the root cause - so likely we want to fix it before we merge the change).

@potiuk did you hear back from the MWAA team?

The issue is that MWAA pins watchtower==2.0.1 , as per docs

Note
For Apache Airflow v2 and above, Amazon MWAA installs Watchtower version 2.0.1 after performing pip3 install -r requirements.txt, to ensure compatibility with CloudWatch logging is not overridden by other Python library installations.

Trying to find this PR in the amazon providers changelog, to double check which is the max version of the amazon providers that will work with MWAA. I think it's 8.24.0

@potiuk
Copy link
Member Author

potiuk commented Jul 18, 2024

@potiuk did you hear back from the MWAA team?

Yes. See #39946 (comment) and maybe @vincbeck or @ferruzzi or @o-nikolas can comment on that :)

@o-nikolas
Copy link
Contributor

@potiuk did you hear back from the MWAA team?

Yes. See #39946 (comment) and maybe @vincbeck or @ferruzzi or @o-nikolas can comment on that :)

I've asked around on the MWAA team and I'll report what I hear back!

romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
…apache#39946)

With this change we are running tests of downgrading Airflow
dependencies to "lowest-direct" ones - separately for "core" tests
and for each provider (and run corresponding tests with it). This
should allows us to determine what are the lowest bounds for the
dependencies - for Airflow and for individual providers and continue
doing it while Airflow evolves in the future.

Fixes: apache#35549
Related: apache#39100
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
The output of parallel tests especially for lowest-direct tests
is very long because we are printing state every 10 seconds and there
are many paralell test types (90+ for lowest direct). We do not need
to print progress that often, and it has already been added in apache#39946
but one place to add it was missing - context manager still had
the default 10 seconds refresh time.

After this change the output will be printed every 20 seconds in
the regular tests and every 2 minutes in "lowest-direct" tests
(controlled by env variable) so the output should be much easier
to find reasons for issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
all versions If set, the CI build will be forced to use all versions of Python/K8S/DBs area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) use self-hosted runners When set on a PR run self-hosted runners will be used by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce upgrades of min-versions for our dependencies