You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "V0lantis (via GitHub)" <gi...@apache.org> on 2023/10/18 15:33:48 UTC
[PR] feat(dockerfile): Add pip caching for faster build [airflow]
V0lantis opened a new pull request, #35026:
URL: https://github.com/apache/airflow/pull/35026
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
## Pip caching
For our corporation, we are doing a custom build. I noticed that every time, docker was downloading again and again the python packages and I thought it was a little bit too slow. I might want to enlarge this usage of pip caching for our CI as well.
wdyt?
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "V0lantis (via GitHub)" <gi...@apache.org>.
V0lantis commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1786974465
Hey !
I came up with a better solution in 65318caa011f995555d05a6a04f1b51bbe631ae1:
I am passing pip cache directory directly to [an env variable](https://github.com/apache/airflow/blob/65318caa011f995555d05a6a04f1b51bbe631ae1/Dockerfile#L1205) (`PIP_CACHE_DIR=/tmp/.cache/pip`). This allow to avoid passing the following arg: `--cache-dir=/tmp/.cache/pip` to every `pip install` command : I could forget one and loose the potential of caching whereas with the globa `ENV` definition, no forgetting 😲 . I tested it and it seems to be working fine.
Wdyt guys?
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1775417593
> Also re- architecture - I think it is not **really** needed. Building prod image on a different architecture is unllikely to happen on the same machine (building the image on ARM for X86 is ~ 16 times slower with emulation so it takes literally hours).
On the other hand: I read this one: https://github.com/moby/buildkit/issues/2598 , and yeah we should leave it in since TARGETARCH ARG is conveniently set by default, so it's perfectly OK to leave it in
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785607908
BTW. Some of the hadolint messages (such as avoid using TARGETARCH should be ignored - but it should be explicit comment in the Dockerfile - we have good reasons why we are using it and we should explain it in the comment.
(We already have a few of those)
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "V0lantis (via GitHub)" <gi...@apache.org>.
V0lantis commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1774724955
Thank you @potiuk for this detailed comment on how the Dockerfile is built. I came up with the following (d952c9c5b1):
To invalidate cache between two different python versions, I am using the `id` param from `--mount=type=cache` [argument](https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#run---mounttypecache). This allow to specify the version of python and the architecture on which it is built. I hope this will solve the issue of cached invalidation you mentioned above.
Result :
```jsx
=> [airflow-build-image 15/16] RUN --mount=type=cache,[id=dockerhub.containers.mpi-internal.com/python:3.11-slim-bullseye-arm64,target=/home/airflow/.cache/pip,uid=50000](http://id%3Ddockerhub.containers.mpi-internal.com/python:3.11-slim-bullseye-arm64,target=/home/airflow/.cache/pip,uid=50000) if [[ false == "true" ]]; then bash /scr 7.7s
```
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1786322251
FYI @V0lantis -> you found a bug in build-ci-image with timeout: https://github.com/apache/airflow/pull/35282 . This is why "Build CI image" did not report the problem - only the next step did.
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785596842
Actually @V0lantis -> there are a few problems that our static-checks detected - so you will need to fix them before merging - some of them is just `running` pre-commit (and it will fix them for you) but some of them a bit more involved - hadolint with we use to lint the dockerfile for example complains now that we are using the same cache for both segments which is - apparently - bad practice (and I can understand why - because both segments can be buidl in parallel and override each-other's cache. So I am afraid it won't make it to 2.7.3 (so you will have to modify the changelog to point to 2.8.0) and we will merge it later.
Changing the milestone back to 2.8.0
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785576790
> cc: @ephraimbuddy -> will it make it still to 2.7.3 if I merge it (just local caching improvement when building custom images)
Yes, it will make it as I have decided to wait for the providers. Marking it with the milestone...
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #35026:
URL: https://github.com/apache/airflow/pull/35026
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "V0lantis (via GitHub)" <gi...@apache.org>.
V0lantis commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1786831270
> FYI @V0lantis -> you found a bug in build-ci-image with timeout: #35282 . This is why "Build CI image" did not report the problem - only the next step did.
Nice catch !
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "V0lantis (via GitHub)" <gi...@apache.org>.
V0lantis commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1770403457
> I would love to learn more about your use case and see how and whether it can help at all.
The goal here is to try every possible way to improve the build time of Airflow. We are using custom building and in our CI, the build doesnt use the cached layers 😠So we have an average build time of 8minutes. This is not much but a lot of people are working on our repository and we would like to improve their developer experience as a whole.
> I am not sure if it changes anything and it adds some assumptions that the image will be built several times on the same machine while also for some reason it will not use the already cached layers.
Yes, in our CI, the layers are not cached even we are using the buildkit experimental feature :
```
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3
with:
version: latest
driver-opts: image=moby/buildkit:latest
buildkitd-flags: --debug
```
and in our `build` command:
```sh
docker buildx build .... \
--cache-to=type=gha,scope=platform-$ARCH,mode=max \
--cache-from=type=gha,scope=platform-$ARCH"
```
> Could you please show some usage where it actually helps and improve things ? Some benchmarks showing what you've done and before/after would be good.
Ok, so it's not a proper benchmark, I just used the `time` bash method, but it can give you an idea
### **Without** mounting cache:
```bash
$ time docker build \
--build-arg PYTHON_BASE_IMAGE=python:3.11-slim-bullseye \
--build-arg AIRFLOW_VERSION=2.6.3 \
--build-arg AIRFLOW_HOME=/usr/local/airflow \
--build-arg AIRFLOW_EXTRAS=async,celery,redis,google_auth,hashicorp,statsd \
--build-arg 'ADDITIONAL_PYTHON_DEPS=apache-airflow-providers-amazon==8.3.1 apache-airflow-providers-celery==3.2.1 apache-airflow-providers-common-sql==1.6.0 apache-airflow-providers-cncf-kubernetes==7.4.2 apache-airflow-providers-datadog==3.3.1 apache-airflow-providers-docker==3.7.1 apache-airflow-providers-ftp==3.4.2 apache-airflow-providers-github==2.3.1 apache-airflow-providers-google==10.3.0 apache-airflow-providers-http==4.4.2 apache-airflow-providers-imap==3.2.2 apache-airflow-providers-mysql==5.1.1 apache-airflow-providers-postgres==5.5.2 apache-airflow-providers-sftp==4.3.1 apache-airflow-providers-slack==7.3.1 apache-airflow-providers-ssh==3.7.1 apache-airflow-providers-tableau==4.2.1 apache-airflow-providers-zendesk==4.3.1 apache-airflow-providers-salesforce==5.4.1' \
--build-arg 'ADDITIONAL_RUNTIME_APT_DEPS=groff gettext git' \
--build-arg AIRFLOW_CONSTRAINTS_LOCATION=/docker-context-files/constraints-airflow.txt \
--build-arg DOCKER_CONTEXT_FILES=docker-context-files \
--build-arg INSTALL_MYSQL_CLIENT=true \
--build-arg INSTALL_MSSQL_CLIENT=false \
--build-arg ADDITIONAL_DEV_APT_DEPS=pkgconf \
--no-cache
33.74s user 15.55s system 11% cpu 7:01.16 total
```
In the above, all pip requirements have been downloaded before being installed. In my case, I don't have at the moment a good internet connection, so it is even more slower than someone with a good internet connection
### **WITH** mounting cache:
```bash
$ time docker build
...
--no-cache
17.52s user 7.62s system 7% cpu 5:16.20 total
```
The `--no-cache` method argument above doesn't influence the mounted pip cache.
> Quite for sure caching when installing `pip` (first part of your change) has no effect.
Sure, mistake from my side, I removed the line in 77c6813
> This is something that is only done once to upgrade to latest version and then (regardless of caching) `pip` does not get re-installed again, because it is already in the target version. And (unless you have some other scenario in mind) re-using cache will only actually work when you use the same docker engine. And in this case - unless you change your docker file parameters, it is already handled by docker layer caching. I.e. if you are building your docker image twice in the same docker engine with the same parameters, then caching is done at the "container layer" level. So when you run your build again, on the same docker engine you should not see it trying to install Python again. You should instead see that CACHED layer is used. And this is all without having to mount cache volume.
>
> So I wonder why you would like to cache pip upgrade with cache.
>
> The second case might be a bit more interesting - but there also you will see reinstallation only happening if you change requirements.txt. And yes - in this case it will be a bit faster when you just add one requirement, but this will also work for the same user when rebuilding the image over and over again on the same machine, which is generally not something that you should use airflow's dockerfile for (usually) - usually this can be done by extending the image (`FROM apache/airflow`) and adding lines in your own Dockerfile (where you could use indeed --mount-type cache etc.).
>
> I wonder if you could describe your case in more detail and show some benchmark example of how much time is saved for particular scenarios.
>
> I am not totally against it, but I would love to understand your case, because a) maybe you do not gain as much as you think or b) maybe you are doing somethign that prevents you from using docker layer caching or c) maybe you are using the Dockerfile of Airlfow in unintended way.
>
> Looking forward for more detailed explanation of your case.
>
> So caching `pip` install makes no sense at all.
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1770882181
Looking at the discussion @V0lantis @hussein-awala and thinking a bit - yeah, I think it's actually quite fine to use the local cache (even for pip installation) as long as it does not interfere with any of the caching mechanisms of ours and ability to build different images (especially for multiple python versions).
As long as the remote cache works as it did (it should), aadding those local caches should not impact the build. And possibly can speed up some local build cases.
I have one worry though. Knowing that "cache invalidation is the most difficult thing". I am not 100% sure how it will work - how does the local cache gets allocated when you have differnet arguments passed. Do you know how the local cache is determined in case we change parameters/arguments and use the same Dockerfile?
To be honest I am quite worried about the case where base python changes (see my previous comment), about this case:
```
docker build --build-arg PYTHON_BASE_IMAGE=python:3.11-slim-bullseye ... Dockerfile
```
Then (note Python version change):
```
docker build --build-arg PYTHON_BASE_IMAGE=python:3.10-slim-bullseye ... Dockerfile
```
And about the case where `pip` version changes. Will the `pip` cache be usable in this case? Will they interfere? Do we have to have mechanism to invalidate the cache in this case?
I am asking all those questions, because if any problem like that happens, someone who will have it, will inevitably open isssue in our repo and will (rightfully) complain and ask us what to do.
I do recall cases in the past that `pip` installation broke when the same cache has been used for different python version s - including different Python patchlevel versions. I recall CI failure after Python upgrade from 3.8.x to 3.8.(x +1). That was of course extreme case, but this might happen. I also recall there were some issues when new releases of `pip` introduced some breaking changes or caused irrecoverable errors when cache gets broken - example case here https://github.com/pypa/pip/issues/11985 . Even very recently `pip` has changed the format of their cache storage - see 23.3 release notes (https://pip.pypa.io/en/stable/news/). This new format is kept in a different directory - so it should be safe to upgrade, but still that makes me a little worried.
The main reason why we have `--no-cache` there is that I did not want to to worry about all those cases and scenarios.
Curious about your thoughts on whether this is something we should worry about ? Maybe cache location be dependent on Python version and PIP_VERSION to mitigate those simply ? Maybe we should have a way to clear the cache when things get broken (other than nuking whole docker installation). And finally possibly we should document it somewhere what to do when it gets broken.
WDYT?
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785481276
cc: @ephraimbuddy -> will it make it still to 2.7.3 if I merge it (just local caching improvement when building custom images)
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1787739107
Yep.
> Hey ! I came up with a better solution in [65318ca](https://github.com/apache/airflow/commit/65318caa011f995555d05a6a04f1b51bbe631ae1):
>
> I am passing pip cache directory directly to [an env variable](https://github.com/apache/airflow/blob/65318caa011f995555d05a6a04f1b51bbe631ae1/Dockerfile#L1205) (`PIP_CACHE_DIR=/tmp/.cache/pip`). This allow to avoid passing the following arg: `--cache-dir=/tmp/.cache/pip` to every `pip install` command : I could forget one and loose the potential of caching whereas with the globa `ENV` definition, no forgetting 😲 . I tested it and it seems to be working fine.
>
> Wdyt guys?
Yep. Good idea.
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "V0lantis (via GitHub)" <gi...@apache.org>.
V0lantis commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785708077
Ok, will fix this, thanks
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "V0lantis (via GitHub)" <gi...@apache.org>.
V0lantis commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785355018
> On the other hand: I read this one: [moby/buildkit#2598](https://github.com/moby/buildkit/issues/2598) , and yeah we should leave it in since TARGETARCH ARG is conveniently set by default, so it's perfectly OK to leave it in
Good. Thanks for the hint
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "V0lantis (via GitHub)" <gi...@apache.org>.
V0lantis commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785817711
> I guess similar changes should then be applied to the CI image too. Might be benefitial as well in some cases (same scripts are used in both).
Yes, but I don't understand what makes the CI fail right now.
```
docker: Error response from daemon: No such image: ghcr.io/apache/airflow/main/ci/python3.8:4db4408536c0cb4d6dae098cb0fcba574887e7cf.
```
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785847445
> > I guess similar changes should then be applied to the CI image too. Might be benefitial as well in some cases (same scripts are used in both).
>
> Yes, but I don't understand what makes the CI fail right now.
>
> ```
> docker: Error response from daemon: No such image: ghcr.io/apache/airflow/main/ci/python3.8:4db4408536c0cb4d6dae098cb0fcba574887e7cf.
> ```
Unfold previous step and see the output of docker build comand - it fails to build. Possibly there is an error somewhere there because (independent from that) in the previous step because it's the previous step that should fail.
Try it locally `breeze ci-image build` locally to reproduce the failure.
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1775031757
Also re- architecture - I think it is not **really** needed. Building prod image on a different architecture is unllikely to happen on the same machine (building the image on ARM for X86 is ~ 16 times slower with emulation so it takes literally hours).
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1770749079
> Also, I don't have the feeling that your [CI is using cache layers](https://github.com/apache/airflow/actions/runs/6572143194/job/17852736374#step:6:697) even though [it should?](https://github.com/apache/airflow/blob/5c2dc53bcb17ae515f9565c41d15cc6d1693382c/dev/breeze/src/airflow_breeze/utils/docker_command_utils.py#L445). As @hussein-awala said, once buildkit will have implemented a way to use `binded cache` in CI, it would also help to improve build speed
It's because you were extremely unlucky when checking it. Python **just** released a new base image 4 hours ago, and our cache will refresh with the new base image after we merge something to main and the build in `main` succeeds:
The way how we are using remote caching is specifically designed to automatically handle such cache refreshes. The whole CI of ourse is designed to automatically upgrade to latest versions of the base images, apt dependencies and Python dependencies as soon as:
* they are released
* all our tests pass in main (also called `canary`) run
This allows us to automtically upgrade and test our software and test suite to "latest", "most secure" and "consistent" set of dependencies (including base dependencies for the image), while our PRs keeps on using the "stable and confirmed to work in the past" set of python dependencies.
This way we can detect breaking changes in incoming dependencies. We detected many breaking changes this way "early" - hence `canary` name (including even couple of breaking changes in Python patchlevel releases)
Indeed the build you see does not use the cache that corresponds to the latest pyhon version that's why it takes 14 minutes. Because cache is not yet refreshed. But it still works (which is great) while the cache is being rebuilt and refreshed.
You can see the same job from 5 hours ago https://github.com/apache/airflow/actions/runs/6565914251/job/17835531939#step:6:1070 which lasted 1 minute and actually used the cache.
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1772444407
> Refactoring the dockerfile to only COPY requirements first and frequently changed sources later is another good optimization technique. But i haven't looked at the dockerfile in detail to see if that is already done.
There were likely 10s iterations or so of making the right sequence of COPY/RUN there and also - in the main stage the number of layers is minimised. But maybe some of it could be improved still - it's open to contributions :).
The whole Dockerfile is now designed with the "incremental rebuild" in mind - it does selectively COPY only those things that are supposed to be used in the next step. I think what `--mount-cache` solves is that you really iterate over requirements.txt only. Similarly apt cache could be used (and is only useful) when someone experiments and iterates over apt dependencies
Actually final `main` segment is really small and simple because it is optimized for production use - i.e. size. So what the final segment does (except ARG/ENV setting) literally three steps:
* installing OS dependencies (runtime only - no build-essentials and dev libraries needed for compilation)
* installling DB clients (parameterized with ARGs - only those that are needed)
* copying installed airflow from `build` segment (we never install in the main segment - we always copy the installation from the build segment, this saves ~200 MB
I believe we could add `--mount-cache` for all those steps - providing that we account for those cache validity problems.
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1775028567
> Thank you @potiuk for this detailed comment on how the Dockerfile is built. I came up with the following ([d952c9c](https://github.com/apache/airflow/commit/d952c9c5b1464087ad8e0bf510b84a71bcb49afc)):
>
> To invalidate cache between two different python versions, I am using the `id` param from `--mount=type=cache` [argument](https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#run---mounttypecache). This allow to specify the version of python and the architecture on which it is built. I hope this will solve the issue of cached invalidation you mentioned above.
>
> Result :
>
> ```js
> => [airflow-build-image 15/16] RUN --mount=type=cache,[id=dockerhub.containers.mpi-internal.com/python:3.11-slim-bullseye-arm64,target=/home/airflow/.cache/pip,uid=50000](http://id%3Ddockerhub.containers.mpi-internal.com/python:3.11-slim-bullseye-arm64,target=/home/airflow/.cache/pip,uid=50000) if [[ false == "true" ]]; then bash /scr 7.7s
> ```
Yep. I like this approach a lot.
If we make it a little more elaborated, I am perfectly ok with it and solves all my caching problems. There are two things I would like to add:
* We should also add AIRFLOW_PIP_VERSION ARG part of the ID
* Also similarly to https://github.com/apache/airflow/blob/e19ce433be66015aaa87b566d94a66dfb360260f/Dockerfile.ci#L1292 we can add (for example) "PIP_CACHE_EPOCH" arg to be part of the Dockerfile and also make it part of the ID - this way we will be able to invalidate all caches with simply bumping the PIP_CACHE_EPOCH default value or passing a new value with --build-arg
And of course we need to add documentation to https://github.com/apache/airflow/blob/main/docs/docker-stack/build.rst and https://github.com/apache/airflow/blob/main/docs/docker-stack/build-arg-ref.rst and release notes here: https://github.com/apache/airflow/blob/main/docs/docker-stack/changelog.rst (targetting it for 2.7.3)
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "V0lantis (via GitHub)" <gi...@apache.org>.
V0lantis commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1770442529
Also, I don't have the feeling that your [CI is using cache layers](https://github.com/apache/airflow/actions/runs/6572143194/job/17852736374#step:6:697) even though [it should?](https://github.com/apache/airflow/blob/5c2dc53bcb17ae515f9565c41d15cc6d1693382c/dev/breeze/src/airflow_breeze/utils/docker_command_utils.py#L445). As @hussein-awala said, once buildkit will have implemented a way to use `binded cache` in CI, it would also help to improve build speed
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "V0lantis (via GitHub)" <gi...@apache.org>.
V0lantis commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785424482
I updated the documentation in b14f42e784 but I am not very familiar with changelog updates. Would you be so kind to show me the way?
Thank so much otherwise for all your hard work and hints @potiuk 💪
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785809770
> I am afraid I had to change the `dockerfile.ci` image as well which might make the CI fail 😢
I guess similar changes should then be applied to the CI image too. Might be benefitial as well in some cases (same scripts are used in both).
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1769500355
> This might be useful to someone playing with the requirements file to resolve a conflict, so if there is no potential risk from this change I would be OK with merging it.
Yep I think this one is not a bad idea if there are no side-effects. I would still love to see the scenarios and ways how it is used.
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1772102486
> The second case might be a bit more interesting - but there also you will see reinstallation only happening if you change requirements.txt.
Or changes to content used in any of the layers **above** pip install. It seems like it could be quite many, eg `COPY scripts`, `COPY ${AIRFLOW_SOURCES_FROM} `happen above the pip install.
Refactoring the dockerfile to only COPY requirements first and frequently changed sources later is another good optimization technique. But i haven't looked at the dockerfile in detail to see if that is already done.
> I have one worry though. Knowing that "cache invalidation is the most difficult thing". I am not 100% sure how it will work - how does the local cache gets allocated when you have differnet arguments passed. Do you know how the local cache is determined in case we change parameters/arguments and use the same Dockerfile?
>
> To be honest I am quite worried about the case where base python changes (see my previous comment), about this case:
>
> ```
> docker build --build-arg PYTHON_BASE_IMAGE=python:3.11-slim-bullseye ... Dockerfile
> ```
>
> Then (note Python version change):
>
> ```
> docker build --build-arg PYTHON_BASE_IMAGE=python:3.10-slim-bullseye ... Dockerfile
> ```
>
YES! Good observation, this is a very possible issue. When doing similar caching with `apt install`, it's very easy to mix the base image Ubuntu version across caches and end up in bad situations.
To solve this, we usually follow convention of suffixing the version in the cache key, eg for Ubuntu 22.04:
`RUN --mount=type=cache,sharing=locked,target=/var/cache/apt,id=aptcache2204 --mount=type=cache,sharing=locked,target=/var/lib/apt,id=aptlib2204 \`
You can probably do this with variables to not forget it everywhere.
For pip, i don't know if cache is reusable across python versions.
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1772488509
Just to complete it - explaining the `build` segment.
The `airflow-build-image` segment is slightly more complicated: - it allows for more sophisticated caching (used during our CI build) but essentially it does this:
* Installing OS dependencies (dev) = build-essentials + dev libraries
````
RUN bash /scripts/docker/install_os_dependencies.sh dev
````
* Installing database clients (parameterized with args)
````
RUN bash /scripts/docker/install_mysql.sh dev && \
bash /scripts/docker/install_mssql.sh dev && \
bash /scripts/docker/install_postgres.sh dev
````
* Branch Tip caching - (this is skipped by default in regular builds) - pre-installing `pip` dependencies from the tip of the branch (used in CI only (this is a "poor man's" version of incremental caching for CI (so what @hussein-awala mentioned as something might be implemented via https://github.com/moby/buildkit/issues/1512 ). This allows in our CI to use Docker caching layer instead of local cache for incremental builds when new requirements are added.
This layer is not invalidated when setup.py/requirements change, so we use it as to pre-install the dependencies in main, so that if someone adds new dependency, it will not reinstall everything from scratch.
```
RUN bash /scripts/docker/install_pip_version.sh; \
if [[ ${AIRFLOW_PRE_CACHED_PIP_PACKAGES} == "true" && \
${INSTALL_PACKAGES_FROM_CONTEXT} == "false" && \
${UPGRADE_TO_NEWER_DEPENDENCIES} == "false" ]]; then \
bash /scripts/docker/install_airflow_dependencies_from_branch_tip.sh; \
fi
```
* Installing airflow and packages (depends on args - it will install them either from `pypi` or from paackages in docker-context files and alows to also specify ADDITIONAL_PYTHON_DEPS
```
RUN if [[ ${INSTALL_PACKAGES_FROM_CONTEXT} == "true" ]]; then \
bash /scripts/docker/install_from_docker_context_files.sh; \
fi; \
if ! airflow version 2>/dev/null >/dev/null; then \
bash /scripts/docker/install_airflow.sh; \
fi; \
if [[ -n "${ADDITIONAL_PYTHON_DEPS}" ]]; then \
bash /scripts/docker/install_additional_dependencies.sh; \
fi; \
find "${AIRFLOW_USER_HOME_DIR}/.local/" -name '*.pyc' -print0 | xargs -0 rm -f || true ; \
find "${AIRFLOW_USER_HOME_DIR}/.local/" -type d -name '__pycache__' -print0 | xargs -0 rm -rf || true ; \
# make sure that all directories and files in .local are also group accessible
find "${AIRFLOW_USER_HOME_DIR}/.local" -executable -print0 | xargs --null chmod g+x; \
find "${AIRFLOW_USER_HOME_DIR}/.local" -print0 | xargs --null chmod g+rw
```
* Installing from requirements.txt placed in docker-context-files
```
RUN if [[ -f /docker-context-files/requirements.txt ]]; then \
pip install --no-cache-dir --user -r /docker-context-files/requirements.txt; \
fi
```
So any "local" caching of python installation should be done here @V0lantis - if you want to pursue it and attempt to do it in the way that we avoid the potential cache invalidation issues I mentioned (we could also add apt-caching as suggested by @hterik - those adding/iterating with new `apt` dependencies is less likely.
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1769427274
> but there also you will see reinstallation only happening if you change requirements.txt. And yes - in this case it will be a bit faster when you just add one requirement, but this will also work for the same user when rebuilding the image over and over again on the same machine
I agree, it will be useful in the CI when https://github.com/moby/buildkit/issues/1512 will be implemented, in that case we can combine it with Github cache to persist the mounted cache. This could work now if we replace `--mount=type=cache,target=<path>` by `--mount=type=bind,source=<path>,target=<path>`, but it will impact the users who don't have a cache folder in their host.
> I am not totally against it, but I would love to understand your case, because a) maybe you do not gain as much as you think or b) maybe you are doing somethign that prevents you from using docker layer caching or c) maybe you are using the Dockerfile of Airlfow in unintended way.
This might be useful to someone playing with the requirements file to resolve a conflict, so if there is no potential risk from this change I would be OK with merging it.
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1787780544
Congrats :tada:
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "V0lantis (via GitHub)" <gi...@apache.org>.
V0lantis commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785782062
I am afraid I had to change the `dockerfile.ci` image as well which might make the CI fail 😢
--
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
Re: [PR] feat(dockerfile): Add pip caching for faster build [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35026:
URL: https://github.com/apache/airflow/pull/35026#issuecomment-1785599276
Or ratheer `2.7.4` - > not sure if we will have one, but if we do - the changelog should Assume we will. (in case we don't - we will change it to 2.8.0 when preparing the release.
--
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