potiuk commented on code in PR #74556: URL: https://github.com/apache/airflow/pull/74556#discussion_r4510642024
########## scripts/docker/filter_ci_install_set.py: ########## @@ -0,0 +1,146 @@ +#!/usr/bin/env python +# 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. +""" +Select the workspace members the CI image installs for one Python minor. + +``uv sync --all-packages`` installs every workspace member, including providers excluded for this +Python minor, and the third-party packages only those providers reach - some of which have no wheels +for the new interpreter. Listing the members that are kept lets the installer export the lock for +exactly those: ``uv export --package <member>`` emits the lock's own closure, already carrying the +hashes and the environment markers that encode each provider's exclusions, so the install filters +itself on the target interpreter. No second resolution runs, so nothing can drift from the lock. + +Exclusions are read from ``generated/provider_dependencies.json``, which the provider tooling +regenerates from each ``provider.yaml``. Only providers appear there, so a core distribution can +never drop out of the install set by accident. +""" + +from __future__ import annotations + +import argparse +import json +import sys +import tomllib +from glob import glob +from pathlib import Path + +PROVIDER_DEPENDENCIES_PATH = Path("generated") / "provider_dependencies.json" +GLOB_CHARACTERS = frozenset("*?[") +# Exit codes the shell probe distinguishes: anything else means the probe itself failed. +EXIT_HAS_EXCLUDED_MEMBERS = 0 +EXIT_NO_EXCLUDED_MEMBERS = 10 + + +def has_glob_characters(pattern: str) -> bool: + return any(character in GLOB_CHARACTERS for character in pattern) + + +def find_workspace_members(root: Path, patterns: list[str]) -> list[Path]: + """Expand the ``[tool.uv.workspace] members`` patterns into directories holding a manifest.""" + members: list[Path] = [] + for pattern in patterns: + if has_glob_characters(pattern): + matches = [Path(match) for match in glob(str(root / pattern))] + members.extend(match for match in matches if (match / "pyproject.toml").is_file()) + continue + member = root / pattern + if not member.is_dir(): + raise ValueError(f"Workspace member {pattern!r} does not exist under {root}") + if not (member / "pyproject.toml").is_file(): + raise ValueError(f"Workspace member {pattern!r} has no pyproject.toml") + members.append(member) + return sorted(set(members)) + + +def load_manifest(directory: Path) -> dict: + return tomllib.loads((directory / "pyproject.toml").read_text(encoding="utf-8")) + + +def read_distribution_name(directory: Path) -> str: + name = load_manifest(directory).get("project", {}).get("name") + if not name: + raise ValueError(f"No project.name in {directory / 'pyproject.toml'}") + return name + + +def build_provider_distribution_name(provider_id: str) -> str: + """Turn a provider id from the generated metadata (``apache.kafka``) into its distribution name.""" + return "apache-airflow-providers-" + provider_id.replace(".", "-") + + +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")) Review 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.* ########## scripts/docker/install_airflow_when_building_images.sh: ########## @@ -33,13 +33,122 @@ # INSTALL_LOCKED_THIRD_PARTY_ONLY - if "true", only the third-party dependencies locked in uv.lock are installed # from sources (--no-install-workspace), so only the pyproject.toml files # and uv.lock are needed. When a frozen dry run shows that uv.lock does not -# match them, or for upgrade builds, nothing is installed; any other -# failure fails. The steps that need the workspace packages are skipped. +# match them, for upgrade builds, or when providers are excluded for the +# image Python version, nothing is installed; any other failure fails. +# The steps that need the workspace packages are skipped. # # shellcheck shell=bash disable=SC2086 # shellcheck source=scripts/docker/common.sh . "$( dirname "${BASH_SOURCE[0]}" )/common.sh" +function has_providers_excluded_for_python() { + local python_minor="${AIRFLOW_PYTHON_VERSION%.*}" Review 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.* ########## scripts/ci/prek/update_airflow_pyproject_toml.py: ########## @@ -286,8 +286,10 @@ def get_exclusion_marker(provider_dependencies: dict[str, Any]) -> str: f'"{provider_id}" = [\n "{distribution_name}>={min_provider_version}{exclusion_marker}"{comment}\n]\n' ) else: - 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' Review 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. ########## scripts/docker/install_airflow_when_building_images.sh: ########## @@ -75,6 +201,13 @@ function install_from_sources() { ${extra_sync_flags} --no-binary-package lxml --no-binary-package xmlsec \ --no-python-downloads --no-managed-python else + 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. Review 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 "<name> 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.* -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
