Repository navigation
Conversation
boring-cyborg
Bot
added
area:dev-tools
area:production-image
61e6fb4 to
4da71f4
Compare
Dev-iL
changed the title
4da71f4 to
7dab969
Compare
7dab969 to
1d231a5
Compare
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks for tackling this. Not installing the dependencies of providers that are excluded for the image's Python version is worth having. Details are inline; the main points are how requires-python is evaluated, that a failing probe silently falls back to the old path, and that the filtered install drifts from what uv sync --frozen does.
One bigger question before going further: could most of this be replaced by uv itself? Something like uv export --frozen --all-packages --group ci-image --prune <excluded-member> ... piped into uv pip install --require-hashes installs straight from the lock. There is no second resolver pass, no platform/marker drift, no separate hash handling, and no hand-maintained copy of uv's workspace/group semantics. I haven't tried it against our workspace, so there may be a reason it doesn't work. If so, it would be good to say why in the PR description.
Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting
|
@potiuk VERY good point on using uv's capabilities for this - the PR was redone accordingly, leading to savings of several hundreds of LOCs. |
1d231a5 to
3fe1d1b
Compare
b45b15d to
888721b
Compare
888721b to
78e28b5
Compare
github-actions
Bot
added
the
fork ci required
|
Project CI in use. A maintainer added the This comment was posted automatically by the fork CI monitor from templates drafted with an AI-assisted tool, so it may contain mistakes. An Apache Airflow maintainer, a real person, will review the pull request once its fork CI is green. Why the first pass is automated. |
github-actions
Bot
added
the
needs project ci
|
@potiuk Can you please see if your concerns have been addressed? |
Dev-iL
added
the
use project ci
78e28b5 to
5803087
Compare
github-actions
Bot
removed
needs project ci
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks for reworking this around uv export. All the points from my earlier review are addressed, and the filter is much simpler now. Two new problems need fixing before this can merge: source image builds now depend on the git-ignored generated/provider_dependencies.json, and the stale-lock fallback added for the earlier thread no longer triggers for a lock that is missing a new dependency. Also, PR CI only built 3.11 images, which don't take the new path, so the filtered install has never run in an image build. Please add the all versions label so CI builds 3.14 before this merges.
Source image builds now require the git-ignored provider_dependencies.json (scripts/docker/filter_ci_install_set.py:88)
generated/provider_dependencies.json is git-ignored (generated/.gitignore), and generated/README.md says it is generated "when needed by either breeze or pre-commit hooks". The probe now reads it unconditionally on every source install, for every Python version, so a build context without it fails with Could not determine excluded providers (exit 1). Builds that never needed the file before now break. Not every build path generates it. breeze ci-image build runs update_providers_dependencies.py only in the non-cache branch; the --prepare-buildx-cache branch (build_cache) skips it. breeze prod-image build never runs it, so a PROD build from sources (AIRFLOW_INSTALLATION_METHOD=.) on a clean checkout fails, and so does a plain docker build of either Dockerfile. The if [ -f generated/provider_dependencies.json ] guard in the dependency-manifests stage turns a missing file into this later failure instead of reporting it. Make sure the file exists wherever the image is built from sources: generate it in the cache and PROD build paths, or have the probe derive the exclusions from the provider.yaml files that are already in the context. Keep the loud failure if the metadata really can't be found, but name the missing file in the error.
Flagged by both the primary and adversarial reviewers.
The relock fallback no longer triggers for a lock that is missing a new dependency (scripts/docker/install_airflow_when_building_images.sh:207)
This comment and the allow-relock path (added for the earlier thread about AIRFLOW_FALLBACK_NO_CONSTRAINTS_INSTALLATION) assume the helper stops with " is not in the lock". The helper no longer resolves anything, and nothing in it emits that message. uv export --frozen exports the existing lock without checking it against the manifests, so a PR that adds a dependency to a member without updating uv.lock gets through prepare_filtered_install_set. The editable install then uses --no-deps, and uv pip check at line 148 fails outside the recovery block. The same PR still builds on 3.11-3.13. So the fallback the thread asked for is effectively dead for the case it was written for. Detect a stale lock before exporting, for example with uv lock --check, and route that failure into the relock branch. Also update the comment to describe what actually fails now.
Flagged by codex (adversarial review); cross-checked.
Smaller observations
- See inline comments on
scripts/docker/install_airflow_when_building_images.sh:45,scripts/ci/prek/update_airflow_pyproject_toml.py:290.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. After you've
addressed the points above and pushed an update, an Apache Airflow
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Airflow handles maintainer review:
Contributing guide.
|
|
||
| def read_excluded_distributions(root: Path, minor: str) -> set[str]: | ||
| """Return the distribution names of providers excluded for ``minor``.""" | ||
| metadata = json.loads((root / PROVIDER_DEPENDENCIES_PATH).read_text(encoding="utf-8")) |
There was a problem hiding this comment.
Source image builds now require the git-ignored provider_dependencies.json
generated/provider_dependencies.json is git-ignored (generated/.gitignore), and generated/README.md says it is generated "when needed by either breeze or pre-commit hooks". The probe now reads it unconditionally on every source install, for every Python version, so a build context without it fails with Could not determine excluded providers (exit 1). Builds that never needed the file before now break. Not every build path generates it. breeze ci-image build runs update_providers_dependencies.py only in the non-cache branch; the --prepare-buildx-cache branch (build_cache) skips it. breeze prod-image build never runs it, so a PROD build from sources (AIRFLOW_INSTALLATION_METHOD=.) on a clean checkout fails, and so does a plain docker build of either Dockerfile. The if [ -f generated/provider_dependencies.json ] guard in the dependency-manifests stage turns a missing file into this later failure instead of reporting it. Make sure the file exists wherever the image is built from sources: generate it in the cache and PROD build paths, or have the probe derive the exclusions from the provider.yaml files that are already in the context. Keep the loud failure if the metadata really can't be found, but name the missing file in the error.
Flagged by both the primary and adversarial reviewers.
There was a problem hiding this comment.
Agreed. Nothing in the image build reads or copies generated/provider_dependencies.json any more. The exclusions now come from each provider's git-tracked pyproject.toml, whose requires-python Breeze writes from excluded-python-versions. The helper parses it strictly: it accepts only the two clauses Breeze writes and stops the build on anything else. A plain docker build of the 3.14 PROD image from a fresh worktree, with no JSON, now builds through the filtered path.
Explanation
One correction on which paths broke. Breeze already regenerates the file: every Breeze command except --help calls generate_provider_dependencies_if_needed() at startup (dev/breeze/src/airflow_breeze/commands/main_command.py). That rewrites the file when it is missing, or when the provider manifests changed since it was written. With the file moved aside, breeze prod-image build --python 3.14 logged Regenerating provider dependencies file and built. What did break is a plain docker build of either Dockerfile, on a checkout with no copy of the file or with one older than the last provider change.
Breeze writes each provider's requires-python as >=3.11 plus one !=<entry>.* per excluded-python-versions entry (dev/breeze/src/airflow_breeze/utils/packages.py), and the update-providers-build-files hook keeps it in sync. The file is already in every build context, including the manifests-only stage, and parses with tomllib, so the probe still runs on a bare interpreter.
On the two routes you named:
- Generating the JSON: Breeze already does it, and the plain
docker buildwould still be broken. - Reading
provider.yaml: at that point of the build only pip and uv are installed, so there is no PyYAML to read it with.provider.yamlisn't in the manifests stage either.
This does mean requires-python is parsed again, which I said earlier wouldn't happen. This time the parsing is strict. The helper understands exactly >=<version> and !=<version>.*, evaluated as PEP 440 does against the interpreter's full release, so a patch entry such as !=3.13.1.* matches by prefix. Any other clause stops the build, and the error names the pyproject.toml and the clause. That covers <3.14, ~=, and !=3.15 without the wildcard, so the earlier blind spot can't come back silently. Only provider members are evaluated, so the root, airflow-core and task-sdk still can't drop out.
Tests
test_is_python_version_allowed_agrees_with_pep_440checks the evaluator againstpackaging.specifiers.test_is_python_version_allowed_rejects_clauses_breeze_does_not_writecovers the rejected clauses.test_split_members_names_the_manifest_and_clause_it_cannot_evaluateandtest_split_members_names_a_manifest_it_cannot_readcover the error messages.test_source_install_follows_the_real_helperruns the real helper under the installer with onlyuvstubbed, so the probe's exit codes and the helper's options can't drift apart from the shell.- The fixture holds only manifests, with no
generated/directory.
On the real workspace, the helper's exclusions for 3.11 to 3.15 match what the provider.yaml files declare: only apache-airflow-providers-apache-tinkerpop, only on 3.14. That holds both in the full tree and in a copy holding only the pyproject.toml files and uv.lock. The plain docker build of the 3.14 PROD image, from a fresh worktree with no generated/provider_dependencies.json (run before the latest rebase), logged:
Providers excluded for Python 3.14.10: apache-airflow-providers-apache-tinkerpop; kept members: 140
+ uv lock --check --no-python-downloads --no-managed-python
+ uv pip install --python /home/airflow/.local/bin/python --exact --no-deps ... --require-hashes ... --requirement /tmp/ci-install-set/requirements.txt
Resolved 777 packages
+ uv pip install --python /home/airflow/.local/bin/python --no-deps ... --editable /opt/airflow --editable /opt/airflow/airflow-core ...
Resolved 140 packages
+ uv pip check --python /home/airflow/.local/bin/python
Checked 917 packages
All installed packages are compatible
Drafted-by: Claude Code (Opus 5.5); reviewed by @Dev-iL before posting
| set +x | ||
| if has_providers_excluded_for_python; then | ||
| # Mirror the frozen path below: a lock that does not yet carry a new dependency makes the | ||
| # helper stop with "<name> is not in the lock", so re-lock and filter again when allowed. |
There was a problem hiding this comment.
The relock fallback no longer triggers for a lock that is missing a new dependency
This comment and the allow-relock path (added for the earlier thread about AIRFLOW_FALLBACK_NO_CONSTRAINTS_INSTALLATION) assume the helper stops with " is not in the lock". The helper no longer resolves anything, and nothing in it emits that message. uv export --frozen exports the existing lock without checking it against the manifests, so a PR that adds a dependency to a member without updating uv.lock gets through prepare_filtered_install_set. The editable install then uses --no-deps, and uv pip check at line 148 fails outside the recovery block. The same PR still builds on 3.11-3.13. So the fallback the thread asked for is effectively dead for the case it was written for. Detect a stale lock before exporting, for example with uv lock --check, and route that failure into the relock branch. Also update the comment to describe what actually fails now.
Flagged by codex (adversarial review); cross-checked.
There was a problem hiding this comment.
You're right: uv export --frozen exported a stale lock as it stood, and the failure only came at uv pip check. The frozen filtered path now runs uv lock --check before the export. A stale lock is re-locked when AIRFLOW_FALLBACK_NO_CONSTRAINTS_INSTALLATION=true and fails the build before any install otherwise. The comment now describes this check.
Explanation
I measured the check's exit codes: it exits 1 for a stale or missing lock, and 2 when it can't check at all, such as a malformed pyproject.toml.
- Exit 1, fallback allowed. It re-locks, then exports and installs.
- Exit 1, fallback not allowed. The build fails before anything is exported or installed, saying that
uv.lockdoesn't match the dependency manifests. - Any other non-zero. The build fails with that status before relocking, whatever the fallback setting, saying the lock could not be checked. A check that couldn't run doesn't read as a stale lock, the same rule as the probe's exit codes.
- Upgrade path. It re-locks just before, so it doesn't check.
On the real command, adding a dependency to a provider's pyproject.toml without relocking makes uv lock --check --offline exit 1 in about half a second. It exits 0 on the clean tree.
Tests
test_frozen_filtered_install_relocks_a_stale_lock_only_when_fallback_is_allowed covers five cases, asserting the command order, the exit status and the message:
- a fresh lock;
- a stale lock with fallback;
- a stale lock without fallback;
- a check that fails to run, with fallback and without.
With the check removed, all five fail. When any failing exit is treated as stale, the two cases where the check can't run fail.
test_source_install_selects_path_from_the_probe asserts that the frozen path asks for the check and the upgrade path, which has just re-locked, doesn't. Dropping allow-relock from the frozen call fails it and test_source_install_follows_the_real_helper, which asserts that a frozen build starts with uv lock --check.
Drafted-by: Claude Code (Opus 5.5); reviewed by @Dev-iL before posting
| . "$( dirname "${BASH_SOURCE[0]}" )/common.sh" | ||
|
|
||
| function has_providers_excluded_for_python() { | ||
| local python_minor="${AIRFLOW_PYTHON_VERSION%.*}" |
There was a problem hiding this comment.
Python minor taken from the build argument rather than the interpreter
${AIRFLOW_PYTHON_VERSION%.*} assumes a patch-level value. Breeze always passes one, but a direct docker build can leave the argument empty (allowed for the hardened flavor) or pass 3.14. That gives an empty string or 3, which matches no exclusion, so the build silently takes the unfiltered uv sync path. install_development_dependencies.py in this PR already uses the running interpreter (platform.python_version()). Let filter_ci_install_set.py default --python-minor to sys.version_info of the interpreter it runs under, which is the target interpreter here, and drop the derivation from the build argument in both places (lines 45 and 93).
Flagged by codex (adversarial review); cross-checked.
There was a problem hiding this comment.
Done. The helper now evaluates the interpreter running it, which is the image's own, and both derivations from AIRFLOW_PYTHON_VERSION are gone.
Explanation
- The helper defaults to
sys.version_info. - The option is now
--python-version, sincerequires-pythonis evaluated against the full release. Only the tests use it. - The helper logs the version it evaluated and the providers it excluded, so the build log shows which path was chosen and why.
Tests
test_main_defaults_to_the_running_interpretercovers the helper default.test_probe_leaves_the_python_version_to_the_interpreterruns the probe with the build argument unset, empty,3.14and3.14.2. It asserts that no version reaches the helper and that the path chosen doesn't change.
Drafted-by: Claude Code (Opus 5.5); reviewed by @Dev-iL before posting
| all_optional_dependencies.append(f'"{provider_id}" = [\n "{distribution_name}"\n]\n') | ||
| all_provider_lines.append(f' "{distribution_name}",\n') | ||
| all_optional_dependencies.append( | ||
| f'"{provider_id}" = [\n "{distribution_name}{exclusion_marker}"\n]\n' |
There was a problem hiding this comment.
Flag any changed or added behaviour without a corresponding test
Flag any changed or added behaviour without a corresponding test, and flag tests that a reviewer cannot fail by reverting the PR's change. The target is exactly 100% coverage of what the PR changes — no more, no less.
The exclusion marker is now applied to providers without a minimum version, but scripts/tests/ci/prek/test_update_airflow_pyproject_toml.py is unchanged. Reverting these lines keeps the suite green. Add a case for a provider with excluded-python-versions and no minimum version, and assert the marker appears in both the per-provider extra and the all list.
There was a problem hiding this comment.
Added. The new test builds the dependency entries for a provider that excludes 3.15 and has no minimum version, and checks that the marker rejects 3.15 and accepts 3.14.
To make it testable, the per-provider extra and the all line moved out of the __main__ block into build_provider_dependency_entries. Rerunning update-pyproject-toml leaves pyproject.toml unchanged.
test_build_provider_dependency_entries_marks_a_provider_without_a_minimum_version builds both entries for a provider with excluded-python-versions: ["3.15"] and no minimum version. Each entry is parsed as TOML and as a requirement, and the test asserts that the marker rejects 3.15 and accepts 3.14. Dropping the marker from the no-minimum branch fails the test.
Drafted-by: Claude Code (Opus 5.5); reviewed by @Dev-iL before posting
Dev-iL
added
the
all versions
bc71571 to
04e017e
Compare
Dev-iL
changed the title
04e017e to
1f3e9c6
Compare
|
Quickest fix: git fetch upstream main && git rebase upstream/main
rm uv.lock && uv lock
git add uv.lock && git rebase --continue
git push --force-with-leaseAutomated nudge — ignore if you're not ready to rebase. This comment is updated in place on future |
f6c723a to
546dd0f
Compare
uv sync --all-packages installs every workspace member, so a provider excluded for a Python version still installed its third-party dependencies into that version's CI image. Some of those dependencies have no wheel for a new interpreter and fail to build from source, which blocks the image even though the provider is not supported there. The exclusions come from the providers' own pyproject.toml files rather than from generated metadata: those files are tracked and present in every build context, including the stage that holds only the dependency manifests, whereas the generated metadata is git-ignored and only some build paths create it. They are evaluated for the interpreter that runs the build, because the Python version build argument may be empty or name only the minor version. A lock that no longer matches the manifests has to be caught before the lock is exported, since the export carries the lock as it stands and the missing dependency would only surface after the install, where nothing re-resolves it. The in-container reinstall of development dependencies had the same gap: it collected the devel dependencies of every provider, so excluded providers' dependencies were still built for the new interpreter. The CI image's locked third-party warm-up, which runs before the workspace sources are copied in, is skipped for such a Python version, because its plain uv sync would build the excluded providers' dependencies.
The generator added the python_version marker for an excluded provider only when the dependent declared a minimum version, so dependents without one kept an unconditional requirement on the excluded provider.
546dd0f to
314a802
Compare
related: #74555
Image builds from sources no longer install providers excluded for the image's Python version, along with the third-party packages only those providers need. An unsupported dependency of an excluded provider can no longer block the image for every other provider.
What changed: when a provider's
requires-pythonrejects the image's interpreter, the installer exportsuv.lockfor the workspace members that are kept and installs exactly that, with the lock's pins and hashes. No second resolution runs, so the install can't drift from the lock. The in-container reinstall of development dependencies skips excluded providers too.How it was shown working:
docker buildof the 3.14 PROD image from a clean checkout with nogenerated/metadata.Commands and output are under Verification.
Decisions:
pyproject.toml, not fromprovider.yamlor the git-ignoredgenerated/provider_dependencies.json. It is git-tracked and in every build context, and it parses with the standard library. Breeze regenerates the JSON at startup, but a plaindocker buildnever runs Breeze.requires-pythonevaluator understands only the clauses Breeze writes (>=<version>,!=<version>.*), with PEP 440 semantics, so a patch entry such as!=3.13.1.*matches by prefix. Anything else fails the build and names the file and clause.uv exportplus a hash-checkeduv pip install.uv sync --frozenwith one--packageper kept member would drop the same packages, but it would also install every locked version of a package pinned twice, without noticing (details below).Debt left:
apache.tinkerpop(3.14) is excluded anywhere.Before and after (real, current main,
breeze ci-image build --python 3.14):uv sync --frozen --all-packages. That installsapache-airflow-providers-apache-tinkerpopandgremlinpython, even though the provider declares it doesn't support 3.14. On a new interpreter, such a dependency without a wheel fails the whole image, asconfluent-kafkadoes on 3.15.Providers excluded for Python 3.14.10: apache-airflow-providers-apache-tinkerpop. The image holds the other 140 workspace members and none of the 4 packages only tinkerpop needs.On merge, the only CI images whose install path changes are 3.14, because tinkerpop is the only provider excluded for an interpreter CI builds (3.11 to 3.14). 3.11 to 3.13 keep
uv sync --frozen, and PROD images installed from PyPI are unaffected.How it works
scripts/docker/filter_ci_install_set.pyreads[tool.uv.workspace] membersand each member'spyproject.toml. A provider is excluded when itsrequires-pythonrejects the interpreter running the helper, which is the image's own. Breeze writes that field fromexcluded-python-versionsinprovider.yaml(dev/breeze/src/airflow_breeze/utils/packages.py), andupdate-providers-build-fileskeeps it in sync. Non-provider members are never evaluated, soapache-airflow,airflow-coreandtask-sdkcan't drop out.Probe.
--probeexits0when some provider is excluded and10when none is. Any other status fails the build instead of falling back to the unfiltereduv sync.Filtered install. With exclusions,
install_airflow_when_building_images.shwrites the kept member names and directories, then runs:uv export --frozenwith one--packageper kept member;uv pip install --exact --no-deps --require-hashesof the exported pins;uv pip check.--exactremoves packages an earlier cached layer installed. The PROD stage targets${VIRTUAL_ENV}.Stale lock. On the frozen path,
uv lock --checkruns before the export.AIRFLOW_FALLBACK_NO_CONSTRAINTS_INSTALLATION=true, and fails the build otherwise.Warm-up. The CI image's locked third-party warm-up, which runs before the sources are copied in, is skipped when providers are excluded. Its plain
uv syncwould install the excluded providers' dependencies.Generator fix. The
update-pyproject-tomlgenerator now adds the exclusion marker for providers without a minimum version too. That applies both in the provider's own extra and inall.Dockerfiles. Their copies of the scripts are generated by the existing inlining hook.
Why the hash-checked install matters for a lock that pins a package twice
Earlier in this PR's life,
uv.lockgavecassandra-driveran edge todeprecated1.3.1 on Python 3.14 and later, while every other dependent used 3.0.0 there. Two tools treat that differently:uv sync --frozendoesn't notice. Its dry run on 3.14 listed+ deprecated==1.3.1and+ deprecated==3.0.0, so the 3.14 image installed both, one over the other.The hash-checked install refuses it. The first local 3.14 build of this PR failed with:
This PR no longer carries a lock change: main's lock was re-resolved in the meantime (for example in #76334), and
cassandra-drivernow pinsdeprecated1.3.1 below 3.12 and 3.0.1 from 3.12. On current main,uv lock --checkwith uv 0.13.6, the image's version, passes, and the filtered 3.14 export installs underuv pip install --dry-run --exact --no-deps --require-hashes --no-configwith onedeprecated(3.0.1).Verification
Counts for Python 3.14 on Linux x86_64, from
uv export --frozen --group ci-image --no-emit-workspacewith markers evaluated for that interpreter (every pin counted, duplicates included):--all-packages)--packageper kept member)Dropped on 3.14:
apache-airflow-providers-apache-tinkerpop,aenum,async-timeout,gremlinpythonandnest-asyncio. No package is added and no pinned version changes.Exclusions on the real workspace match
provider.yamlfor every minor. This holds in the full tree and in a copy holding only thepyproject.tomlfiles anduv.lock, with nogenerated/:uv lock --checkexits 1 on a stale lock and 2 when it can't check:breeze ci-image build --python 3.14:In that image,
uv pip listshowsapache-airflow,apache-airflow-coreandapache-airflow-task-sdk. It shows onedeprecated(3.0.0, with a single dist-info), and none of tinkerpop,gremlinpython,aenum,async-timeoutornest-asyncio.A plain
docker buildofDockerfilefor 3.14 ran from a freshgit worktreeof the head, with nogenerated/provider_dependencies.json. It used the build argumentsbreeze prod-image build --python 3.14 --dry-runprints and installed into the PROD virtualenv:In that image,
airflow versionprints3.5.0,deprecatedis 3.0.0 with a single dist-info, and none of the tinkerpop-only packages is installed.breeze prod-image build --python 3.14takes the same path and gives the same result.Local checks on the pushed head:
Each commit's own tests (
scripts/tests/docker,scripts/tests/in_container/test_install_development_dependencies.pyandscripts/tests/ci/prek/test_update_airflow_pyproject_toml.py, where present) also pass at that commit.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5.5) following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.