Add unified CacheManager with prioritized multi-backend lookup - #1216
Add unified CacheManager with prioritized multi-backend lookup#1216Xaenalt wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds cache keys, backends, routing, stats, and manager orchestration. Estimated code review effort: 5 (Critical) | ~120 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: 6
🧹 Nitpick comments (1)
tests/test_cache.py (1)
1045-1103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert same-run promotion to the local backend here.
This test proves the file was downloaded, but not that the
CacheManageractually promoted it intolocal_backendfor subsequent lookups. Adding a second lookup (or a directlocal_backend.lookup(...)assertion) would catch regressions where remote hits never become local cache hits.Based on learnings and path instructions: "tests/**: Verify test actually tests the intended behavior. Check for missing edge cases."🤖 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 `@tests/test_cache.py` around lines 1045 - 1103, The remote-hit test currently only verifies the download result and does not confirm same-run promotion into the local cache. Update test_remote_hit_downloads_to_local_store in CacheManager flow to perform a second lookup or directly query local_backend after the first remote hit, and assert that the item is now served from LocalDirectoryBackend as a local hit. Use the existing symbols CacheManager.lookup_wheel, local_backend, and RemotePEP503Backend to keep the test focused on promotion behavior.Source: Path instructions
🤖 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 `@src/fromager/bootstrapper.py`:
- Around line 1535-1541: The wheel mirror update is happening before the routed
cache entry is stored, which can leave the internal index out of sync for the
newly copied path. In `bootstrapper.py`, reorder the flow in the cache-hit
handling around `server.update_wheel_mirror`, `self.ctx.package_build_info`, and
`self.ctx.cache.store_wheel` so the wheel is stored in the routed collection
first and the mirror is updated afterward. Keep the existing routing logic
intact, just move the mirror refresh to run after `store_wheel()` completes.
In `@src/fromager/cache.py`:
- Around line 664-667: After backend.fetch in the cache hit path, register the
downloaded artifact with collection.store_backend so its index is updated as a
local artifact instead of remaining remote-only. Use the existing cache-hit flow
around backend.fetch and the collection.store_backend object to add the
downloaded local_path back into the store manager in the same way promoted
artifacts are tracked, so live cache/admin commands can see it.
- Around line 454-463: Verify and enforce the advertised sha256 when downloading
a remote wheel in fetch(). After the session.get/iter_content write completes,
compute the file’s SHA256 for the downloaded target and compare it against
info.sha256 from ArtifactInfo; if it does not match, reject the wheel by
removing the cached file and raising an error. Keep the existing download flow
in fetch() but add the hash validation before returning target so only verified
wheels are accepted.
- Around line 504-535: The _parse_project_page() helper is accepting untrusted
anchor text as ArtifactInfo.filename, which later gets joined by fetch() when
writing to dest, so reject any remote wheel name containing path components or
traversal segments. Update _parse_project_page() to validate the parsed filename
before appending ArtifactInfo, and only allow a plain basename for remote
artifacts while keeping the existing URL and sha256 parsing logic intact.
In `@src/fromager/commands/cache_cmd.py`:
- Around line 92-99: The variant cache setup in cache_cmd.py is reusing the same
prebuilt_backend instance in both the variant and default collections, which
breaks collection scoping and causes double counting/mutating shared files.
Update the CacheCollection construction logic around wkctx.variant so the
variant collection only owns variant-specific backends, while prebuilt_backend
remains attached to the default collection and is still reachable via
search_order. Keep the unique symbols CacheCollection, variant_backends,
collections[wkctx.variant], and search_order aligned so cache list/stats and
invalidate --collection <variant> operate on separate backend instances.
In `@tests/test_bootstrapper_iterative.py`:
- Around line 1319-1368: The cache hit stats test is bypassing the real lookup
path by mocking _find_cached_wheel_via_manager, so CacheManager.lookup_wheel()
never executes and the hit event cannot be verified. Update
test_cache_hit_records_stats to exercise the actual manager lookup path by only
stubbing the unpack step (for example, _unpack_metadata_from_wheel) and then
assert the relevant stats counter/event produced by CacheManager/Bootstrapper.
If this test is only duplicating phase progression, rename or remove it so it
validates the intended stats behavior.
---
Nitpick comments:
In `@tests/test_cache.py`:
- Around line 1045-1103: The remote-hit test currently only verifies the
download result and does not confirm same-run promotion into the local cache.
Update test_remote_hit_downloads_to_local_store in CacheManager flow to perform
a second lookup or directly query local_backend after the first remote hit, and
assert that the item is now served from LocalDirectoryBackend as a local hit.
Use the existing symbols CacheManager.lookup_wheel, local_backend, and
RemotePEP503Backend to keep the test focused on promotion behavior.
🪄 Autofix (Beta)
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
Run ID: a423bd0f-a4ca-4a6b-892c-f98580939f94
📒 Files selected for processing (9)
pyproject.tomlsrc/fromager/bootstrapper.pysrc/fromager/cache.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/cache_cmd.pysrc/fromager/context.pysrc/fromager/requirements_file.pytests/test_bootstrapper_iterative.pytests/test_cache.py
28b92ad to
c2e2a8e
Compare
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 `@src/fromager/cache.py`:
- Line 599: The assignment in the cache setup is treating an explicit empty
variant_packages set as missing, so callers can unintentionally fall back to
accelerated_packages. Update the logic in the relevant cache initialization path
around self._variant_packages to distinguish None from an empty set, preserving
an explicitly provided empty variant_packages while still using
accelerated_packages only when variant_packages is not supplied.
- Around line 686-692: The cache lookup path in lookup_wheel/cache fetch
handling treats a fetch failure as fatal instead of falling through to the next
cache or build fallback. Update the logic around backend.fetch and the local
index store in cache.py so exceptions from unusable hits (HTTP errors, hash
mismatch, I/O failures) are caught and treated like a miss, allowing the loop
over backends to continue. Keep the successful-path registration with
collection.store_backend.store intact, but only after a confirmed fetch.
- Around line 366-369: The cache write path in store() currently skips copying
when a same-named artifact already exists, which can leave stale wheels on disk
while still treating the new build as stored. Update the logic around
Cache.store() and the dest check so existing entries are either verified before
reuse or overwritten with the newly built artifact before indexing, ensuring the
cache index always matches the actual file on disk.
- Around line 461-474: The download path in the cache handling logic should not
trust an existing wheel at target without validating it first, and it should not
leave partial files behind after a failed or interrupted write. Update the
cache/download flow around the existing target check and the write-and-hash
block in the cache module so that an existing file is SHA256-verified before
returning it, and any checksum mismatch or write failure removes the target
before raising. Use the existing download helper and the hasher/target logic to
keep validation and cleanup consistent.
🪄 Autofix (Beta)
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
Run ID: 9a7ae0f7-3f32-4f7e-b0a2-36b4e2177ca6
📒 Files selected for processing (6)
src/fromager/bootstrapper.pysrc/fromager/cache.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/cache_cmd.pytests/test_bootstrapper_iterative.pytests/test_cache.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/fromager/commands/bootstrap.py
- src/fromager/commands/cache_cmd.py
- tests/test_cache.py
- tests/test_bootstrapper_iterative.py
- src/fromager/bootstrapper.py
0cab1ce to
be8d8ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/fromager/cache.py (1)
718-725: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the requirement logging context to this warning.
This warning is per-requirement but does not use
req_ctxvar_context(), so it can lose the structured requirement context used elsewhere.As per coding guidelines, “Use
req_ctxvar_context()for per-requirement logging.”🤖 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 `@src/fromager/cache.py` around lines 718 - 725, The cache hit warning in the requirement fetch path is missing the structured per-requirement logging context. Update the logging in the code path around the cache fetch warning to run inside req_ctxvar_context() so the warning emitted from the requirement-specific logic carries the same requirement context used elsewhere. Keep the existing logger.warning message and symbols like req, version, collection_name, backend.name, and err, but ensure the warning is emitted within the req_ctxvar_context() wrapper.Source: Coding guidelines
🤖 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 `@src/fromager/cache.py`:
- Around line 475-489: The temporary download path in the cache download flow is
deterministic, so parallel fetches can overwrite each other’s in-progress file.
Update the download logic in the cache helper that creates tmp_target and later
calls tmp_target.replace(target) to use a unique temp file under dest for each
download, then keep the atomic replace into target once the content and sha256
check succeed. Ensure the change stays localized to the existing
download-and-verify path so concurrent builders do not share the same temporary
filename.
- Around line 367-369: The cache write in the artifact copy path is not atomic,
so concurrent readers can see a partially written wheel. Update the logic in the
copy block that uses `self._directory`, `dest`, and `shutil.copy2` to write to a
unique temporary file in the same directory first, then atomically swap it into
place with `replace()`. Keep the existing freshness check around
`artifact.samefile(dest)`/`dest.exists()` and apply the atomic replace only when
the cached file needs updating.
---
Nitpick comments:
In `@src/fromager/cache.py`:
- Around line 718-725: The cache hit warning in the requirement fetch path is
missing the structured per-requirement logging context. Update the logging in
the code path around the cache fetch warning to run inside req_ctxvar_context()
so the warning emitted from the requirement-specific logic carries the same
requirement context used elsewhere. Keep the existing logger.warning message and
symbols like req, version, collection_name, backend.name, and err, but ensure
the warning is emitted within the req_ctxvar_context() wrapper.
🪄 Autofix (Beta)
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
Run ID: 79f7fc32-8f30-49aa-bc6c-755c6c82cc77
📒 Files selected for processing (1)
src/fromager/cache.py
|
This pull request has merge conflicts that must be resolved before it can be merged. |
7c4cbf0 to
4449d85
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/test_cache.py (2)
1143-1155: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert remote hits are registered in the local backend index.
The integration test verifies the file is downloaded, but not that
collection.store_backend.store()updated the local index for later lookups/admin commands. Add an assertion againstlocal_backend.lookup(key). As per path instructions, “Verify test actually tests the intended behavior. Check for missing edge cases.”🤖 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 `@tests/test_cache.py` around lines 1143 - 1155, The integration test for manager.lookup_wheel only checks the downloaded wheel result and misses verifying that a remote hit is recorded in the local backend index. After the existing assertions, add an assertion using local_backend.lookup(key) to confirm collection.store_backend.store() updated the local index for later lookups/admin commands. Use the existing manager.lookup_wheel flow and the local_backend/key symbols from this test to locate the right spot.Source: Path instructions
566-592: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover the existing-file SHA mismatch path.
This test only proves that an existing file is reused when no hash is provided. Add the edge case where
info.sha256is present and the existing file is corrupt, so the re-download behavior cannot regress. As per path instructions, “Verify test actually tests the intended behavior. Check for missing edge cases.”🤖 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 `@tests/test_cache.py` around lines 566 - 592, The existing-file reuse test in test_fetch_skips_existing_file only covers the no-hash case, so extend it to include an ArtifactInfo with sha256 set and a preexisting corrupt file in the destination. Update the assertions around RemotePEP503Backend.fetch so the existing-file shortcut is only accepted when the file matches the expected SHA256, and verify that a hash mismatch triggers re-download instead of returning the stale path. Use the existing test_fetch_skips_existing_file, RemotePEP503Backend.fetch, and ArtifactInfo symbols to keep the new case aligned with the current behavior.Source: Path instructions
🤖 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 `@src/fromager/cache.py`:
- Around line 324-335: Skip symlinked wheels during cache indexing in the logic
that scans self._directory in cache.py before building WheelCacheKey and
ArtifactInfo. Check each wheel_file from the glob and continue past any symlink
so url_or_path never points to a resolved target outside the cache root. Keep
the change localized to the wheel discovery loop used by invalidate/gc to
prevent later unlink operations from following symlinks.
- Around line 573-588: The URL handling in the artifact resolution logic still
allows plaintext http:// wheel links and accepts downloads without integrity
metadata. Update the URL resolution path in cache.py so the link parsing code
that builds url from href and strips sha256 fragments rejects any non-HTTPS
artifact URL or otherwise enforces that a valid sha256 is present before the
artifact is accepted. Make the check in the same flow that handles href
resolution and fragment parsing so absolute URLs cannot bypass the integrity
requirement.
In `@src/fromager/commands/cache_cmd.py`:
- Around line 357-398: Reject negative values for the gc command’s --keep-latest
option by constraining the Click parameter at the boundary in gc and/or the
`@click.option` definition. Update the keep_latest handling so only zero or
positive integers are accepted before the entries[keep_latest:] slicing logic
runs, preventing negative indexes from causing unintended cache deletions.
In `@tests/test_cache.py`:
- Around line 329-346: The test URLs in RemotePEP503Backend setup use
cache.example.com, but repository conventions require .test domains in tests.
Update the affected assertions and fixture-like instantiations in test_cache.py
to use cache.test instead, keeping the same RemotePEP503Backend cases such as
test_name_default, test_not_writable, and test_store_raises.
---
Nitpick comments:
In `@tests/test_cache.py`:
- Around line 1143-1155: The integration test for manager.lookup_wheel only
checks the downloaded wheel result and misses verifying that a remote hit is
recorded in the local backend index. After the existing assertions, add an
assertion using local_backend.lookup(key) to confirm
collection.store_backend.store() updated the local index for later lookups/admin
commands. Use the existing manager.lookup_wheel flow and the local_backend/key
symbols from this test to locate the right spot.
- Around line 566-592: The existing-file reuse test in
test_fetch_skips_existing_file only covers the no-hash case, so extend it to
include an ArtifactInfo with sha256 set and a preexisting corrupt file in the
destination. Update the assertions around RemotePEP503Backend.fetch so the
existing-file shortcut is only accepted when the file matches the expected
SHA256, and verify that a hash mismatch triggers re-download instead of
returning the stale path. Use the existing test_fetch_skips_existing_file,
RemotePEP503Backend.fetch, and ArtifactInfo symbols to keep the new case aligned
with the current behavior.
🪄 Autofix (Beta)
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
Run ID: f189df6f-a24c-47ee-9690-90fe700d225e
📒 Files selected for processing (9)
pyproject.tomlsrc/fromager/bootstrapper.pysrc/fromager/cache.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/cache_cmd.pysrc/fromager/context.pysrc/fromager/requirements_file.pytests/test_bootstrapper_iterative.pytests/test_cache.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/fromager/requirements_file.py
- pyproject.toml
- src/fromager/context.py
- src/fromager/commands/bootstrap.py
- src/fromager/bootstrapper.py
- tests/test_bootstrapper_iterative.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/fromager/cache.py`:
- Around line 592-597: The plaintext URL rejection in the cache artifact
filtering logic is too case-sensitive because it uses url.startswith("http://"),
which can miss uppercase or mixed-case schemes. Update the check in the artifact
handling path to inspect the parsed URL scheme instead of string-prefix
matching, so the no-hash rejection applies consistently for any HTTP scheme
casing while preserving the existing skip-and-log behavior in the same block.
🪄 Autofix (Beta)
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
Run ID: 71891b2b-023c-4547-accb-f675a4e3561e
📒 Files selected for processing (4)
src/fromager/cache.pysrc/fromager/commands/cache_cmd.pytests/test_cache.pytests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fromager/commands/cache_cmd.py
28678d6 to
55be181
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_cache.py (1)
442-443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove local imports to the top of the file.
import requests(Line 442),from fromager.cache import ArtifactInfo(Lines 543, 568), andimport pytest(Line 672) are all imported inside test functions.pytestis already used at module scope elsewhere (e.g., Line 353), so the Line 672 import is redundant too.♻️ Proposed fix
+import pytest +import requests +from fromager.cache import ArtifactInfo + ... def test_scan_handles_network_error( self, requests_mock: requests_mock.Mocker ) -> None: """scan() gracefully handles a network error.""" - import requests - requests_mock.get(Apply similarly to remove the local imports at lines 543, 568, and 672.
As per coding guidelines, "All imports at the top of the file, no local imports."
Also applies to: 543-543, 568-568, 672-672
🤖 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 `@tests/test_cache.py` around lines 442 - 443, The test module has several local imports inside test functions that should be moved to the top-level import section to match the import style used elsewhere. Update the affected tests in test_cache.py by hoisting the local requests import, the repeated ArtifactInfo import from fromager.cache, and the redundant pytest import; use the existing module-scope pytest usage and keep the test logic unchanged.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@tests/test_cache.py`:
- Around line 442-443: The test module has several local imports inside test
functions that should be moved to the top-level import section to match the
import style used elsewhere. Update the affected tests in test_cache.py by
hoisting the local requests import, the repeated ArtifactInfo import from
fromager.cache, and the redundant pytest import; use the existing module-scope
pytest usage and keep the test logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83dd4fa0-2aab-4c0b-8133-d4c735fdc4f2
📒 Files selected for processing (6)
src/fromager/cache.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/cache_cmd.pytests/test_cache.pytests/test_cli.pytests/test_commands.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_commands.py
- tests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/fromager/commands/bootstrap.py
- src/fromager/commands/cache_cmd.py
- src/fromager/cache.py
Xaenalt
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
This pull request has merge conflicts that must be resolved before it can be merged. |
|
@dhellmann @tiran what do you guys think? |
|
@LalatenduMohanty @rd4398 PTAL |
LalatenduMohanty
left a comment
LalatenduMohanty
left a comment
There was a problem hiding this comment.
@Xaenalt This is a significant change to the caching. Without a proposal to understand the architecture , pros and cons it will be hard for me to accept the PR. Will you willing to send a proposal first? Please check the existing proposals to understand the format.
44f387e to
3176b4b
Compare
|
@LalatenduMohanty How does that proposal format look? |
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
I have left some comments.
I also want to bring following points:
- This is a very huge PR and I think we are shipping two features : cache manager + variant routing in the same PR. I would prefer if these are two separate PRs with smaller commits which are easier to review. You can look at Doug's latest PR as example #1226
- Fromager does not have concept of collections. It is something we do downstream. In my opinion we should not mention anything collection specific here.
| ) | ||
| return {} | ||
|
|
||
| def lookup(self, key: WheelCacheKey) -> ArtifactInfo | None: |
There was a problem hiding this comment.
I think we need a lock here. We should consider adding a threading.Lock to both backends, given that _find_cached_wheel is called from background threads. More detailed context:
def lookup(self, key):
info = self._index.get(key) # thread A reads
if info is not None:
file_path = pathlib.Path(info.url_or_path)
if file_path.exists():
return info
del self._index[key] # thread A deletes
If thread B calls store() between the get and del, the entry could be re-added and then immediately deleted. Similarly, RemotePEP503Backend._project_cache has a check-then-populate race:
if key.package not in self._project_cache: # thread A checks
self._project_cache[key.package] = self._fetch_project_page(...) # thread A fetches
Two threads looking up the same package will both fetch the project page. This is wasteful but not corrupting — however, the _available_packages set has the same pattern and could lead to duplicate network calls on startup.
| @@ -0,0 +1,429 @@ | |||
| """CLI commands for cache management and observability.""" | |||
There was a problem hiding this comment.
This file repeatedly does
if not hasattr(backend, "_index"):
continue
for key, info in backend._index.items():
This bypasses the CacheBackend Protocol and reaches into implementation details.
It means:
- cache list silently skips remote backends entirely (they have _project_cache, not _index)
- cache verify only works with LocalDirectoryBackend (checked via isinstance, which is fine) but then accesses _index directly
- cache gc directly mutates backend._index with del — no locking, no abstraction
Can we consider adding a method to the Protocol (e.g., items() -> Iterable[tuple[WheelCacheKey, ArtifactInfo]]) or at minimum use isinstance checks consistently?
| server, | ||
| ) | ||
| from .build import build_parallel | ||
| from .cache_cmd import _build_cache_manager |
There was a problem hiding this comment.
The factory function _build_cache_manager is business logic, not CLI logic. I think it should live in cache.py itself or a dedicated cache_factory.py. Having the bootstrap command import from the cache CLI module creates a circular-feeling dependency and means _build_cache_manager can't be tested without importing the entire CLI stack.
| size_bytes=wheel_file.stat().st_size, | ||
| ) | ||
| self._index[key] = info | ||
| except Exception: |
There was a problem hiding this comment.
This looks it's overly broad for what should only catch InvalidWheelFilename from parse_wheel_filename.
|
|
||
|
|
||
| @dataclasses.dataclass(frozen=True) | ||
| class SdistCacheKey: |
There was a problem hiding this comment.
Looks like this is never used. I think we should remove it or implements sdist caching as well.
| @@ -0,0 +1,1271 @@ | |||
| """Unit tests for the cache subsystem.""" | |||
There was a problem hiding this comment.
There are still some gaps in tests:
- No concurrency tests. Given the thread-safety concerns above, there should be at least one test that exercises lookup/store from multiple threads.
- No test for _build_cache_manager with variant != "cpu". The variant collection creation path is untested at the factory level.
- cache list/stats/verify/invalidate/gc CLI commands have zero tests. 429 lines of CLI code with no test coverage. The test_cli.py change just adds "cache" to the command list check.
- Remote backend fetch timeout/retry behavior untested. The session.get() call has no timeout parameter and no retry logic for transient failures.
I agree. |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
3176b4b to
56d01e6
Compare
Xaenalt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fromager/bootstrapper/_prepare_source.py (1)
25-52: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
unpacked.nameinstead ofunpacked.stemfor cached unpacked wheels.
pathlib.Path.stemstrips text after the final dot, sopkg-1.2.3becomespkg-1.2. The cached-wheel source path uses that value assdist_root_dir, and the non-CacheManager path passes it through to dependency lookup/build setup as a filesystem path. The CacheManager short-circuit recovers via.parent, but the legacy backend still needs the child directory name.🐛 Proposed fix
if unpacked is not None: return PreparedSourceData( - sdist_root_dir=unpacked / unpacked.stem, + sdist_root_dir=unpacked / unpacked.name, cached_wheel_filename=cached_wheel, )🤖 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 `@src/fromager/bootstrapper/_prepare_source.py` around lines 25 - 52, Update the cached-unpacked-wheel branch in _bg_prepare_source to use unpacked.name when constructing PreparedSourceData.sdist_root_dir instead of unpacked.stem, preserving the full directory name including version suffixes for legacy backend dependency lookup and build setup.
🧹 Nitpick comments (4)
tests/test_cache.py (2)
726-732: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the CLI imports to module scope.
from click.testing import CliRunnerandfrom fromager.commands.cache_cmd import cache_cliare repeated inside seven test bodies. As per coding guidelines, "Place all imports at the top of the file; do not use local imports."♻️ Proposed change
import pytest import requests_mock +from click.testing import CliRunner from packaging.requirements import Requirement @@ +from fromager.commands.cache_cmd import cache_clithen drop the seven in-body import pairs.
🤖 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 `@tests/test_cache.py` around lines 726 - 732, Move the CliRunner and cache_cli imports used by test_cache_list_empty and the other six affected cache CLI tests to module scope, then remove all seven duplicated in-body import pairs while preserving test behavior.Source: Coding guidelines
808-829: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFactory's remote path is untested.
build_cache_manager(wkctx, cache_url=..., allow_insecure=...)appends aRemotePEP503Backendand forwardsallow_insecure(src/fromager/cache.py lines 901-908) — neither is covered. A test asserting the fourth backend name isremote:<url>would catch wiring regressions cheaply.🤖 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 `@tests/test_cache.py` around lines 808 - 829, Extend TestBuildCacheManager with a test calling build_cache_manager using cache_url and allow_insecure, then assert the appended fourth lookup backend is named remote:<url> and that the allow_insecure option is forwarded to the RemotePEP503Backend. Keep the existing local backend ordering and store backend assertions intact.src/fromager/cache.py (2)
114-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
time.monotonic()is not a timestamp.
CacheEvent.timestampgets an arbitrary reference-point value, so it can't be serialized or compared against wall-clock time. Usetime.time()for the event timestamp and keepmonotoniconly for durations.♻️ Proposed change
- timestamp=time.monotonic(), + timestamp=time.time(),(apply in
record_hit,record_miss, andrecord_store)Also applies to: 144-152
🤖 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 `@src/fromager/cache.py` around lines 114 - 120, Update CacheEvent timestamp creation in record_hit, record_miss, and record_store to use time.time() so events contain wall-clock timestamps; retain time.monotonic() exclusively for measuring duration_ms.
632-698: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse PEP 503 href parsing instead of filename-included link text.
_parse_index_page()and_parse_project_page()currently depend on link text containing the artifact name, so multi-line anchors miss packages, and indexes can provide different filename text than thehref. Parsea[href]with a smallHTMLParserutility and take the wheel filename from the href basename/fragment; reusedownloads.extract_filename_from_url()where it matches this URL shape.🤖 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 `@src/fromager/cache.py` around lines 632 - 698, Update _parse_index_page and _parse_project_page to parse anchor href attributes with a small HTMLParser utility instead of regex link text. Derive package and wheel names from each href’s basename, removing query/fragment handling as needed, and reuse downloads.extract_filename_from_url() when applicable; preserve hash extraction, URL resolution, security checks, and ArtifactInfo construction.Source: Coding guidelines
🤖 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`:
- Around line 28-34: Update the CacheManager diagram to include
LocalDirectoryBackend(build-dir) as the first lookup backend, matching the
ordering configured by build_cache_manager; retain downloads, prebuilt, and the
optional remote backend in their existing order.
In `@src/fromager/cache.py`:
- Around line 803-809: Update lookup_wheel’s CacheResult construction to return
the build tag of the matched artifact rather than the empty search tag. For
local misses, derive result.build_tag from the matched wheel filename; for
remote matches with an empty search tag, use the parsed remote tag before
constructing CacheResult, while preserving explicitly requested build tags.
- Around line 910-915: Update build_cache_manager to accept a force option and
forward it when constructing CacheManager, ensuring callers such as CLI paths
can enable CacheManager.force; preserve existing behavior when force is not
provided.
In `@src/fromager/commands/cache_cmd.py`:
- Around line 194-231: Update invalidate and gc to operate only on the downloads
store backend, rather than iterating every manager.lookup_backends entry. Reuse
the existing backend identity/type or store-selection mechanism to skip
local:build and local:prebuilt while retaining deletion and rescanning behavior
for local:downloads.
In `@tests/test_cache.py`:
- Around line 169-179: Add a regression test alongside test_store_same_file_noop
for LocalDirectoryBackend.store that pre-seeds the destination with stale bytes,
stores a same-named wheel from a different source path, and asserts the
destination contains the fresh content. Reuse the existing WheelCacheKey setup
and verify the overwrite path rather than the same-file no-op path.
- Around line 625-664: The test_concurrent_store_and_lookup test does not create
contention or verify index consistency. Make all store_worker and lookup_worker
threads use one shared WheelCacheKey and corresponding wheel path, coordinate
their execution so concurrent operations overlap, then assert the final
lookup/index state contains the expected stored entry; retain error collection
to detect thread failures.
---
Outside diff comments:
In `@src/fromager/bootstrapper/_prepare_source.py`:
- Around line 25-52: Update the cached-unpacked-wheel branch in
_bg_prepare_source to use unpacked.name when constructing
PreparedSourceData.sdist_root_dir instead of unpacked.stem, preserving the full
directory name including version suffixes for legacy backend dependency lookup
and build setup.
---
Nitpick comments:
In `@src/fromager/cache.py`:
- Around line 114-120: Update CacheEvent timestamp creation in record_hit,
record_miss, and record_store to use time.time() so events contain wall-clock
timestamps; retain time.monotonic() exclusively for measuring duration_ms.
- Around line 632-698: Update _parse_index_page and _parse_project_page to parse
anchor href attributes with a small HTMLParser utility instead of regex link
text. Derive package and wheel names from each href’s basename, removing
query/fragment handling as needed, and reuse
downloads.extract_filename_from_url() when applicable; preserve hash extraction,
URL resolution, security checks, and ArtifactInfo construction.
In `@tests/test_cache.py`:
- Around line 726-732: Move the CliRunner and cache_cli imports used by
test_cache_list_empty and the other six affected cache CLI tests to module
scope, then remove all seven duplicated in-body import pairs while preserving
test behavior.
- Around line 808-829: Extend TestBuildCacheManager with a test calling
build_cache_manager using cache_url and allow_insecure, then assert the appended
fourth lookup backend is named remote:<url> and that the allow_insecure option
is forwarded to the RemotePEP503Backend. Keep the existing local backend
ordering and store backend assertions intact.
🪄 Autofix (Beta)
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: f7b2826e-edd2-4f80-90aa-b657ed1f6273
📒 Files selected for processing (13)
docs/proposals/unified-cache-manager.rstpyproject.tomlsrc/fromager/bootstrapper/_cache.pysrc/fromager/bootstrapper/_prepare_source.pysrc/fromager/cache.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/cache_cmd.pysrc/fromager/context.pysrc/fromager/requirements_file.pytests/test_bootstrapper_iterative.pytests/test_cache.pytests/test_cli.pytests/test_commands.py
🚧 Files skipped from review as they are similar to previous changes (4)
- pyproject.toml
- src/fromager/requirements_file.py
- tests/test_cli.py
- src/fromager/context.py
|
@coderabbitai Re: PEP 503 href/HTMLParser nitpick — deferring for this PR. The current regex parser covers the common simple-index shapes we target; switching to an HTMLParser + href-basename approach is a good follow-up if we hit multi-line anchors or mismatched link text in practice. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
56d01e6 to
37a8223
Compare
|
@coderabbitai yes once this PR is merged |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="python-wheel-build/fromager"
pr_url="https://github.com/python-wheel-build/fromager/pull/1216"
body=$(cat <<'EOF'
## Summary
Harden `RemotePEP503Backend` PEP 503 project-page parsing by extracting artifact URLs from HTML anchor `href` attributes, rather than relying solely on the current regex parser and anchor text.
## Rationale
The current implementation intentionally covers the common simple-index shapes targeted by the unified cache manager. It may not handle multiline anchor elements or pages where the anchor text does not match the artifact basename. An `HTMLParser`-based approach should make parsing robust while preserving the existing cache lookup and integrity behavior.
## Affected area
- `src/fromager/cache.py`
- `RemotePEP503Backend` project-page parsing helpers
## Required changes
- Parse PEP 503 anchor elements with an HTML parser and use each anchor's `href` as the artifact source.
- Derive and validate the artifact filename from the URL path/basename rather than trusting anchor text.
- Preserve existing URL safety checks, SHA256 fragment handling, and insecure-HTTP policy.
- Add focused tests for multiline anchors and mismatched anchor text.
## Acceptance criteria
- Valid wheel links are discovered when anchor tags span multiple lines.
- A valid wheel link is discovered when visible anchor text differs from the wheel filename.
- Unsafe filenames/URLs and insecure HTTP behavior remain rejected or allowed according to the existing policy.
- Existing PEP 503 parsing coverage remains passing.
## Backlinks
- Unified cache manager PR: https://github.com/python-wheel-build/fromager/pull/1216
- Deferral discussion: https://github.com/python-wheel-build/fromager/pull/1216#issuecomment-0
Requested by: `@Xaenalt`
EOF
)
gh issue create \
--repo "$repo" \
--title "Harden RemotePEP503Backend PEP 503 HTML link parsing" \
--body "$body" \
--assignee "Xaenalt"Length of output: 2145
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/fromager/commands/cache_cmd.py (1)
306-314: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_format_sizemutates anintparam into afloat.The
# type: ignore[assignment]is masking a signature issue rather than fixing it.♻️ Proposed change
def _format_size(size_bytes: int) -> str: """Format bytes as human-readable size.""" if size_bytes == 0: return "0 B" + size = float(size_bytes) for unit in ("B", "KB", "MB", "GB"): - if abs(size_bytes) < 1024: - return f"{size_bytes:.1f} {unit}" - size_bytes /= 1024 # type: ignore[assignment] - return f"{size_bytes:.1f} TB" + if abs(size) < 1024: + return f"{size:.1f} {unit}" + size /= 1024 + return f"{size:.1f} TB"🤖 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 `@src/fromager/commands/cache_cmd.py` around lines 306 - 314, Update _format_size so it does not reassign the integer size_bytes parameter to a float during unit conversion. Introduce a separate numeric working variable for the division, keep size_bytes as int, and remove the type-ignore assignment suppression while preserving the existing formatting and unit progression.
🤖 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 `@tests/test_cache.py`:
- Around line 264-272: Update test_parse_project_page_rejects_path_traversal so
the malicious href uses a parent-directory traversal path ending in .whl,
ensuring it passes the wheel suffix check and exercises the safe_filename !=
filename guard while preserving the expected single valid artifact.
---
Nitpick comments:
In `@src/fromager/commands/cache_cmd.py`:
- Around line 306-314: Update _format_size so it does not reassign the integer
size_bytes parameter to a float during unit conversion. Introduce a separate
numeric working variable for the division, keep size_bytes as int, and remove
the type-ignore assignment suppression while preserving the existing formatting
and unit progression.
🪄 Autofix (Beta)
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: 8dff3794-8750-47c4-a74d-6febf9f87e1d
📒 Files selected for processing (13)
docs/proposals/unified-cache-manager.rstpyproject.tomlsrc/fromager/bootstrapper/_cache.pysrc/fromager/bootstrapper/_prepare_source.pysrc/fromager/cache.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/cache_cmd.pysrc/fromager/context.pysrc/fromager/requirements_file.pytests/test_bootstrapper_iterative.pytests/test_cache.pytests/test_cli.pytests/test_commands.py
🚧 Files skipped from review as they are similar to previous changes (10)
- pyproject.toml
- src/fromager/context.py
- tests/test_cli.py
- tests/test_commands.py
- docs/proposals/unified-cache-manager.rst
- src/fromager/requirements_file.py
- src/fromager/bootstrapper/_prepare_source.py
- tests/test_bootstrapper_iterative.py
- src/fromager/commands/bootstrap.py
- src/fromager/bootstrapper/_cache.py
37a8223 to
b582d60
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/fromager/cache.py (1)
886-922: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFactory never memoizes the manager on the context.
wkctx.cache is not Noneshort-circuits, but the constructed manager is never stored back, so every caller (each CLI command, bootstrap, etc.) builds new backends and re-scans every wheel directory — and accumulates stats in an object nobody else sees.♻️ Proposed change
manager = CacheManager( lookup_backends=lookup_backends, store_backend=downloads_backend, ) manager.initialize() + wkctx.cache = manager return manager🤖 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 `@src/fromager/cache.py` around lines 886 - 922, Update the cache-manager factory around the existing wkctx.cache guard to assign the newly initialized CacheManager to wkctx.cache before returning it. Preserve the current backend construction, initialization, and early-return behavior so subsequent callers reuse the memoized manager.tests/test_cache.py (2)
787-791: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
gc's actual removal path is untested.The only
gctest hits the empty-cache branch. Grouping by package+version, build-tag sort, thekeep_latestslice, andunlink/rescan (cache_cmd.py Lines 266-300) get zero coverage — that's the destructive code path.💚 Suggested test
def test_cache_gc_keeps_latest_build_tag(self, wkctx: typing.Any) -> None: # Arrange old = wkctx.wheels_downloads / "requests-2.31.0-1-py3-none-any.whl" old.write_bytes(b"old") new = wkctx.wheels_downloads / "requests-2.31.0-2-py3-none-any.whl" new.write_bytes(b"new") # Act result = CliRunner().invoke(cache_cli, ["gc"], obj=wkctx) # Assert assert result.exit_code == 0 assert not old.exists() assert new.exists()🤖 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 `@tests/test_cache.py` around lines 787 - 791, Add a test alongside test_cache_gc_nothing_to_remove that creates multiple cached wheel files for the same package and version with different build tags, invokes cache_cli with “gc”, and verifies successful execution, removal of the older build, and retention of the latest build. Exercise the destructive cleanup path, including grouping, sorting, keep_latest behavior, and the resulting cache state.Source: Path instructions
418-443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing the inverse case: existing file with a bad hash.
This covers the matching-hash reuse, but the re-download branch (cache.py Lines 545-548: unlink + refetch) is uncovered. A test that pre-seeds
destwith wrong bytes, mocks the URL, and asserts the file content is replaced would lock that in.🤖 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 `@tests/test_cache.py` around lines 418 - 443, Add an inverse test alongside test_fetch_uses_cached_file_if_sha256_matches that pre-seeds the destination wheel with bytes whose hash differs from ArtifactInfo.sha256, mocks the artifact URL response, invokes RemotePEP503Backend.fetch, and asserts the cached file is replaced with the downloaded content, covering the unlink-and-refetch path.Source: Path instructions
🤖 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 `@src/fromager/commands/cache_cmd.py`:
- Around line 90-106: Update cache_stats and the cache lifecycle so reported
hit/miss metrics reflect the previous run instead of a newly initialized
CacheManager; persist the CacheStats summary at bootstrap completion and load
that persisted summary for the stats command, while retaining the existing
on-disk inventory metrics. Ensure the documented “from last run” behavior
applies to Total lookups, Hits, Misses, Hit rate, and Stores.
---
Nitpick comments:
In `@src/fromager/cache.py`:
- Around line 886-922: Update the cache-manager factory around the existing
wkctx.cache guard to assign the newly initialized CacheManager to wkctx.cache
before returning it. Preserve the current backend construction, initialization,
and early-return behavior so subsequent callers reuse the memoized manager.
In `@tests/test_cache.py`:
- Around line 787-791: Add a test alongside test_cache_gc_nothing_to_remove that
creates multiple cached wheel files for the same package and version with
different build tags, invokes cache_cli with “gc”, and verifies successful
execution, removal of the older build, and retention of the latest build.
Exercise the destructive cleanup path, including grouping, sorting, keep_latest
behavior, and the resulting cache state.
- Around line 418-443: Add an inverse test alongside
test_fetch_uses_cached_file_if_sha256_matches that pre-seeds the destination
wheel with bytes whose hash differs from ArtifactInfo.sha256, mocks the artifact
URL response, invokes RemotePEP503Backend.fetch, and asserts the cached file is
replaced with the downloaded content, covering the unlink-and-refetch path.
🪄 Autofix (Beta)
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: 0bd59978-9cb9-4e35-b674-261b6224082b
📒 Files selected for processing (13)
docs/proposals/unified-cache-manager.rstpyproject.tomlsrc/fromager/bootstrapper/_cache.pysrc/fromager/bootstrapper/_prepare_source.pysrc/fromager/cache.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/cache_cmd.pysrc/fromager/context.pysrc/fromager/requirements_file.pytests/test_bootstrapper_iterative.pytests/test_cache.pytests/test_cli.pytests/test_commands.py
🚧 Files skipped from review as they are similar to previous changes (8)
- pyproject.toml
- src/fromager/requirements_file.py
- tests/test_commands.py
- docs/proposals/unified-cache-manager.rst
- src/fromager/context.py
- src/fromager/bootstrapper/_cache.py
- src/fromager/commands/bootstrap.py
- src/fromager/bootstrapper/_prepare_source.py
b582d60 to
91d080e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_cache.py (1)
418-443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the stale-cached-file re-download branch.
fetch()(src/fromager/cache.py Lines 545-548) unlinks an existing file whose SHA256 doesn't match and re-downloads. Nothing exercises that path, so a regression there would go unnoticed.Suggested test
def test_fetch_redownloads_when_existing_sha256_mismatches( self, tmp_path: pathlib.Path, requests_mock: requests_mock.Mocker ) -> None: # Arrange wheel_data = b"good wheel" sha256 = hashlib.sha256(wheel_data).hexdigest() url = "https://cache.test/simple/requests/requests-2.31.0-1-py3-none-any.whl" requests_mock.get(url, content=wheel_data) dest = tmp_path / "downloads" dest.mkdir() (dest / "requests-2.31.0-1-py3-none-any.whl").write_bytes(b"stale") backend = RemotePEP503Backend( server_url="https://cache.test/simple", download_dir=tmp_path ) key = WheelCacheKey( package=canonicalize_name("requests"), version=Version("2.31.0"), build_tag=(1, ""), ) info = ArtifactInfo( filename="requests-2.31.0-1-py3-none-any.whl", url_or_path=url, sha256=sha256, ) # Act result = backend.fetch(key, info, dest) # Assert assert result.read_bytes() == wheel_data🤖 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 `@tests/test_cache.py` around lines 418 - 443, Add a test alongside test_fetch_uses_cached_file_if_sha256_matches that creates a stale existing wheel, mocks the artifact URL to return valid wheel_data, and calls RemotePEP503Backend.fetch with the matching ArtifactInfo checksum. Assert the returned file contains the downloaded data, covering the unlink-and-redownload branch when the cached SHA256 mismatches.
🤖 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 `@src/fromager/cache.py`:
- Around line 537-579: Update the SHA256 comparisons in the cache retrieval flow
around the existing-target verification and downloaded-file validation to
compare hexadecimal digests case-insensitively. Preserve the current mismatch
handling, deletion, and error reporting while normalizing both the computed
digest and info.sha256 for comparison.
In `@src/fromager/commands/cache_cmd.py`:
- Around line 264-286: Update the sorting key in the garbage-collection loop
over groups to use build-tag numbers for tagged entries and the file
modification time for untagged entries. Resolve each untagged entry’s path from
info.url_or_path and use its mtime as the recency suffix, preserving descending
order so --keep-latest retains the newest files.
In `@tests/test_cache.py`:
- Around line 775-785: Update test_cache_stats to assert the complete rendered
count line, such as “Total wheels on disk: 1”, instead of checking for the
standalone character “1”. Keep the existing CLI invocation and other output
assertions unchanged.
---
Nitpick comments:
In `@tests/test_cache.py`:
- Around line 418-443: Add a test alongside
test_fetch_uses_cached_file_if_sha256_matches that creates a stale existing
wheel, mocks the artifact URL to return valid wheel_data, and calls
RemotePEP503Backend.fetch with the matching ArtifactInfo checksum. Assert the
returned file contains the downloaded data, covering the unlink-and-redownload
branch when the cached SHA256 mismatches.
🪄 Autofix (Beta)
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: b854f02a-09f6-4545-bb03-60a0ac6ad701
📒 Files selected for processing (13)
docs/proposals/unified-cache-manager.rstpyproject.tomlsrc/fromager/bootstrapper/_cache.pysrc/fromager/bootstrapper/_prepare_source.pysrc/fromager/cache.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/cache_cmd.pysrc/fromager/context.pysrc/fromager/requirements_file.pytests/test_bootstrapper_iterative.pytests/test_cache.pytests/test_cli.pytests/test_commands.py
🚧 Files skipped from review as they are similar to previous changes (10)
- pyproject.toml
- docs/proposals/unified-cache-manager.rst
- tests/test_cli.py
- src/fromager/context.py
- src/fromager/requirements_file.py
- src/fromager/bootstrapper/_cache.py
- tests/test_commands.py
- src/fromager/bootstrapper/_prepare_source.py
- tests/test_bootstrapper_iterative.py
- src/fromager/commands/bootstrap.py
91d080e to
c238f19
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
tests/test_cache.py (3)
1139-1163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name claims
allow_insecurebut never asserts it propagates.Only backend ordering/type is checked; passing
allow_insecure=Falsewould pass identically.💚 Suggested addition
assert isinstance(manager.lookup_backends[-1], RemotePEP503Backend) + remote = manager.lookup_backends[-1] + assert remote._allow_insecure is TrueUse whatever the actual attribute/property name is on
RemotePEP503Backend. As per path instructions, "Verify test actually tests the intended behavior."🤖 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 `@tests/test_cache.py` around lines 1139 - 1163, Update test_adds_remote_backend_with_allow_insecure to inspect the created RemotePEP503Backend and assert its actual allow-insecure attribute or property is enabled. Keep the existing backend ordering and type assertions, ensuring the test fails when build_cache_manager ignores allow_insecure=True.Source: Path instructions
1021-1028: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
"requests" in result.outputdoesn't validate the JSON output.The
--format jsonpath is only checked for a substring that also matches the wheel filename. Parse it so the serialization contract is actually asserted.💚 Suggested strengthening
result = runner.invoke(cache_cli, ["list", "--format", "json"], obj=wkctx) assert result.exit_code == 0 - assert "requests" in result.output + entries = json.loads(result.output) + assert [e["filename"] for e in entries] == [whl.name]Adjust key names to match the command's payload. As per path instructions, "Verify test actually tests the intended behavior."
🤖 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 `@tests/test_cache.py` around lines 1021 - 1028, Update test_cache_list_with_wheels to parse result.output as JSON and assert the command’s structured payload using the actual key names exposed by cache_cli’s --format json response. Replace the substring assertion with checks that verify the expected wheel entry and its serialized fields.Source: Path instructions
592-616: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlso assert the corrupt download isn't left in
dest.The raise is verified, but nothing checks that the partially/incorrectly downloaded bytes were cleaned up — a leftover
requests-2.31.0-1-py3-none-any.whlin the downloads dir would be silently reused by later runs.💚 Suggested addition
with pytest.raises(ValueError, match="sha256 mismatch"): backend.fetch(key, info, dest) + assert not (dest / info.filename).exists() + assert list(dest.iterdir()) == []🤖 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 `@tests/test_cache.py` around lines 592 - 616, Extend test_fetch_rejects_sha256_mismatch to assert that dest does not contain the expected requests-2.31.0-1-py3-none-any.whl after backend.fetch raises ValueError, verifying corrupt downloads are removed.tests/test_bootstrapper.py (1)
1166-1170: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAssert the
build_tagargument too.
args[2]is the one value derived at call time (pbi.build_tag(resolved_version)); leaving it unchecked means a regression there passes silently.💚 Suggested addition
assert args[1] == Version("1.0") + assert args[2] == tmp_context.package_build_info(wi.req).build_tag(Version("1.0")) assert args[3] == wheel_filename🤖 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 `@tests/test_bootstrapper.py` around lines 1166 - 1170, Extend the assertions for cache.store_wheel in the relevant test to validate args[2], ensuring it equals the build tag produced by pbi.build_tag(resolved_version). Keep the existing assertions for the request, version, and wheel filename unchanged.src/fromager/commands/cache_cmd.py (1)
173-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
--dry-runtoinvalidatefor parity withgc.
invalidatedeletes files with no preview mechanism, unlikegc's--dry-run. Given this is also a destructive, irreversible operation, offering a preview would reduce accidental data loss.♻️ Proposed fix
`@cache_cli.command`() `@click.argument`("packages", nargs=-1) `@click.option`( "--all", "invalidate_all", is_flag=True, default=False, help="Invalidate the entire cache.", ) +@click.option( + "--dry-run", + is_flag=True, + default=False, + help="Show what would be removed without actually deleting.", +) `@click.pass_obj` def invalidate( wkctx: context.WorkContext, packages: tuple[str, ...], invalidate_all: bool, + dry_run: bool, ) -> None: ... for _key, info in keys_to_remove: file_path = pathlib.Path(info.url_or_path) - if file_path.exists(): - file_path.unlink() - logger.info("removed %s", file_path) + if dry_run: + click.echo(f" would remove: {file_path}") + else: + if file_path.exists(): + file_path.unlink() + logger.info("removed %s", file_path) removed += 1 - # Re-scan after removing files to update the index - if keys_to_remove: + # Re-scan after removing files to update the index + if keys_to_remove and not dry_run: backend.scan() - click.echo(f"Invalidated {removed} cached artifact(s).") + verb = "Would invalidate" if dry_run else "Invalidated" + click.echo(f"{verb} {removed} cached artifact(s).")🤖 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 `@src/fromager/commands/cache_cmd.py` around lines 173 - 220, Add a --dry-run flag to the invalidate command and its invalidate function, then skip file deletion, logging, and backend rescanning when enabled while still computing and reporting the artifacts that would be invalidated. Match the existing gc dry-run option’s naming, CLI behavior, and output conventions.src/fromager/bootstrapper/_prepare_source.py (1)
151-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
store_wheelon the cached path double-counts store stats.
lookup_wheel()already registered/promoted this wheel into the store backend before returning it (_find_cached_wheel_via_manager), so re-storing here only adds a secondrecord_storeevent, skewingCacheStats.stores/summary(). Theindex_wheelcall still makes sense.Proposed change
if prepared.cached_wheel_filename is not None and bt.ctx.cache is not None: - pbi = bt.ctx.package_build_info(wi.req) - build_tag = pbi.build_tag(wi.resolved_version) - bt.ctx.cache.store_wheel( - wi.req, - wi.resolved_version, - build_tag, - prepared.cached_wheel_filename, - ) # Wheel is already in downloads; index it without sweeping wheels_build. server.index_wheel(bt.ctx, prepared.cached_wheel_filename)🤖 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 `@src/fromager/bootstrapper/_prepare_source.py` around lines 151 - 161, Remove the redundant cache.store_wheel call from the cached-wheel branch in the preparation flow, while preserving server.index_wheel for indexing the already-stored wheel. Keep the existing cache and filename guards unchanged, and do not alter non-cached wheel handling.
🤖 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 `@src/fromager/cache.py`:
- Around line 814-821: Update the relative-URL resolution logic around the href
checks to detect HTTP and HTTPS schemes case-insensitively, while preserving the
original href as the absolute URL. Ensure uppercase or mixed-case scheme values
bypass base_url concatenation and resolve as valid absolute URLs.
In `@src/fromager/server.py`:
- Around line 61-74: Make _link_wheel_into_index safe for concurrent callers
targeting the same symlink path. Use a lock shared by index_wheel and
update_wheel_mirror, or handle FileExistsError from symlink_to as successful
completion when another caller has already created the link, while preserving
existing dangling-link cleanup and current-link behavior.
---
Nitpick comments:
In `@src/fromager/bootstrapper/_prepare_source.py`:
- Around line 151-161: Remove the redundant cache.store_wheel call from the
cached-wheel branch in the preparation flow, while preserving server.index_wheel
for indexing the already-stored wheel. Keep the existing cache and filename
guards unchanged, and do not alter non-cached wheel handling.
In `@src/fromager/commands/cache_cmd.py`:
- Around line 173-220: Add a --dry-run flag to the invalidate command and its
invalidate function, then skip file deletion, logging, and backend rescanning
when enabled while still computing and reporting the artifacts that would be
invalidated. Match the existing gc dry-run option’s naming, CLI behavior, and
output conventions.
In `@tests/test_bootstrapper.py`:
- Around line 1166-1170: Extend the assertions for cache.store_wheel in the
relevant test to validate args[2], ensuring it equals the build tag produced by
pbi.build_tag(resolved_version). Keep the existing assertions for the request,
version, and wheel filename unchanged.
In `@tests/test_cache.py`:
- Around line 1139-1163: Update test_adds_remote_backend_with_allow_insecure to
inspect the created RemotePEP503Backend and assert its actual allow-insecure
attribute or property is enabled. Keep the existing backend ordering and type
assertions, ensuring the test fails when build_cache_manager ignores
allow_insecure=True.
- Around line 1021-1028: Update test_cache_list_with_wheels to parse
result.output as JSON and assert the command’s structured payload using the
actual key names exposed by cache_cli’s --format json response. Replace the
substring assertion with checks that verify the expected wheel entry and its
serialized fields.
- Around line 592-616: Extend test_fetch_rejects_sha256_mismatch to assert that
dest does not contain the expected requests-2.31.0-1-py3-none-any.whl after
backend.fetch raises ValueError, verifying corrupt downloads are removed.
🪄 Autofix (Beta)
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: b76ef78a-6811-45f2-b35e-09ddc46f9ab4
📒 Files selected for processing (17)
docs/proposals/unified-cache-manager.rstpyproject.tomlsrc/fromager/bootstrapper/_build.pysrc/fromager/bootstrapper/_cache.pysrc/fromager/bootstrapper/_prepare_source.pysrc/fromager/cache.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/cache_cmd.pysrc/fromager/context.pysrc/fromager/requirements_file.pysrc/fromager/server.pytests/test_bootstrapper.pytests/test_bootstrapper_iterative.pytests/test_cache.pytests/test_cli.pytests/test_commands.pytests/test_server.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/test_commands.py
- src/fromager/requirements_file.py
- pyproject.toml
- tests/test_cli.py
- docs/proposals/unified-cache-manager.rst
- src/fromager/commands/bootstrap.py
- tests/test_bootstrapper_iterative.py
…lookup Introduces a CacheManager subsystem that centralizes cache operations behind a prioritized backend search. Multiple backends (local directories, remote PEP 503 servers) are searched in order; stores go to a single designated local backend. Key design decisions: - Flat ordered backend list (no collection/routing abstraction) - Thread-safe with internal locking on backend indices and CacheStats - Graceful fallback: fetch failures continue to next backend - SHA256 verification for remote downloads, atomic writes - Empty expected build tag matches any tagged wheel (legacy find_wheel) - Symlink-aware scan to prevent gc/invalidate from escaping root - items() protocol method for CLI access (no _index access) - Factory function in cache.py (not CLI module) Integration: - PrepareSource short-circuit to ProcessInstallDeps on CacheManager hits - --use-cache-manager and --cache-allow-insecure CLI options - cache CLI group: list, stats, verify, invalidate, gc Co-Authored-By: Claude <claude@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Sean Pryor <spryor@redhat.com>
c238f19 to
644e945
Compare
3 participants
Summary
Adds a unified
CacheManagerthat centralizes wheel cache lookup and store behind a prioritized multi-backend search. This is cache manager only — variant/collection routing is intentionally deferred.Architecture proposal:
docs/proposals/unified-cache-manager.rstWhat
CacheManagerwith ordered local + optional remote PEP 503 backendswheels-repo/downloads/) — no collection/routing abstractionPrepareSourceshort-circuit toProcessInstallDepson cache hits (skips source download, build env, build deps)fromager cache list|stats|verify|invalidate|gc--use-cache-manager,--cache-allow-insecureWhy
Out of scope (follow-up)
CacheCollection,StoreRouter, etc.)Test plan
Unit tests for backends, manager, CLI, and PrepareSource short-circuit
Local E2E with walkerpass4 + PEP 503 mirror (cache hits, 0 source downloads/builds)
CI / reviewer validation with
--use-cache-managerPR follows CONTRIBUTING.md guidelines