Swtich both PROD and CI images to use virtual environments - #19189
Conversation
boring-cyborg
Bot
added
area:dev-tools
area:production-image
52ebaf8 to
a125025
Compare
|
Hey @uranusjr @kaxil @ashb (and others). I have an interesting case that I need your opinion on. Saving you some need to read the (unpleaseant at times) discussion there - short context here (at least my view of it). Seems that the "recommended" way forward for any future installations of pip dependencies is via virtualenv. This is a bit implicit and (as I see it at least) not stated clearly and strongly enough to make it the "only" apprioach, but on the other hand it results in some unremovable warnings in our image that migh confuse our users. Since it seems that no-one had good answers to my questins, I decided to take a stab on it and see what it means for our images (both CI and PROD) to switch from the --user approach to uising venv. I already investigated and fixed a few issues connected (documented also in the PyPI discussion) and I plan to improve the recommendation (and possibly even to help to make it a bit more firm/stronger recommendation - especially for people who build docker images and want to get them optimized by size and follow "aribitrary user id" recommendations from OpenShift. It's mainly about copying venvs between stages in docker image building and applicability of re-using the single venv among mutliple users. I have a few observations and recomendation for other people trying to do it already, but one question remains - so far about our approach of creating venv by PythonVirtualenvOperator. The main problem is (and it is partially an Airflow bug, partially the way how virtualenv work) - that even currently PythonVirtualenvOperator will behave DIFFERENTLY when airflow is instaled as "system" package and when it is installed in virtualenv. This results in The problem is that if Airflow (and all its dependencies) are installed in virtualenv, then PythonVirtualenvOperator will behave wrongly. Specifically in this case it will fail if "dill" and "system_site_packages" are enabled (even without
I tried to fix it in by adding I'd love to hear your opinion on that. I see three possible approaches:
Would love to hear what others think about it. |
|
Hmm. I see e problem of it interacting with It's a mess and I think we will not be able to have a consistent behaviour - seems like users will have to understand the differences between different ways airflow might be installed and it's effect on PythonVirtualenvOperator. |
b6162b2 to
e4acf16
Compare
|
Actually. I made it works with "virtualenv-clone" approach (so approach number 3).
If Airflow is installed as system/user package or when someone specified Let me know what you think about this approach - I think it is as "close" as it can get to expected behaviour and as little potentialy disruptive as possible (I think the case where someone had virtualenv with airflow and no dill/airflow specified in requirements, it would not not work anyway so this is not really breaking change - it fixes a case which was broken and non-functional. |
|
I am also happy to split this change in two if we agree it's good and if we see it works:
|
e4acf16 to
349911f
Compare
|
Looks like it is ready for review - all tests should be passing (there was one intermittent test that failed). |
|
I've be hesitant to propose this since this is technically a backward incompatibility change for those using PROD as a base image for their Dockerfile, the most significant part being (obviously) the location of the interpreter. So while I think this is a good thing to do in a vacuum, this should probably either be done by introducing a new image tag series and deprecating the venv-less one until 3.0. |
First of all I do not thing this is backwards-incompatible, secondly - I do not really think this is a problem even if it was becuase airflow incompatibility has nothing to do with image incompatibility (especially that our image is not yet "official stable" image - it's a "reference" image). Why I think it is not incompatible? Because all the examples and recommendation we had about extending and customising the image, remain unchanged. The image, airlfow, providers and all the tools inside will continue to work if peopel were using all our examples and following them (and we have PLENTY of them). Even more - those examples are automatically validated during the CI build (except image customisation that I run separately every time I make significant change like this one - so I am pretty sure they are working fine. All our prod 'image tests" are also working fine with it (we test if all the imports work, if all providers are installed and are importable, etc. etc. From the user's point of view - who either customizes or extends the image - nothing changes. The only change is where the packages are installed. But if they use (as they should) Even if they manualy added This is really equivalent to refactoring code wihch is not "public" API in Python. The "location" of the instaled packages is not "public API". The 'pip' commands to manipulate those are the API (and those have not changed). Now why this would not be a big problem even if it was more "backwards-incompatible"? The This situation will change however (from my point of view at least - apparently Python maintainers have a different view on that) when we apply for the "official docker image status" - https://docs.docker.com/docker-hub/official_images/. Then I would be far more careful about similar changes. This is about the last two changes I am still hesitant about completing because there were a few open things (like the .venv). When I look at the rate of changes of the image it stabilized significantly. We handle all the cases we want to handle, the API to build those images was significantly simplified and more intuitive, we had far more issues raised by the users that my answer is "Yes - this is supported already by the image see the doc here" (for example when people want to build image in air-gaped environment or when they want to verify provenence of all the python packages, or when they want to add custom entrypoint etc. etc. ). I was building up the knowledge and documentation and I think I am rather close to say "yeah we are ready to get the official image status". By then as well I plan to extract a separate "read-only" repo where only relevant files will be present (I plan to use Plus we will get extra security checks and notifications as the "official" images by Docker get special treatment and got some automated scanning and notifications - and then we will likely also have to build a bit faster loop on rebuilding the images when security issues are discovered in base image. That's another topic to be discussed when we apply for the "official" status. Then such images will be available to pull as See the issues there: https://github.com/apache/airflow/projects/3 - not having "official" status is the only reason why AIP-26 is still "in-progress". |
349911f to
49df783
Compare
|
Just another commment here also @uranusjr (to clarify as I realized it was not clear). My "no broken compatibility" comment relates to "Image" changes, Not the "virtualenvoperator" changes. Currently they are in one PR (so that the tests are passing). But I am ok to split the virtualenv operato changes out and treat them differently. But I am open for discusion and arguments on that one. |
|
BTW. @uranusjr I really got to like the idea of using |
49df783 to
d31451f
Compare
|
Finally had time to return to that one. I will make it pass and then I will look at the remaining comments about PythonVirtualenv operator |
d5bdaae to
a23780a
Compare
|
@uranusjr - I removed the "activation" and left only the variables setting to emulate what |
|
I will take a look tomorrow, but any hints are welcome. |
|
After the whole discussion, I somehow have a feeling (without recalling back all the sentiments during the discussion), that the case that airflow should run in venv in the image is NOT the best idea if we are going to support venv creation by it. Let me know what you think @uranusjr - but if we come to that conclusion, I think we might have a good case where venv is not recommended (which might be good input to PEP 668 that I am going to come back to as well. |
a23780a to
8dd9238
Compare
Seems that the future direction of package installation for Python is to always use virtualenvs to install dependencies. There was a heated discussion about it in the issue here: pypa/pip#10556 and general consensus is that virtualenv building should also be used in Docker images as it can help to avoid multiple problems related to interference between distro-managed and PIP-managed files. This change implements it - both PROD and CI images are converted to use virtualenv for installation and instead of copying the `.local` directory between image segments they do the same with newly created `/.venv` virtual environment. All dependencies and shared libraries are installed there are and shared between all users using the images.
8dd9238 to
093be0c
Compare
| AIRFLOW_CONSTRAINTS_REFERENCE=${AIRFLOW_CONSTRAINTS_REFERENCE} \ | ||
| AIRFLOW_CONSTRAINTS_LOCATION=${AIRFLOW_CONSTRAINTS_LOCATION} \ | ||
| DEFAULT_CONSTRAINTS_BRANCH=${DEFAULT_CONSTRAINTS_BRANCH} \ | ||
| # Install Airflow in a virtual environment, so that we can copy the whole |
There was a problem hiding this comment.
What code is this comment referring to?
|
@uranusjr @mik-laj - I split the Dockerfile modiffcation from this one and #19210 into #20238. I do not think I am going to pursue the venv route actually (at least not shortly). The way how we could easily create venv from within an already active venv is at odds with how PythonVirtualenv works for Apache Airflow. I thought a bit on that and gave myself time to think about it and I think this case (Creating a venv dynamically from another venv) is where the venv approach cannot really work in the same way as if "system" installed application, so I am not going to make Airflow work this way. I will think if I want to come back at all to my suggestions to PEP 668. I gave myself time to think, and to be honest I found myself quite demotivated. I thought I've followed the advice on trying to contribute there, I've followed up, gave the context, presented my experience and willingness to clarify things, but I do not see any interest of the people who are involved there. Seems like clarifying soime of the "venv" statements and prefering to leave them in "vague" state without any user guidance, rather than clarifying cases where it might not be the best approach (for cases like ours for exampel). Which I think is not best approach, but then, who am I to judge. So I think I will simply ignore that PEP for now (which is still in draft) - let's see if it gets an approval and adoption. For now I will just add a comment in our build process that "our case is different and the warning from PIP is not relevant". |
PIP produces a warning when root user is used to run pip install. This is done for a good reason - because installing PIP this way clashes with a number of distro-managed python packages. The warning cannot be disabled even if our use case is legitimate as has been extensively discussed in pypa/pip#10556. However, the advice given by the warning is a bit misleading - it suggests to use virtualenv, but since this is considered a bad practice for container building and because we need to create virtualenvs dynamically inside the image, using virtualenv is a bad solution for us. It's been attempted in apache#19189 and failed. Instead we create an airflow user and use PIP_USER="true" which installs all dependencies in build segment to ~/.local folder from where we can copy it to the main image. That get rids of the warning and at the same time allows us to keep the best practices of building the images.
PIP produces a warning when root user is used to run pip install. This is done for a good reason - because installing PIP this way clashes with a number of distro-managed python packages. The warning cannot be disabled even if our use case is legitimate as has been extensively discussed in pypa/pip#10556. However, the advice given by the warning is a bit misleading - it suggests to use virtualenv, but since this is considered a bad practice for container building and because we need to create virtualenvs dynamically inside the image, using virtualenv is a bad solution for us. It's been attempted in apache#19189 and failed. Instead we create an airflow user and use PIP_USER="true" which installs all dependencies in build segment to ~/.local folder from where we can copy it to the main image. That get rids of the warning and at the same time allows us to keep the best practices of building the images.
PIP produces a warning when root user is used to run pip install. This is done for a good reason - because installing PIP this way clashes with a number of distro-managed python packages. The warning cannot be disabled even if our use case is legitimate as has been extensively discussed in pypa/pip#10556. However, the advice given by the warning is a bit misleading - it suggests to use virtualenv, but since this is considered a bad practice for container building and because we need to create virtualenvs dynamically inside the image, using virtualenv is a bad solution for us. It's been attempted in #19189 and failed. Instead we create an airflow user and use PIP_USER="true" which installs all dependencies in build segment to ~/.local folder from where we can copy it to the main image. That get rids of the warning and at the same time allows us to keep the best practices of building the images.
Using PIP as rot is not recommended, however the recommendation given by PIP in the warning message when someone usess PIP as root contains recommendeation that might be misleading. The message only mentiones recommendation of using virtualenv, however this recommendation does not apply to building containers. This has been extensively discussed with varying opinions in the pypa#10556. There are a number of users who use PIP to build containers who consider using virtualenv as an antipattern, however there are also PIP maintainers who claim that the warning message is ok, even if there were attempts to implement the recommended approach as documented in apache/airflow#19189 where attempts to follow the recommendation failed. This PR attempts to modify the message in a minimal way to still keep the original recommendation but also provides the container developers with more appropriate option of switching to another user and using --user flag instead.
PIP produces a warning when root user is used to run pip install. This is done for a good reason - because installing PIP this way clashes with a number of distro-managed python packages. The warning cannot be disabled even if our use case is legitimate as has been extensively discussed in pypa/pip#10556. However, the advice given by the warning is a bit misleading - it suggests to use virtualenv, but since this is considered a bad practice for container building and because we need to create virtualenvs dynamically inside the image, using virtualenv is a bad solution for us. It's been attempted in #19189 and failed. Instead we create an airflow user and use PIP_USER="true" which installs all dependencies in build segment to ~/.local folder from where we can copy it to the main image. That get rids of the warning and at the same time allows us to keep the best practices of building the images. (cherry picked from commit 3feb057)
PIP produces a warning when root user is used to run pip install. This is done for a good reason - because installing PIP this way clashes with a number of distro-managed python packages. The warning cannot be disabled even if our use case is legitimate as has been extensively discussed in pypa/pip#10556. However, the advice given by the warning is a bit misleading - it suggests to use virtualenv, but since this is considered a bad practice for container building and because we need to create virtualenvs dynamically inside the image, using virtualenv is a bad solution for us. It's been attempted in #19189 and failed. Instead we create an airflow user and use PIP_USER="true" which installs all dependencies in build segment to ~/.local folder from where we can copy it to the main image. That get rids of the warning and at the same time allows us to keep the best practices of building the images. (cherry picked from commit 3feb057)
3 participants
Seems that the future direction of package installation for Python
is to always use virtualenvs to install dependencies. There was a
heated discussion about it in the issue here:
pypa/pip#10556
and general consensus is that virtualenv building should also be
used in Docker images as it can help to avoid multiple
problems related to interference between distro-managed and
PIP-managed files.
This change implements it - both PROD and CI images are converted
to use virtualenv for installation and instead of copying
the
.localdirectory between image segments they do the samewith newly created
/.venvvirtual environment. All dependenciesand shared libraries are installed there are and shared between
all users using the images.
^ 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.