-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Removes pip download when installing from local packages #13422
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -247,31 +247,54 @@ ENV UPGRADE_TO_NEWER_DEPENDENCIES=${UPGRADE_TO_NEWER_DEPENDENCIES} | |
|
||
WORKDIR /opt/airflow | ||
|
||
# remove mysql from extras if client is not installed | ||
# hadolint ignore=SC2086, SC2010 | ||
RUN if [[ ${INSTALL_MYSQL_CLIENT} != "true" ]]; then \ | ||
# Remove mysql from extras if client is not installed \ | ||
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}"; \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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; \ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What update do you suggest? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to leave it as is unless you have better proposal There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might not solve it. I have no 100% certainly but I think I prefer to explicitly force pip version by installing it explicitly after. |
||
else \ | ||
pip install --user "${AIRFLOW_INSTALLATION_METHOD}[${AIRFLOW_EXTRAS}]${AIRFLOW_INSTALL_VERSION}" \ | ||
pip install --upgrade --upgrade-strategy only-if-needed \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
--user "${AIRFLOW_INSTALLATION_METHOD}[${AIRFLOW_EXTRAS}]${AIRFLOW_INSTALL_VERSION}" \ | ||
--constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \ | ||
pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \ | ||
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); \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting one. Instead of This is the case where we haave only "airflow" + "airflow packages" build from sources to install (which happens on CI). |
||
# We want to install apache airflow packages with constraints \ | ||
if [[ "${reinstalling_apache_airflow_packages}" != "" ]]; then \ | ||
if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \ | ||
pip install --force-reinstall --upgrade --upgrade-strategy eager \ | ||
--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}"; \ | ||
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 | \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But for all other packages, we want to just install whatever we have there without dependencies. If the packages are result of |
||
grep -v apache_airflow | grep -v apache-airflow || true); \ | ||
if [[ "${reinstalling_other_packages}" != "" ]]; then \ | ||
pip install --force-reinstall --user --no-deps ${reinstalling_other_packages}; \ | ||
fi; \ | ||
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}"; \ | ||
else \ | ||
pip install --user ${ADDITIONAL_PYTHON_DEPS} --constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \ | ||
pip install --user ${ADDITIONAL_PYTHON_DEPS} --upgrade --upgrade-strategy only-if-needed \ | ||
--constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \ | ||
pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \ | ||
fi; \ | ||
fi; \ | ||
if [[ ${INSTALL_FROM_DOCKER_CONTEXT_FILES} == "true" ]]; then \ | ||
if ls /docker-context-files/*.{whl,tar.gz} 1> /dev/null 2>&1; then \ | ||
pip install --user --no-deps /docker-context-files/*.{whl,tar.gz}; \ | ||
fi ; \ | ||
fi; \ | ||
find /root/.local/ -name '*.pyc' -print0 | xargs -0 rm -r || true ; \ | ||
find /root/.local/ -type d -name '__pycache__' -print0 | xargs -0 rm -r || true | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -281,8 +281,8 @@ RUN pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}" | |
RUN if [[ ${AIRFLOW_PRE_CACHED_PIP_PACKAGES} == "true" ]]; then \ | ||
pip install \ | ||
"https://github.com/${AIRFLOW_REPO}/archive/${AIRFLOW_BRANCH}.tar.gz#egg=apache-airflow[${AIRFLOW_EXTRAS}]" \ | ||
--constraint "${AIRFLOW_CONSTRAINTS_LOCATION}" \ | ||
&& pip uninstall --yes apache-airflow; \ | ||
--constraint "${AIRFLOW_CONSTRAINTS_LOCATION}"; \ | ||
pip uninstall --yes apache-airflow; \ | ||
fi | ||
|
||
# Generate random hex dump file so that we can determine whether it's faster to rebuild the image | ||
|
@@ -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}"; \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want to stick to the constraints here as well (unless setup.py requirements change) |
||
pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"; \ | ||
fi; \ | ||
fi | ||
|
@@ -334,11 +335,28 @@ RUN if [[ ${INSTALL_FROM_PYPI} == "true" ]]; then \ | |
# they are also installed additionally to whatever is installed from Airflow. | ||
COPY docker-context-files/ /docker-context-files/ | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
RUN 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); \ | ||
# We want to install apache airflow packages with constraints \ | ||
if [[ "${reinstalling_apache_airflow_packages}" != "" ]]; then \ | ||
if [[ "${UPGRADE_TO_NEWER_DEPENDENCIES}" != "false" ]]; then \ | ||
pip install --force-reinstall --upgrade --upgrade-strategy eager \ | ||
--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}"; \ | ||
fi; \ | ||
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 | \ | ||
grep -v apache_airflow | grep -v apache-airflow || true); \ | ||
if [[ "${reinstalling_other_packages}" != "" ]]; then \ | ||
pip install --force-reinstall --user --no-deps ${reinstalling_other_packages}; \ | ||
fi; \ | ||
fi; | ||
|
||
# Copy all the www/ files we need to compile assets. Done as two separate COPY | ||
# commands so as otherwise it copies the _contents_ of static/ in to www/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -913,31 +913,40 @@ function build_images::determine_docker_cache_strategy() { | |
} | ||
|
||
|
||
function build_images::build_prod_images_from_packages() { | ||
function build_image::assert_variable() { | ||
local variable_name="${1}" | ||
local expected_value="${2}" | ||
local variable_value=${!variable_name} | ||
if [[ ${variable_value} != "${expected_value}" ]]; then | ||
echo | ||
echo "${COLOR_RED_ERROR}: Variable ${variable_name}: expected_value: '${expected_value}' but was '${variable_value}'!${COLOR_RESET}" | ||
echo | ||
exit 1 | ||
fi | ||
} | ||
|
||
function build_images::build_prod_images_from_locally_built_airflow_packages() { | ||
# We do not install from PyPI | ||
build_image::assert_variable INSTALL_FROM_PYPI "false" | ||
# But then we reinstall airflow and providers from prepared packages in the docker context files | ||
build_image::assert_variable INSTALL_FROM_DOCKER_CONTEXT_FILES "true" | ||
# But we install everything from scratch to make a "clean" installation in case any dependencies got removed | ||
build_image::assert_variable AIRFLOW_PRE_CACHED_PIP_PACKAGES "false" | ||
|
||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
# Remove all downloaded apache airflow packages | ||
rm -f "${AIRFLOW_SOURCES}/dist/"apache_airflow*.whl | ||
rm -f "${AIRFLOW_SOURCES}/dist/"apache-airflow*.tar.gz | ||
|
||
# Remove all downloaded apache airflow packages | ||
mv -f "${AIRFLOW_SOURCES}/dist/"* "${AIRFLOW_SOURCES}/docker-context-files/" | ||
|
||
# Build necessary provider packages | ||
runs::run_prepare_provider_packages "${INSTALLED_PROVIDERS[@]}" | ||
|
||
mv "${AIRFLOW_SOURCES}/dist/"* "${AIRFLOW_SOURCES}/docker-context-files/" | ||
|
||
# Build apache airflow packages | ||
build_airflow_packages::build_airflow_packages | ||
|
||
mv "${AIRFLOW_SOURCES}/dist/"* "${AIRFLOW_SOURCES}/docker-context-files/" | ||
|
||
build_images::build_prod_images_with_group | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we resore PIP to the version specified (eager upgrade will upgrade it)