You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/10/25 19:06:25 UTC
[GitHub] [airflow] potiuk opened a new pull request #19210: Optimizes building of Production image for non-modified www files
potiuk opened a new pull request #19210:
URL: https://github.com/apache/airflow/pull/19210
When none of the `www` files changed, we do not have to compile
assets. Unfortunately the old Dockerfile run COPY for all sources
before the asset compilation, so whenever any file changed in
sources, the asset compilation was triggered. This change fixes it
in the way that assets are compiled first, the dist files are
stored on the side and copied back after all sources are copied
again (this way they will not get overwritten).
This is done in the "build" segment of the image so it has no
impact of the size of the "final" image. It will speed up both
developing changes for the image as well as building PROD image
in CI when none of the www files change (which is very, very
often)
Based on #19189 (only last commit needs to be reviewed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #19210: Optimizes building of Production image for non-modified www files
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19210:
URL: https://github.com/apache/airflow/pull/19210#discussion_r751158036
##########
File path: Dockerfile
##########
@@ -160,13 +160,21 @@ ARG INSTALL_PROVIDERS_FROM_SOURCES="false"
# But it also can be `.` from local installation or GitHub URL pointing to specific branch or tag
# Of Airflow. Note That for local source installation you need to have local sources of
# Airflow checked out together with the Dockerfile and AIRFLOW_SOURCES_FROM and AIRFLOW_SOURCES_TO
-# set to "." and "/opt/airflow" respectively.
+# set to "." and "/opt/airflow" respectively. Similarly AIRFLOW_SOURCES_WWW_FROM/TO are set to right source
+# and destination
ARG AIRFLOW_INSTALLATION_METHOD="apache-airflow"
# By default latest released version of airflow is installed (when empty) but this value can be overridden
# and we can install version according to specification (For example ==2.0.2 or <3.0.0).
ARG AIRFLOW_VERSION_SPECIFICATION=""
# By default we do not upgrade to latest dependencies
ARG UPGRADE_TO_NEWER_DEPENDENCIES="false"
+# By default we install latest airflow from PyPI so we do not need to copy sources of Airflow
+# www to compile the assets but in case of breeze/CI builds we use latest sources and we override those
+# those SOURCES_FROM/TO with "airflow/wwww" and "/opt/airflow/airflow/wwww" respectively.
+# This is to rebuild the assets only when any of the www sources change
+ARG AIRFLOW_SOURCES_WWW_FROM="empty"
+ARG AIRFLOW_SOURCES_WWW_TO="/empty"
Review comment:
The `empty' is as the solution for to build the image using either released PyPI packages or local sources (which is optimised for iteration speed with kind tests).
By default when prod docker image is built, you specify Airflow version (2.2.2 with latest version) and the image is built using that version from PyPI. This is case 1) of building the PROD image - and it does not matter at all what local sources you have of airflow - always PyPI package is installed.
For testing and building image in the CI we need to use local sources - so the PROD image in CI is built using locally prepared packages from sources. In this case "airflow" and "provider" packages are built locally and copied to "docker-context-files" - and installed from there rather than from PyPI. This is case 2) of building the image.
However the very same Dockerfile is also used locally to test "kind" tests - using local sources. We do not have packages in PYPI for those local sources, and we do not want to rebuild the airflow package and copy it to "docker-context-files", as it would mean a very slow iteration speed. If we use packages then after every single source code change you'd have to rebuild the packages, copy them to docker-context files, rebuild the imagae and upload it to kind. This is terribly slow iteration speed because any change to "docker-context-files" invalidates the layer when the packages are installed and this means a lot of time lost for rebuilding the layer.
So instead of building airlfow from packages, for local kind testing we are installing airflow from sources (similarly as in CI images). Instead of preparing packages and copying them to docker-context files we simply COPY "airflow" folder for the prod image and airflow is installed from those sources in editable mode. In the future in this case we could even employ some framework to mount the airflow folder to kind cluster to avoid rebuilding altogether when the sources change (but that's future optimisation if needed. That saves at least couple of minutes on every iteration of tests. This is case 3) of building the PROD image
In case 1) and 2) we do not want to copy airflow sources (it slows down the docker build a lot and increases the size of the "build" segment a lot) - also it means that this layer of the image would be invalidated every time airflow sources change. So by default in those cases we copy"empty" directory - it's super fast and will not invalidate if you change sources. Unfortunately Dockerfile does not have "optional" steps - so if in case 3) we want to COPY airflow sources, then in case 1) and 2) we also need to copy "something". I impemented it in the way that "what" and "where" we copy is passed as args, which works nicely - in 1) 2) i copy empty folder to /empty dir and in 3) we copy "airflow/www" to "/opt/airlfow/airflow/www" and later "airflow" to "/opt/airlfow/airflow". The optimisation above will do it in two steps in order to optimise the case when no "www" changes so we save time on invalidating layers and rebuilding the "yarn" stuff - saving another good 30 seconds (or mo
re if you have slow connection) on rebuilding this layer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #19210: Optimizes building of Production image for non-modified www files
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19210:
URL: https://github.com/apache/airflow/pull/19210#discussion_r751081095
##########
File path: Dockerfile
##########
@@ -160,13 +160,21 @@ ARG INSTALL_PROVIDERS_FROM_SOURCES="false"
# But it also can be `.` from local installation or GitHub URL pointing to specific branch or tag
# Of Airflow. Note That for local source installation you need to have local sources of
# Airflow checked out together with the Dockerfile and AIRFLOW_SOURCES_FROM and AIRFLOW_SOURCES_TO
-# set to "." and "/opt/airflow" respectively.
+# set to "." and "/opt/airflow" respectively. Similarly AIRFLOW_SOURCES_WWW_FROM/TO are set to right source
+# and destination
ARG AIRFLOW_INSTALLATION_METHOD="apache-airflow"
# By default latest released version of airflow is installed (when empty) but this value can be overridden
# and we can install version according to specification (For example ==2.0.2 or <3.0.0).
ARG AIRFLOW_VERSION_SPECIFICATION=""
# By default we do not upgrade to latest dependencies
ARG UPGRADE_TO_NEWER_DEPENDENCIES="false"
+# By default we install latest airflow from PyPI so we do not need to copy sources of Airflow
+# www to compile the assets but in case of breeze/CI builds we use latest sources and we override those
+# those SOURCES_FROM/TO with "airflow/wwww" and "/opt/airflow/airflow/wwww" respectively.
+# This is to rebuild the assets only when any of the www sources change
+ARG AIRFLOW_SOURCES_WWW_FROM="empty"
+ARG AIRFLOW_SOURCES_WWW_TO="/empty"
Review comment:
```suggestion
# By default we install latest airflow from PyPI so we do not need to copy sources of Airflow
# www to compile the assets but in case of breeze/CI builds we use latest sources and we override those
# those SOURCES_FROM/TO with "airflow/www" and "/opt/airflow/airflow/www" respectively.
# This is to rebuild the assets only when any of the www sources change
ARG AIRFLOW_SOURCES_WWW_FROM="empty"
ARG AIRFLOW_SOURCES_WWW_TO="/empty"
```
Also what does `empty` mean?
##########
File path: scripts/ci/libraries/_verify_image.sh
##########
@@ -340,7 +340,7 @@ function verify_image::verify_prod_image_as_root() {
function verify_image::verify_production_image_has_dist_folder() {
start_end::group_start "Verify prod image has dist folder (compiled www assets): ${DOCKER_IMAGE}"
# shellcheck disable=SC2016
- verify_image::check_command "Dist folder" '[ -f $(python -m site --user-site)/airflow/www/static/dist/manifest.json ] || exit 1'
+ verify_image::check_command "Dist folder" '[ -f /.venv/lib/python$(python -c "import sys; print(str(sys.version_info[0])+\".\"+str(sys.version_info[1]))")/site-packages/airflow/www/static/dist/manifest.json ] || exit 1'
Review comment:
Probably easier to use `importlib.resources` instead of manually constructing the path.
##########
File path: airflow/operators/python.py
##########
@@ -380,6 +390,8 @@ def __init__(
self.requirements.append('lazy-object-proxy')
if self.use_dill and 'dill' not in self.requirements:
self.requirements.append('dill')
+ # Only allows to use clone_virtualenv_packages if no python version is specified
+ self.clone_virtualenv_packages = clone_airflow_virtualenv if not python_version else False
Review comment:
I think we should
1. Raise an exception if both arguments are set
2. Maybe ignore `python_version` if it matches `sys.version_info`...?
##########
File path: airflow/utils/python_virtualenv.py
##########
@@ -75,7 +85,11 @@ def remove_task_decorator(python_source: str, task_decorator_name: str) -> str:
def prepare_virtualenv(
- venv_directory: str, python_bin: str, system_site_packages: bool, requirements: List[str]
+ venv_directory: str,
+ python_bin: str,
+ system_site_packages: bool,
+ requirements: List[str],
+ clone_virtualenv_packages: bool = False,
Review comment:
I feel we may want to infer this from `system_site_packages` by default. I’d imagine most existing usages of this function specify `system_site_packages` because they want to access Airflow, but that would be broken by this PR since Airflow is no longer a part of the site-packages directory.
##########
File path: scripts/docker/install_from_docker_context_files.sh
##########
@@ -88,14 +87,14 @@ function install_airflow_and_providers_from_docker_context_files(){
--constraint /tmp/constraints.txt
rm /tmp/constraints.txt
# make sure correct PIP version is used \
- pip install ${AIRFLOW_INSTALL_USER_FLAG} --upgrade "pip==${AIRFLOW_PIP_VERSION}"
+ pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"
# then upgrade if needed without using constraints to account for new limits in setup.py
- pip install ${AIRFLOW_INSTALL_USER_FLAG} --upgrade --upgrade-strategy only-if-needed \
+ pip install --upgrade --upgrade-strategy only-if-needed \
${reinstalling_apache_airflow_package} ${reinstalling_apache_airflow_providers_packages}
fi
# make sure correct PIP version is left installed
- pip install ${AIRFLOW_INSTALL_USER_FLAG} --upgrade "pip==${AIRFLOW_PIP_VERSION}"
+ pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"
Review comment:
```suggestion
pip install "pip==${AIRFLOW_PIP_VERSION}"
```
##########
File path: airflow/utils/python_virtualenv.py
##########
@@ -89,10 +103,17 @@ def prepare_virtualenv(
:type system_site_packages: bool
:param requirements: List of additional python packages
:type requirements: List[str]
+ :param clone_virtualenv_packages: whether to clone aurflow virtualenv package if airflow is run
+ in virtualenv - default is False
+ :type clone_virtualenv_packages: bool
:return: Path to a binary file with Python in a virtual environment.
:rtype: str
"""
- virtualenv_cmd = _generate_virtualenv_cmd(venv_directory, python_bin, system_site_packages)
+ virtualenv_cmd = _generate_virtualenv_cmd(
+ venv_directory, python_bin, system_site_packages, clone_virtualenv_packages
+ )
+ # Virtualenv-clone requires the directory to be non-existing
+ shutil.rmtree(path=venv_directory, ignore_errors=True)
Review comment:
We may want to explicitly error out here instead to avoid the user accidentally nuking something. A `force` flag can be added if needed.
##########
File path: scripts/docker/install_airflow_dependencies_from_branch_tip.sh
##########
@@ -39,11 +39,11 @@ function install_airflow_dependencies_from_branch_tip() {
fi
# Install latest set of dependencies using constraints. In case constraints were upgraded and there
# are conflicts, this might fail, but it should be fixed in the following installation steps
- pip install ${AIRFLOW_INSTALL_USER_FLAG} \
+ pip install \
"https://github.com/${AIRFLOW_REPO}/archive/${AIRFLOW_BRANCH}.tar.gz#egg=apache-airflow[${AIRFLOW_EXTRAS}]" \
--constraint "${AIRFLOW_CONSTRAINTS_LOCATION}" || true
# make sure correct PIP version is used
- pip install ${AIRFLOW_INSTALL_USER_FLAG} --upgrade "pip==${AIRFLOW_PIP_VERSION}"
+ pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"
Review comment:
```suggestion
pip install "pip==${AIRFLOW_PIP_VERSION}"
```
`--upgrade` is useless when a version is explicitly specified.
##########
File path: scripts/docker/install_pip_version.sh
##########
@@ -30,11 +30,12 @@
. "$( dirname "${BASH_SOURCE[0]}" )/common.sh"
function install_pip_version() {
- pip install --no-cache-dir --upgrade "pip==${AIRFLOW_PIP_VERSION}" && mkdir -p /root/.local/bin
+ pip install --no-cache-dir --upgrade "pip==${AIRFLOW_PIP_VERSION}"
Review comment:
```suggestion
pip install --no-cache-dir "pip==${AIRFLOW_PIP_VERSION}"
```
##########
File path: setup.cfg
##########
@@ -157,6 +157,7 @@ install_requires =
termcolor>=1.1.0
typing-extensions>=3.7.4;python_version<"3.8"
unicodecsv>=0.14.1
+ virtualenv-clone
Review comment:
We should also add `virtualenv>=20`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #19210: Optimizes building of Production image for non-modified www files
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19210:
URL: https://github.com/apache/airflow/pull/19210#discussion_r756957574
##########
File path: scripts/docker/install_airflow_dependencies_from_branch_tip.sh
##########
@@ -39,11 +39,11 @@ function install_airflow_dependencies_from_branch_tip() {
fi
# Install latest set of dependencies using constraints. In case constraints were upgraded and there
# are conflicts, this might fail, but it should be fixed in the following installation steps
- pip install ${AIRFLOW_INSTALL_USER_FLAG} \
+ pip install \
"https://github.com/${AIRFLOW_REPO}/archive/${AIRFLOW_BRANCH}.tar.gz#egg=apache-airflow[${AIRFLOW_EXTRAS}]" \
--constraint "${AIRFLOW_CONSTRAINTS_LOCATION}" || true
# make sure correct PIP version is used
- pip install ${AIRFLOW_INSTALL_USER_FLAG} --upgrade "pip==${AIRFLOW_PIP_VERSION}"
+ pip install --upgrade "pip==${AIRFLOW_PIP_VERSION}"
Review comment:
Ah yeah. Bad habit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #19210: Optimizes building of Production image for non-modified www files
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #19210:
URL: https://github.com/apache/airflow/pull/19210
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #19210: Optimizes building of Production image for non-modified www files
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19210:
URL: https://github.com/apache/airflow/pull/19210#issuecomment-979280015
I wil rebase that one but let's not comment in #1989 for the changes related to changes there (only last commit is relevant here)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #19210: Optimizes building of Production image for non-modified www files
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19210:
URL: https://github.com/apache/airflow/pull/19210#discussion_r751158763
##########
File path: airflow/operators/python.py
##########
@@ -380,6 +390,8 @@ def __init__(
self.requirements.append('lazy-object-proxy')
if self.use_dill and 'dill' not in self.requirements:
self.requirements.append('dill')
+ # Only allows to use clone_virtualenv_packages if no python version is specified
+ self.clone_virtualenv_packages = clone_airflow_virtualenv if not python_version else False
Review comment:
AH yeah - that's the "previous" PR (this one is based on it) - I will come back to it shortly and address the comments there :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #19210: Optimizes building of Production image for non-modified www files
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19210:
URL: https://github.com/apache/airflow/pull/19210#discussion_r751159987
##########
File path: airflow/operators/python.py
##########
@@ -380,6 +390,8 @@ def __init__(
self.requirements.append('lazy-object-proxy')
if self.use_dill and 'dill' not in self.requirements:
self.requirements.append('dill')
+ # Only allows to use clone_virtualenv_packages if no python version is specified
+ self.clone_virtualenv_packages = clone_airflow_virtualenv if not python_version else False
Review comment:
It's the #19189 - but I will take your comments and answer here and address them there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #19210: Optimizes building of Production image for non-modified www files
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19210:
URL: https://github.com/apache/airflow/pull/19210#discussion_r756957952
##########
File path: setup.cfg
##########
@@ -157,6 +157,7 @@ install_requires =
termcolor>=1.1.0
typing-extensions>=3.7.4;python_version<"3.8"
unicodecsv>=0.14.1
+ virtualenv-clone
Review comment:
Why ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org