docs(proposals): add unified CacheManager design proposal - #1286
Open
Xaenalt wants to merge 1 commit into
Open
Conversation
Describes the design for a unified cache subsystem with prioritized multi-backend lookup, covering motivation, architecture decisions, backend protocol, CLI surface, and integration points. Co-Authored-By: Claude <claude@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Sean Pryor <spryor@redhat.com>
📝 WalkthroughWalkthroughThe PR adds a 160-line proposal for a unified cache manager. It defines prioritized local and remote cache lookup, cache storage, integrity verification, observability, cached-wheel bootstrap short-circuiting, remote PEP 503 fetching, cache management CLI commands, opt-in configuration, security safeguards, verification coverage, and future extensions. Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/proposals/unified-cache-manager.rst`:
- Line 66: Update the PrepareSource.run() reference in the CacheManager
documentation text to use RST’s required double-backtick markup, preserving the
surrounding wording.
- Around line 106-115: The heading "Files added or modified:" in the
unified-cache-manager proposal document is ambiguous and implies these files are
being changed by the current PR, when they actually represent planned changes
for a future implementation. Rename this heading to something like "Planned
implementation touchpoints" or add introductory text that clarifies the list
describes the proposed implementation work rather than current modifications.
- Around line 130-141: The proposal contains conflicting hash-mismatch behavior:
the remote-download rules say mismatches raise errors, while the fetch-failure
rules treat them as cache misses. Choose one contract, update both the
hash-verification and fetch-failure bullets consistently, and if fallback
remains enabled, preserve hash failures as distinct from ordinary cache misses.
- Around line 41-45: Update WheelCacheKey and the local cache lookup to include
and enforce wheel compatibility tags—interpreter, ABI, and platform—when
identifying or matching artifacts. Ensure local lookup filters candidates by all
tags before returning a hit, keeping local and RemotePEP503Backend behavior
consistent and preventing incompatible wheels from being selected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59d3394f-685f-4441-90b7-ff7e8607e3ef
📒 Files selected for processing (1)
docs/proposals/unified-cache-manager.rst
Comment on lines
+41
to
+45
| - ``WheelCacheKey`` — Content-addresses artifacts by canonicalized package | ||
| name, version, and numeric build tag. | ||
| - ``CacheBackend`` protocol — Abstract interface implemented by | ||
| ``LocalDirectoryBackend`` (filesystem) and ``RemotePEP503Backend`` | ||
| (PEP 503 simple repository). Thread-safe with internal locking. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== proposal file around relevant lines =="
sed -n '1,180p' docs/proposals/unified-cache-manager.rst | cat -n
echo
echo "== search unified cache references =="
rg -n "WheelCacheKey|CacheBackend|RemotePEP503Backend|LocalDirectoryBackend|compatibility|interpreter|wheel tag|tags|platform|abi" -S .Repository: python-wheel-build/fromager
Length of output: 40410
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status and tracked proposal diff names =="
git status --short -- docs/src/fromager src/fromager tests docs/proposals/unified-cache-manager.rst || true
git diff --stat HEAD..origin/main -- docs/proposals/unified-cache-manager.rst src/fromager tests 2>/dev/null || true
echo
echo "== parse_wheel_filename and cache-related files =="
rg -n "def parse_wheel_filename|class .*Cache|_find_cached_wheel_via_manager|wheel_tags|WheelCacheKey|extract_info_from_wheel_file" src tests docs -S
echo
echo "== parse_wheel_filename implementation context =="
fd -a "wheels.py|cache.py|_cache.py" . | sed 's#^\./##' | while read -r f; do
echo "--- $f"
wc -l "$f"
doneRepository: python-wheel-build/fromager
Length of output: 2875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/fromager/_cache.py outline =="
ast-grep outline src/fromager/bootstrapper/_cache.py --view expanded || true
echo
echo "== _cache.py lookup/store context =="
sed -n '1,226p' src/fromager/bootstrapper/_cache.py | cat -n
echo
echo "== src/fromager/wheels.py parse implementation =="
sed -n '1,210p' src/fromager/wheels.py | cat -nRepository: python-wheel-build/fromager
Length of output: 18918
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== finders.py relevant context =="
sed -n '1,190p' src/fromager/finders.py | cat -n
echo
echo "== wheel filename behavior probe =="
python3 - <<'PY'
import re
from packaging.utils import parse_wheel_filename
samples = [
"pkg-1.0-cp311-cp311-linux_x86_64.whl",
"pkg-1.0-cp310-cp310-linux_x86_64.whl",
"pkg-1.0-py3-none-any.whl",
"pkg-1.0-cp311-cp311-linux_aarch64.whl",
]
name_re = re.compile(r"^([A-Za-z0-9._-]+)-([A-Za-z0-9._]+)(?:-(\d[\w._-]*))?(?:\+([\w.]*))?\.whl$")
for name in samples:
print(name, ">", parse_wheel_filename(name), "first tag:", sorted(parse_wheel_filename(name)[3]))
m = name_re.match(name)
print(" simple key", (m.group(1), m.group(2), m.group(3)))
PYRepository: python-wheel-build/fromager
Length of output: 8863
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== remaining find_wheel body =="
sed -n '190,220p' src/fromager/finders.py | cat -n
echo
echo "== deterministic local-file-key probe using find_wheel's prefix logic =="
python3 - <<'PY'
import re
from packaging.utils import canonicalize_name
def dist_name_to_filename(dist_name: str) -> str:
canonical_name = canonicalize_name(dist_name)
return re.sub(r"[^\w\d.]+", "_", canonical_name, flags=re.UNICODE)
def local_candidate_bases(name, version, build_tag):
filename_prefix = dist_name_to_filename(name)
candidate_bases_build_tag = f"{build_tag[0]}{build_tag[1]}-" if build_tag else ""
return [
f"{filename_prefix}-{version}-{candidate_bases_build_tag}",
f"{canonicalize_name(name)}-{version}-{candidate_bases_build_tag}",
f"{name}-{version}-{candidate_bases_build_tag}",
f"{name.replace('-', '.')}-{version}-{candidate_bases_build_tag}",
]
wheel_names = [
"pkg-1.0-cp311-cp311-linux_x86_64.whl",
"pkg-1.0-cp310-cp310-linux_x86_64.whl",
"pkg-1.0-py3-none-any.whl",
"pkg-1.0-cp311-cp311-linux_aarch64.whl",
"pkg-1.0.abi3-cp311-cp311-musllinux_1_2_x86_64.whl",
"pkg-1.0-cp39-cp39-macosx_11_0_x86_64.whl",
]
req_name = "pkg"
version = "1.0"
build_tag = ("0", "")
print("local prefixes:", local_candidate_bases(req_name, version, build_tag))
matches = {}
for candidate in local_candidate_bases(req_name, version, build_tag):
hits = [name for name in wheel_names if name.startswith(candidate)]
if hits:
matches[candidate] = hits
print("local prefix hits:", matches)
PYRepository: python-wheel-build/fromager
Length of output: 1687
Include wheel tags in the cache identity.
WheelCacheKey only uses canonical package name, version, and build tag. Local lookup currently matches by filename prefix and returns the first match, while PEP 503 lookups honor interpreter/ABI/platform tags. The same release can therefore return an incompatible local wheel before an incompatible remote fallback is used. Include compatibility tags in WheelCacheKey, or require local lookup to filter by all wheel tags before returning a hit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/proposals/unified-cache-manager.rst` around lines 41 - 45, Update
WheelCacheKey and the local cache lookup to include and enforce wheel
compatibility tags—interpreter, ABI, and platform—when identifying or matching
artifacts. Ensure local lookup filters candidates by all tags before returning a
hit, keeping local and RemotePEP503Backend behavior consistent and preventing
incompatible wheels from being selected.
| ~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| When a cache hit is found during the ``PREPARE_SOURCE`` phase and a | ||
| ``CacheManager`` is active, ``PrepareSource.run()`` skips build environment |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use double backticks for PrepareSource.run().
The repository rule for RST files requires double backticks around function and class names. Change this reference to PrepareSource.run().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/proposals/unified-cache-manager.rst` at line 66, Update the
PrepareSource.run() reference in the CacheManager documentation text to use
RST’s required double-backtick markup, preserving the surrounding wording.
Source: Coding guidelines
Comment on lines
+106
to
+115
| Files added or modified: | ||
|
|
||
| - ``src/fromager/cache.py`` — New module with all cache classes and factory. | ||
| - ``src/fromager/commands/cache_cmd.py`` — CLI commands. | ||
| - ``src/fromager/commands/bootstrap.py`` — Wiring ``--use-cache-manager`` and | ||
| ``--cache-allow-insecure`` options. | ||
| - ``src/fromager/bootstrapper/_cache.py`` — Short-circuit integration via | ||
| ``_find_cached_wheel_via_manager``. | ||
| - ``src/fromager/context.py`` — ``cache`` property on ``WorkContext``. | ||
| - ``tests/test_cache.py`` — Unit tests for the cache subsystem. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that this file list is planned.
This standalone PR adds the proposal document, but “Files added or modified” reads as a list of files changed by this PR. Rename the heading to “Planned implementation touchpoints” or state that the list describes the implementation work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/proposals/unified-cache-manager.rst` around lines 106 - 115, The heading
"Files added or modified:" in the unified-cache-manager proposal document is
ambiguous and implies these files are being changed by the current PR, when they
actually represent planned changes for a future implementation. Rename this
heading to something like "Planned implementation touchpoints" or add
introductory text that clarifies the list describes the proposed implementation
work rather than current modifications.
Comment on lines
+130
to
+141
| - Remote downloads are verified against SHA256 hashes declared in PEP 503 | ||
| index pages. Mismatched files are deleted and raise an error. | ||
| - Plaintext HTTP URLs without SHA256 hashes are rejected by default. | ||
| The ``--cache-allow-insecure`` flag explicitly opts in for internal or | ||
| development registries. | ||
| - Filenames from remote indices are sanitized to prevent directory traversal. | ||
| - Local cache writes use atomic ``tempfile`` + ``rename`` to prevent readers | ||
| from observing partial files. | ||
| - ``scan()`` skips symlinked wheels to prevent ``invalidate``/``gc`` from | ||
| deleting files outside the cache root. | ||
| - Fetch failures (network errors, hash mismatches) are caught and treated as | ||
| cache misses, falling through to the next backend or a fresh build. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists and line count =="
wc -l docs/proposals/unified-cache-manager.rst
echo "== relevant proposal lines 120-150 =="
sed -n '120,150p' docs/proposals/unified-cache-manager.rst | cat -n -v
echo "== all hash mismatch text in proposal =="
rg -n "hash|SHA256|cache miss|cache misses|mismatch" docs/proposals/unified-cache-manager.rst || true
echo "== search implementation references to hash mismatch/cache miss in proposal/design area =="
rg -n "hash mismatch|cache miss|cache misses|integrity|SHA256|mismatch" docs/proposals . -g '!**/.git/**' | head -200Repository: python-wheel-build/fromager
Length of output: 6099
Define one behavior for hash mismatches.
The proposal says mismatched files are deleted and raise an error, while the final bullet says hash mismatches are caught and treated as cache misses. Choose one contract and use it consistently. If fetching falls through, keep hash failures distinct from ordinary cache misses or do not fall through.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/proposals/unified-cache-manager.rst` around lines 130 - 141, The
proposal contains conflicting hash-mismatch behavior: the remote-download rules
say mismatches raise errors, while the fetch-failure rules treat them as cache
misses. Choose one contract, update both the hash-verification and fetch-failure
bullets consistently, and if fallback remains enabled, preserve hash failures as
distinct from ordinary cache misses.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implementation: #1216
Test plan
hatch run docs:build)Made with Cursor