Update our approach for executor-bound dependencies #22573

potiuk · 2022-03-28T19:05:26Z

Kubernetes and Celery are both providers and part of the core.
The dependencies for both are added via "extras" which makes them
"soft" limits and in case of serious dependency bumps this might
end up with a mess (as we experienced with bumping min K8S
library version from 11.0.0 to 22.* (resulting in yanking 4
versions of cncf.kubernetes provider.

After this learning, we approach K8S and Celery dependencies a bit
differently than any other dependencies.

for Celery and K8S (and Dask but this is rather an afterhought)
we do not strip-off the dependencies from the extra (so for
example [cncf.kubernetes] extra will have dependencies on
both 'apache-airflow-providers-cncf-kubernetes' as well as
directly on kubernetes library
We add upper-bound limits for both Celery and Kubernetes to prevent
from accidental upgrades. Both Celery and Kubernetes Python library
follow SemVer, and they are crucial components of Airlfow so they
both squarely fit our "do not upper-bound" exceptions.
We also add a rule that whenever dependency upper-bound limit is
raised, we should also make sure that additional testing is done
and appropriate apache-airflow lower-bound limit is added for
the apache-airflow-providers-cncf-kubernetes and
apache-airflow-providers-celery providers.

As part of this change we also had to fix two issues:

the image was needlesly rebuilt during constraint generation as
we already have the image and we even warn that it should
be built before we run constraint generation
after this change, the currently released, unyanked cncf.kubernetes
provider cannot be installed with airflow, because it has
conflicting requirements for kubernetes library (provider has
<11 and airflow has > 22.7). Therefore during constraint
generation with PyPI providers we install providers from PyPI, we
explicitly install the yanked 3.1.2 version. This should be
removed after we release the next K8S provider version.

That should protect our users in all scenarios where they might
unknowingly attempt to upgrade Kubernetes or Celery to incompatible
version.

Related to: #22560, #21727

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

README.md

setup.py

airflow/providers/celery/provider.yaml

potiuk · 2022-03-28T20:03:59Z

Should be all fixed. I also updated the 2.1 compatibility tests to remove celery and K8S

setup.py

potiuk · 2022-03-29T11:57:45Z

scripts/ci/constraints/ci_generate_constraints.sh

@@ -28,6 +28,4 @@ shift

 build_images::prepare_ci_build

-build_images::rebuild_ci_image_if_needed_with_group


This is not needed (it just makes constraint generation longer - we already have the images pulled at this stage)

- name: > Wait for CI images ${{ needs.build-info.outputs.pythonVersions }}:${{ env.GITHUB_REGISTRY_PULL_IMAGE_TAG }} run: ./scripts/ci/images/ci_wait_for_and_verify_all_ci_images.sh

potiuk · 2022-03-29T13:06:37Z

All green!

.github/workflows/ci.yml

Kubernetes and Celery are both providers and part of the core. The dependencies for both are added via "extras" which makes them "soft" limits and in case of serious dependency bumps this might end up with a mess (as we experienced with bumping min K8S library version from 11.0.0 to 22.* (resulting in yanking 4 versions of `cncf.kubernetes` provider. After this learning, we approach K8S and Celery dependencies a bit differently than any other dependencies. * for Celery and K8S (and Dask but this is rather an afterhought) we do not strip-off the dependencies from the extra (so for example [cncf.kubernetes] extra will have dependencies on both 'apache-airflow-providers-cncf-kubernetes' as well as directly on kubernetes library * We add upper-bound limits for both Celery and Kubernetes to prevent from accidental upgrades. Both Celery and Kubernetes Python library follow SemVer, and they are crucial components of Airlfow so they both squarely fit our "do not upper-bound" exceptions. * We also add a rule that whenever dependency upper-bound limit is raised, we should also make sure that additional testing is done and appropriate `apache-airflow` lower-bound limit is added for the `apache-airflow-providers-cncf-kubernetes` and `apache-airflow-providers-celery` providers. As part of this change we also had to fix two issues: * the image was needlesly rebuilt during constraint generation as we already have the image and we even warn that it should be built before we run constraint generation * after this change, the currently released, unyanked cncf.kubernetes provider cannot be installed with airflow, because it has conflicting requirements for kubernetes library (provider has <11 and airflow has > 22.7). Therefore during constraint generation with PyPI providers we install providers from PyPI, we explicitly install the yanked 3.1.2 version. This should be removed after we release the next K8S provider version. That should protect our users in all scenarios where they might unknowingly attempt to upgrade Kubernetes or Celery to incompatible version. Related to: apache#22560, apache#21727

jedcunningham

Few nits, otherwise looks good!

jedcunningham · 2022-03-29T17:55:23Z

setup.py


    In the constraint mechanism we save both - provider versions and it's dependencies
    version, which means that installation using constraints is repeatable.

+    For K8s, Celery which are both "Core executors" and "Providers" we have to


Suggested change

For K8s, Celery which are both "Core executors" and "Providers" we have to

For K8s and Celery which are both "Core executors" and "Providers" we have to

jedcunningham · 2022-03-29T17:55:48Z

setup.py


    In the constraint mechanism we save both - provider versions and it's dependencies
    version, which means that installation using constraints is repeatable.

+    For K8s, Celery which are both "Core executors" and "Providers" we have to
+    add the base dependencies to the core as well - in order to mitigate problems where


Suggested change

add the base dependencies to the core as well - in order to mitigate problems where

add the base dependencies to core as well, in order to mitigate problems where

jedcunningham · 2022-03-29T17:55:58Z

setup.py

+    newer version of provider will have less strict limits. This should be done for both
+    extras and their deprecated aliases. This is not a full protection however, the way
+    extras work, this will not add "hard" limits for Airflow and the user who does not use
+    constraints


Suggested change

constraints

constraints.

github-actions · 2022-03-29T18:23:43Z

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

These suggestions were missed in apache#22573 - https://github.com/apache/airflow/pull/22573/files#r837754616 - https://github.com/apache/airflow/pull/22573/files#r837755144 - https://github.com/apache/airflow/pull/22573/files#r837755355

These suggestions were missed in #22573 - https://github.com/apache/airflow/pull/22573/files#r837754616 - https://github.com/apache/airflow/pull/22573/files#r837755144 - https://github.com/apache/airflow/pull/22573/files#r837755355

These suggestions were missed in apache/airflow#22573 - https://github.com/apache/airflow/pull/22573/files#r837754616 - https://github.com/apache/airflow/pull/22573/files#r837755144 - https://github.com/apache/airflow/pull/22573/files#r837755355 GitOrigin-RevId: b0b69f3ea7186e76a04b733022b437b57a087a2e

potiuk requested a review from jedcunningham as a code owner March 28, 2022 19:05

boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:providers labels Mar 28, 2022

potiuk requested review from dstandish, ashb, ephraimbuddy and kaxil March 28, 2022 19:05

jedcunningham reviewed Mar 28, 2022

View reviewed changes

potiuk force-pushed the update-dependencies-approach branch from 852f6c9 to cc98ee9 Compare March 28, 2022 20:03

jedcunningham reviewed Mar 28, 2022

View reviewed changes

setup.py Outdated Show resolved Hide resolved

potiuk force-pushed the update-dependencies-approach branch from cc98ee9 to 2d838c8 Compare March 29, 2022 09:40

potiuk requested a review from mik-laj as a code owner March 29, 2022 09:40

potiuk force-pushed the update-dependencies-approach branch 2 times, most recently from cafbb72 to e05861f Compare March 29, 2022 11:55

potiuk commented Mar 29, 2022

View reviewed changes

potiuk force-pushed the update-dependencies-approach branch from e05861f to ad6a527 Compare March 29, 2022 12:17

potiuk commented Mar 29, 2022

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

potiuk force-pushed the update-dependencies-approach branch 4 times, most recently from 69b6081 to ec77fc2 Compare March 29, 2022 14:50

potiuk force-pushed the update-dependencies-approach branch from ec77fc2 to 51e0a2d Compare March 29, 2022 14:51

jedcunningham approved these changes Mar 29, 2022

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 29, 2022

potiuk merged commit 6db30f3 into apache:main Mar 29, 2022

potiuk deleted the update-dependencies-approach branch March 29, 2022 18:26

kaxil mentioned this pull request Mar 29, 2022

Doc: Update description for executor-bound dependencies #22601

Merged

potiuk mentioned this pull request Apr 7, 2022

Status of testing Providers that were prepared on April 07, 2022 #22836

Closed

46 tasks

ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Apr 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update our approach for executor-bound dependencies #22573

Update our approach for executor-bound dependencies #22573

potiuk commented Mar 28, 2022 •

edited

Loading

potiuk commented Mar 28, 2022

potiuk Mar 29, 2022 •

edited

Loading

potiuk commented Mar 29, 2022

jedcunningham left a comment

jedcunningham Mar 29, 2022

jedcunningham Mar 29, 2022

jedcunningham Mar 29, 2022

github-actions bot commented Mar 29, 2022

		@@ -28,6 +28,4 @@ shift

		build_images::prepare_ci_build

		build_images::rebuild_ci_image_if_needed_with_group

	For K8s, Celery which are both "Core executors" and "Providers" we have to
	For K8s and Celery which are both "Core executors" and "Providers" we have to

	add the base dependencies to the core as well - in order to mitigate problems where
	add the base dependencies to core as well, in order to mitigate problems where

Update our approach for executor-bound dependencies #22573

Update our approach for executor-bound dependencies #22573

Conversation

potiuk commented Mar 28, 2022 • edited Loading

potiuk commented Mar 28, 2022

potiuk Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

potiuk commented Mar 29, 2022

jedcunningham left a comment

Choose a reason for hiding this comment

jedcunningham Mar 29, 2022

Choose a reason for hiding this comment

jedcunningham Mar 29, 2022

Choose a reason for hiding this comment

jedcunningham Mar 29, 2022

Choose a reason for hiding this comment

github-actions bot commented Mar 29, 2022

potiuk commented Mar 28, 2022 •

edited

Loading

potiuk Mar 29, 2022 •

edited

Loading