feat: source resolver configuration#1052
feat: source resolver configuration#1052tiran wants to merge 1 commit intopython-wheel-build:mainfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds a new source-resolver subsystem to fromager.packagesettings: a new _resolver module defining multiple Pydantic resolver models (PyPI: sdist/prebuilt/download/git; GitHub/GitLab tag download/clone; version-map; NotAvailableResolver; HookResolver) and a BuildSDist enum plus DEFAULT_TAG_MATCHER. Exports those types from packagesettings.init. VariantInfo and PackageSettings gain an optional discriminated source field; VariantInfo.pre_build is renamed to pre_built. PackageBuildInfo gains a source_resolver property. Tests and YAML test fixtures are added/updated for each resolver. VersionMapProvider signature/behavior was adjusted to allow package_name=None and disable caching. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
The tests are going to fail. I have not updated the test cases and test configs. |
tiran
force-pushed
the
new-resolver-config2
branch
from
599609d to
976deff
Compare
tiran
force-pushed
the
new-resolver-config2
branch
from
976deff to
19a9602
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/fromager/packagesettings/_models.py (1)
338-342: 💤 Low value
sourcefield missing Python-level= None— may confuse type checkersPydantic v2 reads the
default=NonefromField()insideAnnotated, so runtime behavior is correct. However, unlike all other nullable fields inVariantInfoandPackageSettings(which use= Noneexplicitly), this field has no Python-level default assignment. Static type checkers (e.g., pyright) may flag it as a required field.♻️ Proposed fix (apply to both `VariantInfo` and `PackageSettings`)
- source: typing.Annotated[ - SourceResolver | None, - pydantic.Field(default=None, discriminator="provider"), - ] + source: typing.Annotated[ + SourceResolver | None, + pydantic.Field(default=None, discriminator="provider"), + ] = None🤖 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/packagesettings/_models.py` around lines 338 - 342, The Annotated field declaration for source in _models.py (the "source" field on VariantInfo and PackageSettings) lacks a Python-level default assignment which can make static type checkers treat it as required; update the field declaration to include an explicit Python default (add " = None" to the Annotated[...] expression for the source attribute in both VariantInfo and PackageSettings) while keeping the existing pydantic.Field(default=None, discriminator="provider") inside the Annotated so runtime behavior remains unchanged.src/fromager/packagesettings/__init__.py (1)
16-30: 💤 Low valueConsider exporting
SourceResolverfor external type annotationsThe
SourceResolverdiscriminated union is used as the field type inVariantInfoandPackageSettings, but it's not re-exported from the package's public API. External callers who want to annotate against it would have to reach intofromager.packagesettings._resolver.♻️ Proposed addition
from ._resolver import ( DEFAULT_TAG_MATCHER, BuildSDist, GitHubTagCloneResolver, GitHubTagDownloadResolver, GitLabTagCloneResolver, GitLabTagDownloadResolver, HookResolver, NotAvailableResolver, PyPIDownloadResolver, PyPIGitResolver, PyPIPrebuiltResolver, PyPISDistResolver, + SourceResolver, VersionMapResolver, )And add
"SourceResolver"to__all__.🤖 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/packagesettings/__init__.py` around lines 16 - 30, The package does not re-export the SourceResolver discriminated union used by VariantInfo and PackageSettings, forcing consumers to import from fromager.packagesettings._resolver; update src/fromager/packagesettings/__init__.py to import SourceResolver from ._resolver (alongside DEFAULT_TAG_MATCHER, BuildSDist, etc.) and add "SourceResolver" to the module's __all__ so callers can annotate against SourceResolver via fromager.packagesettings.SourceResolver.
🤖 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 247-281: VersionMapResolver currently relies on
AbstractResolver.resolver_provider which raises NotImplementedError; add an
explicit override on the VersionMapResolver class that either implements the
provider or immediately raises NotImplementedError("versionmap-git not yet
implemented") to make behavior explicit (i.e., define a resolver_provider method
on VersionMapResolver that raises the clear NotImplementedError string or
implements the provider logic), referencing the VersionMapResolver class and
AbstractResolver.resolver_provider to locate where to add the
stub/implementation.
- Around line 404-432: The resolver_provider methods currently pass the literal
"FIXME" as override_download_url which silently produces broken providers;
instead, update each resolver_provider (e.g.,
GitHubTagCloneResolver.resolver_provider and
GitHubTagDownloadResolver.resolver_provider, and do the same for
GitLabTagDownloadResolver and GitLabTagCloneResolver) to fail fast by raising
NotImplementedError with a clear message like "override_download_url template
not implemented for <ResolverClassName>" rather than calling
self._github_provider(...) (or self._gitlab_provider(...)) with the "FIXME"
string.
In `@tests/test_packagesettings.py`:
- Around line 944-975: The four commented-out assertions for matcher_factory
leave test coverage incomplete for the tag-based resolver fixtures (see
TEST_GITHUB_DOWNLOAD, TEST_GITHUB_GIT, TEST_GITLAB_DOWNLOAD, TEST_GITLAB_GIT);
either uncomment the matcher_factory checks or explicitly assert the serialized
JSON omits matcher_factory depending on intended behavior: if matcher_factory
should be present assert it equals
"fromager.packagesettings:DEFAULT_TAG_MATCHER" for each of those fixtures,
otherwise update the test to assert the key is absent/None and remove the
commented lines to reflect the intended serialization.
---
Nitpick comments:
In `@src/fromager/packagesettings/__init__.py`:
- Around line 16-30: The package does not re-export the SourceResolver
discriminated union used by VariantInfo and PackageSettings, forcing consumers
to import from fromager.packagesettings._resolver; update
src/fromager/packagesettings/__init__.py to import SourceResolver from
._resolver (alongside DEFAULT_TAG_MATCHER, BuildSDist, etc.) and add
"SourceResolver" to the module's __all__ so callers can annotate against
SourceResolver via fromager.packagesettings.SourceResolver.
In `@src/fromager/packagesettings/_models.py`:
- Around line 338-342: The Annotated field declaration for source in _models.py
(the "source" field on VariantInfo and PackageSettings) lacks a Python-level
default assignment which can make static type checkers treat it as required;
update the field declaration to include an explicit Python default (add " =
None" to the Annotated[...] expression for the source attribute in both
VariantInfo and PackageSettings) while keeping the existing
pydantic.Field(default=None, discriminator="provider") inside the Annotated so
runtime behavior remains unchanged.
🪄 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: 42e22219-5705-4c79-b697-560c94d2a617
📒 Files selected for processing (14)
docs/reference/config-reference.rstsrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/packagesettings/_resolver.pytests/test_packagesettings.pytests/testdata/context/overrides/settings/test_github-download.yamltests/testdata/context/overrides/settings/test_github-git.yamltests/testdata/context/overrides/settings/test_gitlab-download.yamltests/testdata/context/overrides/settings/test_gitlab-git.yamltests/testdata/context/overrides/settings/test_pypidownload.yamltests/testdata/context/overrides/settings/test_pypigit.yamltests/testdata/context/overrides/settings/test_pypiprebuilt.yamltests/testdata/context/overrides/settings/test_pypisdist.yaml
| ( | ||
| TEST_GITHUB_DOWNLOAD, | ||
| { | ||
| "provider": "github-tag-download", | ||
| # "matcher_factory": "", | ||
| "project_url": "https://github.com/python-wheel-build/fromager", | ||
| }, | ||
| ), | ||
| ( | ||
| TEST_GITHUB_GIT, | ||
| { | ||
| "provider": "github-tag-git", | ||
| # "matcher_factory": "", | ||
| "project_url": "https://github.com/python-wheel-build/fromager", | ||
| }, | ||
| ), | ||
| ( | ||
| TEST_GITLAB_DOWNLOAD, | ||
| { | ||
| "provider": "gitlab-tag-download", | ||
| # "matcher_factory": "", | ||
| "project_url": "https://gitlab.test/python-wheel-build/fromager", | ||
| }, | ||
| ), | ||
| ( | ||
| TEST_GITLAB_GIT, | ||
| { | ||
| "provider": "gitlab-tag-git", | ||
| # "matcher_factory": "", | ||
| "project_url": "https://gitlab.test/python-wheel-build/fromager", | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Commented-out matcher_factory assertions leave coverage incomplete.
The YAML fixtures for all four tag-based resolvers explicitly set matcher_factory: fromager.packagesettings:DEFAULT_TAG_MATCHER, but lines 948, 957, 963, and 973 are commented out. Either matcher_factory is intentionally excluded from JSON serialization (in which case remove the comments), or the assertions are missing and the serialized value should be verified.
🤖 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_packagesettings.py` around lines 944 - 975, The four commented-out
assertions for matcher_factory leave test coverage incomplete for the tag-based
resolver fixtures (see TEST_GITHUB_DOWNLOAD, TEST_GITHUB_GIT,
TEST_GITLAB_DOWNLOAD, TEST_GITLAB_GIT); either uncomment the matcher_factory
checks or explicitly assert the serialized JSON omits matcher_factory depending
on intended behavior: if matcher_factory should be present assert it equals
"fromager.packagesettings:DEFAULT_TAG_MATCHER" for each of those fixtures,
otherwise update the test to assert the key is absent/None and remove the
commented lines to reflect the intended serialization.
19a9602 to
4c9f9fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/fromager/packagesettings/_resolver.py (1)
519-523:HookResolveris unimplemented — offer to helpThe
TODOandraise NotImplementedErrormean any config usingprovider: hookwill always fail.Happy to implement the hook-dispatch logic if you want to open a tracking issue for it.
🤖 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/packagesettings/_resolver.py` around lines 519 - 523, The resolver_provider method currently raises NotImplementedError for provider dispatch; implement it to return the hook provider when config requests provider: "hook" by instantiating and returning the HookResolver (or the project's resolver.HookResolver) with the provided ctx and req_type, otherwise delegate to the existing default provider factory/logic—replace the TODO/raise with a conditional that constructs resolver.HookResolver(ctx, req_type) for hook providers and falls back to the existing resolver selection path (returning an instance of resolver.BaseProvider).
🤖 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 439-446: The override_download_url passed from
GitHubTagCloneResolver.resolver_provider and
GitLabTagCloneResolver.resolver_provider currently uses a plain string with
"{self.project_url}" / "{self.clone_url}" which causes str.format to look up a
nonexistent "self" key; change those literals to f-strings (prefix with f) so
self.project_url or self.clone_url is interpolated first (e.g.,
f"git+{self.project_url}@{{tagname}}" and f"git+{self.clone_url}@{{tagname}}")
before the provider calls .format(...).
---
Nitpick comments:
In `@src/fromager/packagesettings/_resolver.py`:
- Around line 519-523: The resolver_provider method currently raises
NotImplementedError for provider dispatch; implement it to return the hook
provider when config requests provider: "hook" by instantiating and returning
the HookResolver (or the project's resolver.HookResolver) with the provided ctx
and req_type, otherwise delegate to the existing default provider
factory/logic—replace the TODO/raise with a conditional that constructs
resolver.HookResolver(ctx, req_type) for hook providers and falls back to the
existing resolver selection path (returning an instance of
resolver.BaseProvider).
🪄 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: 8fe16787-0813-4bdb-9b85-a04d5522f92d
📒 Files selected for processing (15)
docs/reference/config-reference.rstsrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/packagesettings/_resolver.pysrc/fromager/resolver.pytests/test_packagesettings.pytests/testdata/context/overrides/settings/test_github-download.yamltests/testdata/context/overrides/settings/test_github-git.yamltests/testdata/context/overrides/settings/test_gitlab-download.yamltests/testdata/context/overrides/settings/test_gitlab-git.yamltests/testdata/context/overrides/settings/test_pypidownload.yamltests/testdata/context/overrides/settings/test_pypigit.yamltests/testdata/context/overrides/settings/test_pypiprebuilt.yamltests/testdata/context/overrides/settings/test_pypisdist.yaml
✅ Files skipped from review due to trivial changes (11)
- tests/testdata/context/overrides/settings/test_gitlab-download.yaml
- tests/testdata/context/overrides/settings/test_pypidownload.yaml
- tests/testdata/context/overrides/settings/test_gitlab-git.yaml
- tests/testdata/context/overrides/settings/test_github-git.yaml
- tests/testdata/context/overrides/settings/test_pypisdist.yaml
- tests/testdata/context/overrides/settings/test_pypiprebuilt.yaml
- src/fromager/packagesettings/_pbi.py
- tests/testdata/context/overrides/settings/test_pypigit.yaml
- docs/reference/config-reference.rst
- src/fromager/packagesettings/_models.py
- tests/test_packagesettings.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/testdata/context/overrides/settings/test_github-download.yaml
- src/fromager/packagesettings/init.py
Introduce new configuration for source resolver and downloads. The new
system uses profiles for common tasks:
- `pypi-sdist`: resolve versions of sdists from PyPI, download sdist
- `pypi-prebuilt`: resolve versions of platform wheels from PyPI,
download pre-built wheel from PyPI.
- `pypi-download`: resolve versions of any package from PyPI, download
from external URL (with `{version}` variable)
- `pypi-git`: resolve versions for any package from PyPI, git clone
(with `{version}` variable)
- `versionmap-git`: resolve with a version map, git clone
- `github-tag-download`: resolve from GitHub tag, download tarball
- `github-tag-git`: resolve from GitHub tag, git clone
- `gitlab-tag-download`: resolve from GitLab tag, download tarball
- `gitlab-tag-git`: resolve from GitLab tag, git clone
- `not-available`: block resolving
- `hook`: call Fromager hooks
The new settings will eventually replace `download_source`,
`resolver_dist`, and `git_options` top-level options as well as
`wheel_server_url` and `pre_built` flags for variants.
Signed-off-by: Christian Heimes <cheimes@redhat.com>
tiran
force-pushed
the
new-resolver-config2
branch
from
4c9f9fb to
5d955ff
Compare
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
This looks mostly okay. I have left a few comments / suggestions.
I will prefer if @LalatenduMohanty and / or @dhellmann takes a look as well
| ) | ||
|
|
||
|
|
||
| class NotAvailableResolver(pydantic.BaseModel): |
There was a problem hiding this comment.
It implements resolver_provider() with the same signature but aren't part of the type hierarchy
Can we make it inherit from AbstractResolver for consistency?
| raise ValueError("package is not available") | ||
|
|
||
|
|
||
| class HookResolver(pydantic.BaseModel): |
There was a problem hiding this comment.
Same here,
it implements resolver_provider() with the same signature but aren't part of the type hierarchy
Can we make it inherit from AbstractResolver for consistency?
| req_type: requirements_file.RequirementType, | ||
| override_download_url: str | None = None, | ||
| ) -> resolver.GitLabTagProvider: | ||
| assert self.project_url.path # for type checker |
There was a problem hiding this comment.
Can we make this an explicit if not ... raise ValueError(...) like the GitHub equivalent does?
Something like :
if not self.project_url.path or self.project_url.path.count("/") != 2:
raise ValueError(
f"Invalid path in {self.project_url}, expected two elements"
)
| self, ctx: context.WorkContext, req_type: requirements_file.RequirementType | ||
| ) -> resolver.BaseProvider: | ||
| return resolver.VersionMapProvider( | ||
| version_map=versionmap.VersionMap(self.version_map), # type: ignore |
There was a problem hiding this comment.
This suppresses what's likely a real type mismatch (dict[PackageVersion, str] vs expected VersionMap constructor arg).
Can we add a proper cast/conversion, or at minimum use a scoped # type: ignore[arg-type] with a comment?
| def resolver_provider( | ||
| self, ctx: context.WorkContext, req_type: requirements_file.RequirementType | ||
| ) -> resolver.BaseProvider: | ||
| # TODO |
There was a problem hiding this comment.
Users who configure provider: hook will get an unhelpful crash. How about we add an error message something like "hook resolver is not yet implemented"?
| build_sdist: BuildSDist = BuildSDist.pep517 | ||
| """Source distribution build method""" | ||
|
|
||
| min_release_age: int | None = pydantic.Field(default=None, ge=0) |
There was a problem hiding this comment.
Non blocker: The min_release_age field with identical definition and docstring appears on both AbstractPyPIResolver and AbstractGitSourceResolver. Since both inherit from AbstractResolver, this field could live there instead. Resolvers that don't need it (like VersionMapResolver) would inherit it harmlessly as None.
2 participants
Pull Request Description
What
Introduce new configuration for source resolver and downloads. The new system uses profiles for common tasks:
pypi-sdist: resolve versions of sdists from PyPI, download sdistpypi-prebuilt: resolve versions of platform wheels from PyPI, download pre-built wheel from PyPI.pypi-download: resolve versions of any package from PyPI, download from external URL (with{version}variable)pypi-git: resolve versions for any package from PyPI, git clone (with{version}variable)versionmap-git: resolve with a version map, git clonegithub-tag-download: resolve from GitHub tag, download tarballgithub-tag-git: resolve from GitHub tag, git clonegitlab-tag-download: resolve from GitLab tag, download tarballgitlab-tag-git: resolve from GitLab tag, git clonenot-available: block resolvinghook: call Fromager hooksThe new settings will eventually replace
download_source,resolver_dist, andgit_optionstop-level options as well aswheel_server_urlandpre_builtflags for variants.Why
New resolver configuration with tag provider and git clone #937
[proposal] New resolver configuration #938
PR follows CONTRIBUTING.md guidelines