Skip to content
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

Add lower bound to asgiref in providers #43001

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

rawwar
Copy link
Collaborator

@rawwar rawwar commented Oct 14, 2024

related to #42989

@rawwar
Copy link
Collaborator Author

rawwar commented Oct 14, 2024

Since, asgiref already has a lower bound in hatch_build.py, I tried adding them to all providers where asgiref was mentioned without any constraint. Seems like that did not help.

@rawwar
Copy link
Collaborator Author

rawwar commented Oct 14, 2024

@potiuk , Can you please check this PR? I added dependencies for all providers(with missing pins for asgiref) and yet its printing the warning.

Link to warning - https://github.com/apache/airflow/actions/runs/11325445901/job/31492421131?pr=43001#step:10:601

@potiuk
Copy link
Member

potiuk commented Oct 15, 2024

Yeah. that's expected and it should fix itself when we merge to main (you will be able to see it in canary builds):

This piece of output is:

  #48 4.743 Installing airflow from main. It is used to cache dependencies
  #48 4.743 
  #48 4.745 + curl -fsSL https://github.com/apache/airflow/archive/main.tar.gz
  #48 4.745 + tar xz -C /tmp/tmp.0VWy67lki1 --strip 1
  #48 6.344 + uv pip install --python /usr/local/bin/python --editable '/tmp/tmp.0VWy67lki1[devel-ci]'

If you look closely - it downloads airlfow from main and then uses it to install it locally. This is a "smart" way of caching - we are downloading the "main" version so that we can pre-install packages without invalidating docker image when dependencies of airflow change.

The way how docker layers work makes it difficult to cache dependencies from Python - because you first need to copy the dependency specifications (pyproject.toml, hatch_build.py, provider.yaml files) or airflow sources to the image in order to perform installation:

As an example (simplified):

1# COPY pyproject.toml src .
2# uv pip install .

The thing is that when you copy pyproject.toml the 1# layer gets invalidated (which also invalidates layer 2# - and it means that EVERY TIME pyproject.toml changes, we need to install whole airflow installation from the scratch (becuase the 2# layer has "installed airflow" and it gets invalidated.

There are various strategies to cope with it - most of them can use pip or uv cache, or using cache mounts: https://docs.docker.com/build/cache/optimize/#use-cache-mounts for local builds.

But airflow is a BEAST. The uv cache almost doubles the size of our image (2GB -> 4GB) because the uv cache is huge and is not optimized for size but for speed. The "cache mounts" only works for local builds and it takes ~6 minutes or so (less than pip but still substantial) to install airflow for the first time locally in the cache - also such local cache has some edge cases when it needs to be invalidated etc.

Instead we are using remote cache https://docs.docker.com/build/cache/optimize/#use-an-external-cache - basically our images, when they build locally use --cache-from) - and our CI builds and uploads cache to ghcr.io (with --cache-to).

This way the cache is refreshed every time main is green, and anyone who builds breeze image locally will use that cache.

And the "download main archive + install it" - will generally prepare a base installation. This layer is not invalidated for quite some time (usually it will be when a new python base image is released, or apt-dependencies are changed). But until then it provides a "base" cache - layer - and then it is not invalidated after pyproject.toml is added:

1# curl -fsSL https://github.com/apache/airflow/archive/main.tar.gz &&
  tar xz -C /tmp/tmp.0VWy67lki1 --strip 1 &&
  uv pip install --python /usr/local/bin/python --editable '/tmp/tmp.0VWy67lki1[devel-ci]'
2# COPY pyproject.toml src .
3# uv pip install --python /usr/local/bin/python --editable .

In this scenario:

  • The 1# layer gets refreshed every few weeks -> when python base image changes. It does not get invalidated when pyproject.toml or src changes. This layer is pulled (rather quickly comparing to installation) from ghcr.io when --cache-from is used during breeze ci-image build
  • The 2# layer gets invalidated when pyproject.toml or src change (also 3# is invalidated as it follows 2#. Then uv pip install already has most packages are installed already in 1# - so uv pip install generally will only incrementally install whatever changed in pyproject.toml (and hatch_build.py and provider.yaml in our case.

This means that in most cases rebuilding images is < 1 minute (and in some cases under 20 seconds) when sources or pyproject.toml changes. This saves enormous build time for CI and wait time for developers using breeze.

That's why currently this step installs still asgiref from main. But this will change once we merge this change to main (and we will again install things from main.

Now - I think the caching is currently slightly broken after the "providers" move (that's why you see it in the first place) - I am going to take a look at it shortly #42999 (generally you should not normally see "main" being run in the Dockerfile in your PR. It should come from the cache.

@potiuk
Copy link
Member

potiuk commented Oct 15, 2024

cc: @ashb -> detailed explanation about our CI image strategy ^^

@potiuk potiuk marked this pull request as ready for review October 15, 2024 01:28
@potiuk
Copy link
Member

potiuk commented Oct 15, 2024

I made it ready for review and merged (the mypy issue is fixed in main). You might take a look in a few ours @rawwar for Canary builds and see if the warning appears there https://github.com/apache/airflow/actions?query=event%3Aschedule

@potiuk potiuk merged commit c941484 into apache:main Oct 15, 2024
87 of 90 checks passed
@potiuk
Copy link
Member

potiuk commented Oct 15, 2024

Let's see :)

@rawwar
Copy link
Collaborator Author

rawwar commented Oct 15, 2024

Let's see :)

Locally, i just did a uv cache clean and it did remove asgiref warning. And, also cleaned 12GB of cache :D

R7L208 pushed a commit to R7L208/airflow that referenced this pull request Oct 17, 2024
harjeevanmaan pushed a commit to harjeevanmaan/airflow that referenced this pull request Oct 23, 2024
PaulKobow7536 pushed a commit to PaulKobow7536/airflow that referenced this pull request Oct 24, 2024
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants