feat(resolver): add versionmap-git source resolver - #1197
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughA new 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: 1
🧹 Nitpick comments (1)
docs/reference/config-reference.rst (1)
44-45: ⚡ Quick winAdd a
versionaddeddirective for this new user-facing resolver entry.Line 44 introduces a new config model in user-facing docs, but there’s no explicit version marker in this section. Please add
.. versionadded::next to this entry for release traceability.Suggested patch
.. autopydantic_model:: VersionMapGitResolver + + .. versionadded:: 0.79.0As per coding guidelines: "Use Sphinx
versionadded,versionremoved,versionchangeddirectives for user-facing changes".🤖 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/reference/config-reference.rst` around lines 44 - 45, The VersionMapGitResolver autopydantic_model directive in the config reference documentation is missing a versionadded directive to track when this new user-facing resolver was introduced. Add a `.. versionadded::` directive before the `.. autopydantic_model:: VersionMapGitResolver` line, specifying the appropriate version number where this resolver was first available, to comply with the documentation guidelines for user-facing changes.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/packagesettings/_resolver.py`:
- Around line 517-522: The validate_clone_url method in the Resolver class
currently accepts URLs with only a host and no repository path because Pydantic
v2 normalizes the path to "/" which is truthy. Add an additional validation
check after the existing path validation to explicitly reject URLs where the
path is only "/" or is root-only. This will prevent invalid git URLs that lack
an actual repository path from passing validation.
---
Nitpick comments:
In `@docs/reference/config-reference.rst`:
- Around line 44-45: The VersionMapGitResolver autopydantic_model directive in
the config reference documentation is missing a versionadded directive to track
when this new user-facing resolver was introduced. Add a `.. versionadded::`
directive before the `.. autopydantic_model:: VersionMapGitResolver` line,
specifying the appropriate version number where this resolver was first
available, to comply with the documentation guidelines for user-facing changes.
🪄 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: 48b234c8-c0ab-46c3-a2e3-b4b237852236
📒 Files selected for processing (5)
docs/reference/config-reference.rstsrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_resolver.pytests/test_packagesettings_resolver.pytests/test_resolver.py
5ad9e22 to
6a33ca3
Compare
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
I have left a few comments. Also, please use the linked issue on GitHub as "closes". We should not mention anything related to downstream (AIPCC in this case).
Also, I would appreciate if @tiran and / or @LalatenduMohanty takes a look as well.
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 `@tests/test_packagesettings_resolver.py`:
- Around line 449-455: The current resolver test only verifies git URL rewriting
in resolver_provider and does not cover the new build_sdist contract. Update the
test coverage around resolver_provider and the build path so a versionmap-git
config with build_sdist: tarball is exercised end-to-end, then assert that the
selected sdist strategy comes from sources.build_sdist rather than only checking
VersionMapProvider.version_map. Use the existing resolver_provider, WorkContext,
and sources.build_sdist symbols to locate the behavior and make sure the
regression test fails if build_sdist is ignored downstream.
🪄 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: 3ffdc863-d735-4be6-8304-fbc175a59b48
📒 Files selected for processing (5)
docs/reference/config-reference.rstsrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_resolver.pytests/test_packagesettings_resolver.pytests/test_resolver.py
✅ Files skipped from review due to trivial changes (1)
- docs/reference/config-reference.rst
🚧 Files skipped from review as they are similar to previous changes (3)
- src/fromager/packagesettings/init.py
- src/fromager/packagesettings/_resolver.py
- tests/test_resolver.py
Add `VersionMapGitResolver` config model for the `versionmap-git` provider from the new-resolver-config proposal. Maps version numbers to git refs (commit SHAs or ref paths) with a separate `clone_url`, replacing the need for handwritten Python plugins. - Add `VersionMapGitResolver` pydantic model with `clone_url`, `build_sdist`, and `versionmap` fields - Register in `SourceResolver` discriminated union - Add to config reference docs - Add tests for config parsing, provider output, and validation Closes: python-wheel-build#1180 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Josef Skladanka <jskladan@redhat.com>
|
@rd4398 everything should be addressed. I'm also not sure what to do about the fialing coverage report though. I ran hatch run test:coverage erase
hatch run test:test --cov --cov-report=term-missing --cov-config=pyproject.toml -x -qon both my branch and the current main, and the branch adds 70 statements and 4 branches (new code in the resolver), while actually reducing misses by 1. Coverage is identical at 86% on both branches.
I don't see any coverage regression on my part, but I obviously don't have all the e3e data the CI coverage job combines for the |
|
@jskladan this needs a rebase on top of main. We fixed this coverage issue couple of weeks back |
| version_map = VersionMap(url_map) # type: ignore[arg-type] | ||
| return resolver.VersionMapProvider( | ||
| version_map=version_map, | ||
| package_name=None, |
There was a problem hiding this comment.
[Recommendation]
src/fromager/packagesettings/_resolver.py(~line 535,resolver_providerpassespackage_name=None)tests/test_resolver.py(~lines 938 & 972, tests passpackage_name="testpkg")
The code invocation always passes package_name=None. But the tests always pass package_name="testpkg". So the tests are checking a situation that never actually happens. If the "no name" case is broken, every test still passes and nobody notices.
Fix: add a test that uses the code the way it really runs — with package_name=None —
and actually resolves a package to make sure it works.
| if value.scheme not in {"https", "ssh"}: | ||
| raise ValueError(f"invalid scheme in url {value}") | ||
| if not value.path: | ||
| if not value.path or value.path == "/": |
There was a problem hiding this comment.
[Recommendation]
src/fromager/packagesettings/_resolver.py(~line 237,PyPIGitResolver.validate_clone_url)src/fromager/packagesettings/_resolver.py(~lines 518–523,VersionMapGitResolver.validate_clone_url)
The exact same check exists in two classes, word for word. This PR had to fix the same bug in both copies.
Fix: put the check in one shared helper function and have both classes call it.
| if value.scheme not in {"https", "ssh"}: | ||
| raise ValueError(f"invalid scheme in url {value}") | ||
| if not value.path: | ||
| if not value.path or value.path == "/": |
There was a problem hiding this comment.
[Recommendation]
The PR's stated goal is adding a new resolver, but it also alters the existing PyPIGitResolver by adding the value.path == "/" rejection — a behavior change to shipped code. The diff only adds an empty-path test to the new class, so the fix on PyPIGitResolver ships with no regression test and a later refactor could silently revert it.
Fix: split the PyPIGitResolver change into its own commit/PR with a dedicated test, or keep
it here but add test_clone_url_rejects_root_path under the PyPIGitResolver suite.
| ) -> resolver.VersionMapProvider: | ||
| clone_url = str(self.clone_url) | ||
| url_map = { | ||
| ver: f"git+{clone_url}@{ref}" for ver, ref in self.versionmap.items() |
There was a problem hiding this comment.
[Recommendation]
The ref values from versionmap are dropped straight into f"git+{clone_url}@{ref}" with no validation. A ref containing @, whitespace, or a newline could produce a malformed URL. It's operator-controlled
config so the risk is low, but a quick check would add defense-in-depth.
Validate refs against a simple allowlist in a field_validator("versionmap").
After:
@pydantic.field_validator("versionmap", mode="after")
@classmethod
def validate_refs(cls, value: dict[str, str]) -> dict[str, str]:
for ref in value.values():
if not re.fullmatch(r"[\w./-]+", ref):
raise ValueError(f"invalid git ref {ref!r}")
return value| build_sdist: BuildSDist = BuildSDist.pep517 | ||
| """Source distribution build method.""" | ||
|
|
||
| versionmap: dict[str, str] |
There was a problem hiding this comment.
[Recommendation] — src/fromager/packagesettings/_resolver.py (~line 513, versionmap field) and tests/test_packagesettings_resolver.py
An empty versionmap: {} passes validation, produces an empty VersionMap, and only fails later at resolve time with an opaque "no match" instead of a clear config error. There's also no positive test that the
ssh scheme is accepted (only the http-rejection negative), and no test for the empty-map case.
Reject an empty map with min_length=1, and add the two missing tests.
Before:
versionmap: dict[str, str]After:
versionmap: dict[str, str] = pydantic.Field(min_length=1)There was a problem hiding this comment.
There should also be a unit test case that ensure the empty versionmap dictionary fails.
| build_sdist: BuildSDist = BuildSDist.pep517 | ||
| """Source distribution build method.""" | ||
|
|
||
| versionmap: dict[str, str] |
There was a problem hiding this comment.
There should also be a unit test case that ensure the empty versionmap dictionary fails.
|
|
||
| The ``versionmap-git`` provider maps known version numbers to known git | ||
| refs (commit SHAs or ref paths such as ``refs/tags/1.1``). It clones a | ||
| git repo at the configured ref and builds an sdist with PEP 517. |
There was a problem hiding this comment.
[Nit]
The docstring says the provider "builds an sdist with PEP 517," which reads as fixed. But build_sdist is a configurable field where pep517 is only the default — the wording will mislead anyone configuring a
non-default build method.
Before:
git repo at the configured ref and builds an sdist with PEP 517.After:
git repo at the configured ref and builds an sdist using the configured
``build_sdist`` method (default: PEP 517).| build_sdist: BuildSDist = BuildSDist.pep517 | ||
| """Source distribution build method.""" | ||
|
|
||
| versionmap: dict[str, str] |
There was a problem hiding this comment.
[Nit]
The field name versionmap breaks snake_case consistency with its siblings (clone_url, build_sdist), and the code internally uses version_map/VersionMap. Because the field name is the user-facing YAML
key, the single-word form becomes a permanent API-consistency wart.
Before:
versionmap: dict[str, str]After:
version_map: dict[str, str] = pydantic.Field(alias="versionmap") # if the YAML key must stay| build_sdist: BuildSDist = BuildSDist.pep517 | ||
| """Source distribution build method.""" | ||
|
|
||
| versionmap: dict[str, str] |
There was a problem hiding this comment.
[Question]
Are the versionmap keys validated as PEP 440 versions anywhere? They become version strings in VersionMap, so a typo key like 'v1.0' or 'latest' is accepted at config-load time and would only blow up
later when parsed as a Version. If VersionMap.__init__ doesn't validate keys eagerly, consider a field_validator calling packaging.version.Version(k) per key to fail fast.
| url_map = { | ||
| ver: f"git+{clone_url}@{ref}" for ver, ref in self.versionmap.items() | ||
| } | ||
| version_map = VersionMap(url_map) # type: ignore[arg-type] |
There was a problem hiding this comment.
[Question] — src/fromager/packagesettings/_resolver.py (~line 532, resolver_provider)
VersionMap(url_map) # type: ignore[arg-type] — the type: ignore implies VersionMap is declared to accept something other than dict[str, str]. Runtime tests show a plain str→str mapping works, so the
contract seems satisfied, but the ignore hides whether VersionMap does any key/value coercion this call bypasses. Can you confirm VersionMap.__init__'s real signature rather than suppressing the type error
— or add the proper type so the ignore can be dropped?
| refs (commit SHAs or ref paths such as ``refs/tags/1.1``). It clones a | ||
| git repo at the configured ref and builds an sdist with PEP 517. | ||
|
|
||
| .. versionadded:: 0.79.0 |
There was a problem hiding this comment.
Why are we adding docstring annotation about when something was added? This is what release notest and git history are for.
3 participants
Add
VersionMapGitResolverconfig model for theversionmap-gitprovider from the new-resolver-config proposal. Maps version numbers to git refs (commit SHAs or ref paths) with a separateclone_url, replacing the need for handwritten Python plugins.VersionMapGitResolverpydantic model withclone_url,build_sdist, andversionmapfieldsSourceResolverdiscriminated unionCloses: #1180