Removes pip download when installing from local packages #13422

potiuk · 2021-01-01T07:32:46Z

This PR improves building production image from local packages,
in preparation for moving provider requirements out of setup.cfg.

Previously pip download step was executed in the CI scripts
in order to download all the packages that were needed. However
this had two problems:

PIP download was executed outside of Dockerfile in CI scripts
which means that any change to requirements there could not
be executed in 'workflow_run' event - because main branch version
of CI scripts is used there. We want to add extra requirements
when installing airflow so in order to be able to change
it, those requirements should be added in Dockerfile.
This will be done in the follow-up Removes provider-imposed requirements from setup.cfg #13409 PR.
Packages downloaded with PIP download have a "file" version
rather than regular == version when you run pip freeze/check.
This looks weird and while you can figure out the version
from file name, when you pip install them, they look
much more normal. The airflow package and provider package
will still get the "file" form but this is ok because we are
building those packages from sources and they are not yet
available in PyPI.

Example:

adal==1.2.5
aiohttp==3.7.3
alembic==1.4.3
amqp==2.6.1
apache-airflow @ file:///docker-context-files/apache_airflow-2.1.0.dev0-py3-none-any.whl
apache-airflow-providers-amazon @ file:///docker-context-files/apache_airflow_providers_amazon-1.0.0-py3-none-any.whl
apache-airflow-providers-celery @ file:///docker-context-files/apache_airflow_providers_celery-1.0.0-py3-none-any.whl
...

With this PR, we do not pip download all packages, but instead
we prepare airflow + providers packages as .whl files and
install them from there (all the dependencies are installed
from PyPI)

^ 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 · 2021-01-01T07:33:31Z

I will need to merge this one before making #13409 works

github-actions · 2021-01-01T07:58:55Z

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

github-actions · 2021-01-01T08:32:23Z

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

github-actions · 2021-01-01T13:16:05Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

potiuk · 2021-01-01T13:24:32Z

Dockerfile

 AIRFLOW_EXTRAS=${AIRFLOW_EXTRAS/mysql,}; \
 fi; \
 if [[ ${INSTALL_FROM_PYPI} == "true" ]]; then \
 if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \
 pip install --user "${AIRFLOW_INSTALLATION_METHOD}[${AIRFLOW_EXTRAS}]${AIRFLOW_INSTALL_VERSION}" \
 --upgrade --upgrade-strategy eager; \
+ pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \


Here we resore PIP to the version specified (eager upgrade will upgrade it)

potiuk · 2021-01-01T13:26:14Z

Dockerfile

 else \
- pip install --user "${AIRFLOW_INSTALLATION_METHOD}[${AIRFLOW_EXTRAS}]${AIRFLOW_INSTALL_VERSION}" \
+ pip install --upgrade --upgrade-strategy only-if-needed \


Now - we add "only-if needed " strategy here in case someone changes setup.py requirements and the pre-installed versions no longer fit in those requirements.

potiuk · 2021-01-01T13:27:42Z

Dockerfile

+ fi; \
+ fi; \
+ if [[ ${INSTALL_FROM_DOCKER_CONTEXT_FILES} == "true" ]]; then \
+ reinstalling_apache_airflow_packages=$(ls /docker-context-files/apache?airflow*.{whl,tar.gz} 2>/dev/null || true); \


This is an interesting one. Instead of pip download as we did before, we want to install all dependencies here for airflow packages and follow the constraints we already have (unless we upgrade to newer dependencies -then we do not use constraints).

This is the case where we haave only "airflow" + "airflow packages" build from sources to install (which happens on CI).

potiuk · 2021-01-01T13:30:12Z

Dockerfile

+ fi; \
+ fi ; \
+ # All the others we want to reinstall as-is, without dependencies \
+ reinstalling_other_packages=$(ls /docker-context-files/*.{whl,tar.gz} 2>/dev/null | \


But for all other packages, we want to just install whatever we have there without dependencies. If the packages are result of pip download, this serves the purpose of 'offline installation" mechanism where you download all packages and store them in artifact registry and 'vet' and use them for installation.

potiuk · 2021-01-01T13:32:31Z

Dockerfile.ci

@@ -325,7 +325,8 @@ RUN if [[ ${INSTALL_FROM_PYPI} == "true" ]]; then \
 pip install -e ".[${AIRFLOW_EXTRAS}]" --upgrade --upgrade-strategy eager; \
 pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \
 else \
- pip install -e ".[${AIRFLOW_EXTRAS}]" --upgrade --upgrade-strategy only-if-needed; \
+ pip install -e ".[${AIRFLOW_EXTRAS}]" --upgrade --upgrade-strategy only-if-needed\
+ --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \


We want to stick to the constraints here as well (unless setup.py requirements change)

potiuk · 2021-01-01T13:33:44Z

Dockerfile.ci

-RUN if [[ ${INSTALL_FROM_DOCKER_CONTEXT_FILES} != "true" ]]; then \
- if ls /docker-context-files/*.{whl,tar.gz} 1> /dev/null 2>&1; then \
- pip install --no-deps /docker-context-files/*.{whl,tar.gz}; \
+# hadolint ignore=SC2086, SC2010


Same 'docker-file-context' installation for CI image. It might be useful only in case we want to test CI image built from packages, good to keep that option.

potiuk · 2021-01-01T13:35:50Z

scripts/ci/libraries/_build_images.sh

 # Cleanup dist and docker-context-files folders
 mkdir -pv "${AIRFLOW_SOURCES}/dist"
 mkdir -pv "${AIRFLOW_SOURCES}/docker-context-files"
 rm -f "${AIRFLOW_SOURCES}/dist/"*.{whl,tar.gz}
 rm -f "${AIRFLOW_SOURCES}/docker-context-files/"*.{whl,tar.gz}

- runs::run_pip_download


We do not need to run pip download now. This is good because this code is run from master, so previously we could not change the requirements for that pip download. In the #13409 we are going to add per-image requirements and since they will be in the images, not scripts, they are safe to run in workflow_run

potiuk · 2021-01-01T13:51:56Z

I think this one should be good to go! I need to merge it before #13409

potiuk · 2021-01-01T21:32:28Z

It's green! seems that the "reverse lookup disabling" solves the problem and we are all green!

mik-laj · 2021-01-01T22:29:55Z

Dockerfile

 AIRFLOW_EXTRAS=${AIRFLOW_EXTRAS/mysql,}; \
 fi; \
 if [[ ${INSTALL_FROM_PYPI} == "true" ]]; then \
 if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \
 pip install --user "${AIRFLOW_INSTALLATION_METHOD}[${AIRFLOW_EXTRAS}]${AIRFLOW_INSTALL_VERSION}" \
 --upgrade --upgrade-strategy eager; \
+ pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \


Can we add this limitation to the previous command so that this update never happens?

pip install --user "${AIRFLOW_INSTALLATION_METHOD}[${AIRFLOW_EXTRAS}]${AIRFLOW_INSTALL_VERSION}" \ --constraint <<(echo "pip==${AIRFLOW_PIP_VERSION}") \ --upgrade --upgrade-strategy eager; \

What update do you suggest?

I prefer to leave it as is unless you have better proposal

In the first comment, I added an example. To show my idea better, I prepared the entire commit. Does this solve this problem?

From cc68b6ba19a4bc11e5aed3ec01f3e4f86f76f745 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Bregu=C5=82a?= <kamil.bregula@polidea.com> Date: Sat, 2 Jan 2021 01:16:03 +0100 Subject: Add constraints to prevent upgrade of pip --- Dockerfile | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/Dockerfile b/Dockerfile index 81737a15b..b72d859dc 100644 --- a/Dockerfile +++ b/Dockerfile @@ -255,13 +255,13 @@ RUN if [[ ${INSTALL_MYSQL_CLIENT} != "true" ]]; then \ if [[ ${INSTALL_FROM_PYPI} == "true" ]]; then \ if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \ pip install --user "${AIRFLOW_INSTALLATION_METHOD}[${AIRFLOW_EXTRAS}]${AIRFLOW_INSTALL_VERSION}" \ + --constraint <(echo "pip==${AIRFLOW_PIP_VERSION}") \ --upgrade --upgrade-strategy eager; \ - pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \ else \ pip install --upgrade --upgrade-strategy only-if-needed \ --user "${AIRFLOW_INSTALLATION_METHOD}[${AIRFLOW_EXTRAS}]${AIRFLOW_INSTALL_VERSION}" \ + --constraint <(echo "pip==${AIRFLOW_PIP_VERSION}") \ --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \ - pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \ fi; \ fi; \ if [[ ${INSTALL_FROM_DOCKER_CONTEXT_FILES} == "true" ]]; then \ @@ -270,12 +270,13 @@ RUN if [[ ${INSTALL_MYSQL_CLIENT} != "true" ]]; then \ if [[ "${reinstalling_apache_airflow_packages}" != "" ]]; then \ if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \ pip install --force-reinstall --upgrade --upgrade-strategy eager \ + --constraint <(echo "pip==${AIRFLOW_PIP_VERSION}") \ --user ${reinstalling_apache_airflow_packages}; \ - pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \ else \ pip install --force-reinstall --upgrade --upgrade-strategy only-if-needed \ - --user ${reinstalling_apache_airflow_packages} --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \ - pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \ + --user ${reinstalling_apache_airflow_packages} \ + --constraint <(echo "pip==${AIRFLOW_PIP_VERSION}") \ + --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \ fi; \ fi ; \ # All the others we want to reinstall as-is, without dependencies \ @@ -287,12 +288,14 @@ RUN if [[ ${INSTALL_MYSQL_CLIENT} != "true" ]]; then \ fi; \ if [[ -n "${ADDITIONAL_PYTHON_DEPS}" ]]; then \ if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \ - pip install --user ${ADDITIONAL_PYTHON_DEPS} --upgrade --upgrade-strategy eager; \ - pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \ + pip install --user ${ADDITIONAL_PYTHON_DEPS} \ + --constraint <(echo "pip==${AIRFLOW_PIP_VERSION}") \ + --upgrade --upgrade-strategy eager; \ else \ - pip install --user ${ADDITIONAL_PYTHON_DEPS} --upgrade --upgrade-strategy only-if-needed \ + pip install --user ${ADDITIONAL_PYTHON_DEPS} \ + --upgrade --upgrade-strategy only-if-needed \ + --constraint <(echo "pip==${AIRFLOW_PIP_VERSION}") \ --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \ - pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \ fi; \ fi; \ find /root/.local/ -name '*.pyc' -print0 | xargs -0 rm -r || true ; \ -- 2.28.0

I think it might not solve it. I have no 100% certainly but --constraint is not really binding (at least for pip 20.2.4). When you are upgrading with eager strategy, PIP often installs dependencies which are not within constraints. Constraint is merely a "hint" which does not have to be followed (and especially with 'eager' upgrade strategy.

I think I prefer to explicitly force pip version by installing it explicitly after.

This PR improves building production image from local packages, in preparation for moving provider requirements out of setup.cfg. Previously `pip download` step was executed in the CI scripts in order to download all the packages that were needed. However this had two problems: 1) PIP download was executed outside of Dockerfile in CI scripts which means that any change to requirements there could not be executed in 'workflow_run' event - because main branch version of CI scripts is used there. We want to add extra requirements when installing airflow so in order to be able to change it, those requirements should be added in Dockerfile. This will be done in the follow-up apache#13409 PR. 2) Packages downloaded with PIP download have a "file" version rather than regular == version when you run pip freeze/check. This looks weird and while you can figure out the version from file name, when you `pip install` them, they look much more normal. The airflow package and provider package will still get the "file" form but this is ok because we are building those packages from sources and they are not yet available in PyPI. Example: adal==1.2.5 aiohttp==3.7.3 alembic==1.4.3 amqp==2.6.1 apache-airflow @ file:///docker-context-files/apache_airflow-2.1.0.dev0-py3-none-any.whl apache-airflow-providers-amazon @ file:///docker-context-files/apache_airflow_providers_amazon-1.0.0-py3-none-any.whl apache-airflow-providers-celery @ file:///docker-context-files/apache_airflow_providers_celery-1.0.0-py3-none-any.whl ... With this PR, we do not `pip download` all packages, but instead we prepare airflow + providers packages as .whl files and install them from there (all the dependencies are installed from PyPI)

github-actions · 2021-01-02T01:09:33Z

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.

In the latest change apache#13422 change in the way product images are prepared removed extras from installed airflow - thus caused failing production image verification check. This change restores extras when airflow is installed from packages

In the latest change #13422 change in the way product images are prepared removed extras from installed airflow - thus caused failing production image verification check. This change restores extras when airflow is installed from packages

This PR improves building production image from local packages, in preparation for moving provider requirements out of setup.cfg. Previously `pip download` step was executed in the CI scripts in order to download all the packages that were needed. However this had two problems: 1) PIP download was executed outside of Dockerfile in CI scripts which means that any change to requirements there could not be executed in 'workflow_run' event - because main branch version of CI scripts is used there. We want to add extra requirements when installing airflow so in order to be able to change it, those requirements should be added in Dockerfile. This will be done in the follow-up #13409 PR. 2) Packages downloaded with PIP download have a "file" version rather than regular == version when you run pip freeze/check. This looks weird and while you can figure out the version from file name, when you `pip install` them, they look much more normal. The airflow package and provider package will still get the "file" form but this is ok because we are building those packages from sources and they are not yet available in PyPI. Example: adal==1.2.5 aiohttp==3.7.3 alembic==1.4.3 amqp==2.6.1 apache-airflow @ file:///docker-context-files/apache_airflow-2.1.0.dev0-py3-none-any.whl apache-airflow-providers-amazon @ file:///docker-context-files/apache_airflow_providers_amazon-1.0.0-py3-none-any.whl apache-airflow-providers-celery @ file:///docker-context-files/apache_airflow_providers_celery-1.0.0-py3-none-any.whl ... With this PR, we do not `pip download` all packages, but instead we prepare airflow + providers packages as .whl files and install them from there (all the dependencies are installed from PyPI) (cherry picked from commit e436883)

In the latest change #13422 change in the way product images are prepared removed extras from installed airflow - thus caused failing production image verification check. This change restores extras when airflow is installed from packages (cherry picked from commit 3a73110)

boring-cyborg bot added the area:dev-tools label Jan 1, 2021

potiuk requested review from ashb, feluelle, mik-laj and kaxil January 1, 2021 07:32

potiuk force-pushed the remove-pip-download-when-building-prod-image branch 3 times, most recently from b5b516b to c57e24b Compare January 1, 2021 08:08

potiuk mentioned this pull request Jan 1, 2021

Removes provider-imposed requirements from setup.cfg #13409

Merged

potiuk force-pushed the remove-pip-download-when-building-prod-image branch from c57e24b to a1ba2d2 Compare January 1, 2021 12:31

potiuk force-pushed the remove-pip-download-when-building-prod-image branch from a1ba2d2 to 7144b35 Compare January 1, 2021 13:17

potiuk commented Jan 1, 2021

View reviewed changes

potiuk force-pushed the remove-pip-download-when-building-prod-image branch 3 times, most recently from 26df514 to ee773de Compare January 1, 2021 19:38

potiuk mentioned this pull request Jan 1, 2021

Change timeouts and disables reverse IP lookup for integrations #13424

Merged

mik-laj reviewed Jan 1, 2021

View reviewed changes

potiuk force-pushed the remove-pip-download-when-building-prod-image branch from ee773de to 583b049 Compare January 2, 2021 00:52

mik-laj approved these changes Jan 2, 2021

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 Jan 2, 2021

potiuk merged commit e436883 into apache:master Jan 2, 2021

potiuk deleted the remove-pip-download-when-building-prod-image branch January 2, 2021 10:16

potiuk added this to the Airflow 2.0.1 milestone Jan 2, 2021

potiuk mentioned this pull request Jan 2, 2021

Add extras when installing prod image from packages #13432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes pip download when installing from local packages #13422

Removes pip download when installing from local packages #13422

potiuk commented Jan 1, 2021 •

edited

Loading

potiuk commented Jan 1, 2021

github-actions bot commented Jan 1, 2021

github-actions bot commented Jan 1, 2021

github-actions bot commented Jan 1, 2021

potiuk Jan 1, 2021

potiuk Jan 1, 2021

potiuk Jan 1, 2021 •

edited

Loading

potiuk Jan 1, 2021 •

edited

Loading

potiuk Jan 1, 2021

potiuk Jan 1, 2021

potiuk Jan 1, 2021

potiuk commented Jan 1, 2021

potiuk commented Jan 1, 2021

mik-laj Jan 1, 2021

potiuk Jan 1, 2021

potiuk Jan 1, 2021 •

edited

Loading

mik-laj Jan 2, 2021 •

edited

Loading

potiuk Jan 2, 2021

github-actions bot commented Jan 2, 2021

Removes pip download when installing from local packages #13422

Removes pip download when installing from local packages #13422

Conversation

potiuk commented Jan 1, 2021 • edited Loading

potiuk commented Jan 1, 2021

github-actions bot commented Jan 1, 2021

github-actions bot commented Jan 1, 2021

github-actions bot commented Jan 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk Jan 1, 2021 • edited Loading

Choose a reason for hiding this comment

potiuk Jan 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Jan 1, 2021

potiuk commented Jan 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk Jan 1, 2021 • edited Loading

Choose a reason for hiding this comment

mik-laj Jan 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 2, 2021

potiuk commented Jan 1, 2021 •

edited

Loading

potiuk Jan 1, 2021 •

edited

Loading

potiuk Jan 1, 2021 •

edited

Loading

potiuk Jan 1, 2021 •

edited

Loading

mik-laj Jan 2, 2021 •

edited

Loading