potiuk commented on code in PR #73182:
URL: https://github.com/apache/airflow/pull/73182#discussion_r4025107421
##########
.github/actions/install-prek/action.yml:
##########
@@ -143,16 +137,67 @@ runs:
shell: bash
run: cat ~/.cache/prek/prek.log || true
if: always()
+ - name: "Decide whether to refresh prek cache"
+ id: cache-policy
+ shell: bash
+ env:
+ EVENT_NAME: ${{ github.event_name }}
+ SAVE_CACHE: ${{ inputs.save-cache }}
+ STASH_HIT: ${{ steps.restore-prek-cache.outputs.stash-hit }}
+ TAR_RESTORED: ${{ steps.restore-prek-tar.outputs.tar-restored }}
+ run: |
+ SAVE=false
+ if [[ "${SAVE_CACHE}" == "true" ]]; then
+ if [[ "${STASH_HIT}" != "true" || "${TAR_RESTORED}" != "true" || \
Review Comment:
This condition and the "always validate" step below it disagree with each
other.
The stated reason for running `prek install-hooks` unconditionally is that a
successfully extracted archive can still hold a partial or unusable
environment. But in precisely that situation `STASH_HIT` and `TAR_RESTORED` are
both `true`, so `save` is `false` and whatever install-hooks just repaired is
thrown away. The cache key has not changed, so the next run restores the same
partial archive and repairs it again — and so on indefinitely, except on
`schedule`.
Consider having the install-hooks step signal whether it actually had to
build anything, and OR that into this condition.
##########
.github/actions/install-prek/action.yml:
##########
@@ -46,18 +46,17 @@ runs:
- name: "Compute prek cache key"
id: cache-key
shell: bash
- # Built here rather than assembled from a version each caller formats
for itself, which is how
- # the readers ended up asking for keys no job had ever saved under. uv
resolves the hook
- # environments against the Python on PATH, so callers must run this
after Breeze sets it up.
+ # Include the actual host ABI/OS and absolute environment paths, not
only the target
+ # image platform. Restoring an archive is still followed by prek's
environment validation.
env:
PLATFORM: ${{ inputs.platform }}
UV_VERSION: ${{ steps.versions.outputs.uv-version }}
- PREK_CONFIG_HASH: ${{ hashFiles('**/.pre-commit-config.yaml') }}
- run: |
- PYTHON_VERSION=$(python3 -c 'import platform;
print(platform.python_version())')
-
KEY="cache-prek-v9-${PLATFORM}-python${PYTHON_VERSION}-uv${UV_VERSION}-${PREK_CONFIG_HASH}"
- echo "Prek cache key: ${KEY}"
- echo "key=${KEY}" >> "${GITHUB_OUTPUT}"
+ PREK_VERSION: ${{ steps.versions.outputs.prek-version }}
+ # Dependency changes in local hooks must not reuse an older
environment identity.
+ PREK_CONFIG_HASH: >-
+ ${{ hashFiles('**/.pre-commit-config.yaml',
'**/.pre-commit-hooks.yaml',
Review Comment:
These extra globs invalidate the cache on changes that cannot alter what is
cached.
Counted in this repo: `**/pyproject.toml` → 141 tracked files, `**/uv.lock`
→ 3, `**/setup.cfg` + `**/setup.py` → 1, and `**/.pre-commit-hooks.yaml` →
**0** (a glob that matches nothing).
What is cached is `~/.cache/prek`: hook environments prek builds from
`.pre-commit-config.yaml`, where every `additional_dependencies` is pinned
inline (`ruff==0.16.4`, `black==26.1.0`, `bandit==1.7.6`, …). In that config
`pyproject.toml` only ever appears in `files:` selectors, i.e. which files a
hook runs *on* — never as a source of a hook's environment. The mypy hooks that
do read `uv.lock` build into `.build/mypy-venvs/`, outside this cache entirely.
And the only part of `uv.lock` that reaches the prek environment, the `uv` and
`prek` versions, is already in the key explicitly above as
`UV_VERSION`/`PREK_VERSION`.
18 of the last 332 commits on `main` touched a `pyproject.toml` or
`uv.lock`; each of those now discards a warm cache for no correctness gain.
Suggest reverting to just `**/.pre-commit-config.yaml`.
##########
scripts/ci/prek_cache_key.py:
##########
@@ -0,0 +1,71 @@
+# 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.
+
+"""Compute a cache identity for prek's host-side hook environments."""
+
+from __future__ import annotations
+
+import hashlib
+import json
+import os
+import platform
+import sys
+import sysconfig
+from collections.abc import Mapping
+from pathlib import Path
+
+REQUIRED_INPUTS = ("PLATFORM", "UV_VERSION", "PREK_VERSION",
"PREK_CONFIG_HASH", "GITHUB_WORKSPACE")
+
+
+def build_cache_identity(environ: Mapping[str, str]) -> dict[str, str]:
+ """Do not reuse virtualenvs across incompatible hosts or absolute
interpreter paths."""
+ missing = [name for name in REQUIRED_INPUTS if not environ.get(name)]
+ if missing:
+ raise ValueError(f"Missing prek cache inputs: {', '.join(missing)}")
+ try:
+ os_release = platform.freedesktop_os_release()
+ except OSError:
+ os_release = {}
+ return {
+ **{name: environ[name] for name in REQUIRED_INPUTS},
+ "system": platform.system(),
+ "machine": platform.machine(),
+ "os_id": os_release.get("ID", ""),
+ "os_version": os_release.get("VERSION_ID", ""),
+ "python_version": platform.python_version(),
+ "python_abi": sysconfig.get_config_var("SOABI") or "",
+ "python_executable": str(Path(sys.executable).resolve()),
Review Comment:
The principle is right — virtualenvs embed absolute paths, so reusing one
under a different prefix is genuinely unsafe. My concern is the blast radius
given how few writers there are.
`save-cache: true` is set in exactly two places (`basic-tests.yml:288` and
`ci-image-checks.yml:150`, mutually exclusive via `basic-checks-only`), so a CI
run has **one** cache writer feeding **nine** reader call sites. Keying on
`python_executable`, `python_prefix`, `home` and `GITHUB_WORKSPACE` means a
cache saved on one runner class can never be restored on another. If the writer
and the readers ever sit on different runner classes — self-hosted vs
GitHub-hosted — the effect is not a lower hit rate, it is a hit rate of zero.
This is the change that most needs a before/after hit-rate measurement to
justify itself.
##########
.github/actions/install-prek/action.yml:
##########
@@ -46,18 +46,17 @@ runs:
- name: "Compute prek cache key"
id: cache-key
shell: bash
- # Built here rather than assembled from a version each caller formats
for itself, which is how
- # the readers ended up asking for keys no job had ever saved under. uv
resolves the hook
- # environments against the Python on PATH, so callers must run this
after Breeze sets it up.
+ # Include the actual host ABI/OS and absolute environment paths, not
only the target
Review Comment:
The comment replaced here carried two things worth keeping:
1. *why* the key is built in the action rather than assembled by each caller
— "which is how the readers ended up asking for keys no job had ever saved
under", i.e. a bug that actually happened;
2. the ordering constraint that callers must run this **after Breeze has set
up PATH**, because uv resolves the hook environments against the Python on PATH.
Point 2 still holds — `prek_cache_key.py` calls `platform.python_version()`
on whichever `python3` is ambient — but nothing in the diff records it any
more. Could you fold both back in?
##########
scripts/ci/prek_cache_key.py:
##########
@@ -0,0 +1,71 @@
+# 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.
+
+"""Compute a cache identity for prek's host-side hook environments."""
+
+from __future__ import annotations
+
+import hashlib
+import json
+import os
+import platform
+import sys
+import sysconfig
+from collections.abc import Mapping
+from pathlib import Path
+
+REQUIRED_INPUTS = ("PLATFORM", "UV_VERSION", "PREK_VERSION",
"PREK_CONFIG_HASH", "GITHUB_WORKSPACE")
Review Comment:
`GITHUB_WORKSPACE` is required here but, unlike the other four, is never set
in the action's `env:` block — it works only because the runner exports it by
default. That makes the script fail with a confusing "Missing prek cache
inputs" when run anywhere else.
Worth setting it explicitly alongside
`PLATFORM`/`UV_VERSION`/`PREK_VERSION`/`PREK_CONFIG_HASH` so the step's inputs
are all visible in one place.
##########
scripts/ci/prek_cache_key.py:
##########
@@ -0,0 +1,71 @@
+# 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.
+
+"""Compute a cache identity for prek's host-side hook environments."""
+
+from __future__ import annotations
+
+import hashlib
+import json
+import os
+import platform
+import sys
+import sysconfig
+from collections.abc import Mapping
+from pathlib import Path
+
+REQUIRED_INPUTS = ("PLATFORM", "UV_VERSION", "PREK_VERSION",
"PREK_CONFIG_HASH", "GITHUB_WORKSPACE")
+
+
+def build_cache_identity(environ: Mapping[str, str]) -> dict[str, str]:
+ """Do not reuse virtualenvs across incompatible hosts or absolute
interpreter paths."""
Review Comment:
This reads as an imperative rationale rather than a docstring — it says what
the caller should avoid, not what the function does or returns. Same for
`compute_cache_key` below.
A `#` comment above the `def` is the better home for the reasoning; the
docstring can then say what comes back (the identity mapping the cache key is
derived from).
--
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]
