feat: source resolver configuration#1052
Conversation
📝 WalkthroughWalkthroughThis PR introduces a comprehensive resolver system for the Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Additional NotesObservable detail: The 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. |
599609d to
976deff
Compare
976deff to
19a9602
Compare
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
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
4c9f9fb to
5d955ff
Compare
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
5d955ff to
ac069e7
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/packagesettings/_models.py`:
- Around line 338-342: The new "source" field must be made mutually exclusive
with the legacy fields (download_source, resolver_dist, wheel_server_url,
pre_built): add a model-level validator (e.g., a pydantic root_validator or
`@model_validator`) on the class that declares the "source" field to check if
source is not None and any of
download_source/resolver_dist/wheel_server_url/pre_built are also set, and if so
raise a ValueError with a clear message; implement the same validator logic for
the other occurrence of the "source" field in this file so both model variants
reject mixed configs.
In `@src/fromager/packagesettings/_resolver.py`:
- Around line 376-392: The server_url built in _gitlab_provider drops an
explicit port because it uses self.project_url.host; update the server_url
construction so it preserves any configured port (e.g., use
self.project_url.port or self.project_url.netloc to include host:port) when
creating the resolver.GitLabTagProvider; keep project_path set via
self.project_url.path.lstrip("/") and pass the rest of the same arguments to
GitLabTagProvider.
- Around line 272-294: The version_map entries are being passed as raw refs
(e.g., "refs/tags/1.1") so they become Candidate.url values that aren't
fetchable and the default build_sdist=tarball doesn't match a git checkout flow;
update resolver_provider to transform version_map values that look like git refs
(e.g., start with "refs/" or match a tag/commit pattern) into concrete fetchable
Git URLs using the validated clone_url (combine clone_url and the ref into a
git+<clone_url>@<ref> URL or otherwise construct the provider-specific URL)
before creating versionmap.VersionMap(self.version_map), or use a Git-specific
provider that accepts clone_url + ref (e.g., switch to resolver.GitProvider or
pass package_name plus clone_url/ref into VersionMapProvider); also change the
class default build_sdist (build_sdist: typing.ClassVar[BuildSDist | None]) from
BuildSDist.tarball to a value consistent with git checkouts (e.g., None or the
git/checkout option) so sdist building behavior matches the git resolution flow.
- Around line 506-516: HookResolver currently advertises provider: "hook" but
leaves resolver_provider unimplemented, causing valid configs to pass validation
but crash at runtime; update HookResolver so the settings system rejects "hook"
at validation time by implementing a validation entry that raises a clear
ConfigError (or ValueError) during config parsing instead of
NotImplementedError. Locate the HookResolver class and its resolver_provider
method and either (a) remove/register it from the public SourceResolver union so
it isn't accepted, or (b) add a class-level validation hook (e.g., __post_init__
/ validate / from_config depending on your config framework) that checks
provider == "hook" and raises a configuration error with a helpful message
indicating the provider is not yet supported. Ensure references to HookResolver,
resolver_provider, and provider are used so the fix is applied in the right
place.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2a3dfa4-708b-48ab-a138-e0ac21eb10f5
📒 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 as they are similar to previous changes (3)
- tests/testdata/context/overrides/settings/test_github-download.yaml
- tests/testdata/context/overrides/settings/test_pypiprebuilt.yaml
- tests/testdata/context/overrides/settings/test_pypisdist.yaml
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)
- `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>
ac069e7 to
fe42980
Compare
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/packagesettings/_resolver.py`:
- Around line 374-382: The server_url string currently always appends
:{self.project_url.port}, which yields "https://host:443" when the URL uses the
default HTTPS port; change the construction to only append ":<port>" when
project_url.port is present and not the scheme's default (omit port when scheme
is "https" and port == 443, and similarly for "http" and port == 80), e.g.
compute a local server_port fragment based on self.project_url.scheme and
self.project_url.port and pass
server_url=f"https://{self.project_url.host}{server_port}" into
resolver.GitLabTagProvider so default ports are not included.
🪄 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: 75028634-38e3-410f-9227-0dea62a60cd1
📒 Files selected for processing (7)
docs/reference/config-reference.rstsrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_resolver.pysrc/fromager/resolver.pytests/test_packagesettings.pytests/test_packagesettings_resolver.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_packagesettings.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/reference/config-reference.rst
- src/fromager/resolver.py
…ines Activate the `source` field on `PackageSettings` and `VariantInfo` so YAML-configured providers are used by the resolver and download pipelines. This is the wiring layer for the model classes introduced in PR python-wheel-build#1052. - Uncomment `source: SourceResolver | None` in `PackageSettings` and `VariantInfo` - Add `PackageBuildInfo.source_resolver` property (variant overrides package level) - `default_resolver_provider()` checks `pbi.source_resolver` before returning the default `PyPIProvider` - `default_download_source()` detects `git+https://` / `git+ssh://` URLs and routes to `download_git_source()` - Add `GitOptions.remove_dot_git` field (default `True`) and wire it into `download_git_source()` - Update existing test snapshots and add 20 new tests Closes: python-wheel-build#1048 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Shanmukh Pawan <smoparth@redhat.com>
…ines Activate the `source` field on `PackageSettings` and `VariantInfo` so YAML-configured providers are used by the resolver and download pipelines. This is the wiring layer for the model classes introduced in PR python-wheel-build#1052. - Uncomment `source: SourceResolver | None` in `PackageSettings` and `VariantInfo` - Add `PackageBuildInfo.source_resolver` property (variant overrides package level) - `default_resolver_provider()` checks `pbi.source_resolver` before returning the default `PyPIProvider` - `default_download_source()` detects `git+https://` URL and routes to `git_clone_fast()` with automatic submodule handling - Add `GitOptions.remove_dot_git` field (default `False`) and removes `.git` recursively including submodules - Update existing test snapshots and add new tests Closes: python-wheel-build#1048 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Shanmukh Pawan <smoparth@redhat.com>
…ines Activate the `source` field on `PackageSettings` and `VariantInfo` so YAML-configured providers are used by the resolver and download pipelines. This is the wiring layer for the model classes introduced in PR python-wheel-build#1052. - Uncomment `source: SourceResolver | None` in `PackageSettings` and `VariantInfo` - Add `PackageBuildInfo.source_resolver` property (variant overrides package level) - `default_resolver_provider()` checks `pbi.source_resolver` before returning the default `PyPIProvider` - `default_download_source()` detects `git+https://` URL and routes to `git_clone_fast()` with automatic submodule handling - Add `GitOptions.remove_dot_git` field (default `False`) and removes `.git` recursively including submodules - Change `resolver_provider()` in `_resolver.py` to accept `RequirementType | None` for consistency with all Provider constructors; removes need for `typing.cast` - Update existing test snapshots and add new tests Closes: python-wheel-build#1048 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Shanmukh Pawan <smoparth@redhat.com>
…ines Activate the `source` field on `PackageSettings` and `VariantInfo` so YAML-configured providers are used by the resolver and download pipelines. This is the wiring layer for the model classes introduced in PR python-wheel-build#1052. - Uncomment `source: SourceResolver | None` in `PackageSettings` and `VariantInfo` - Add `PackageBuildInfo.source_resolver` property (variant overrides package level) - `default_resolver_provider()` checks `pbi.source_resolver` before returning the default `PyPIProvider` - `default_download_source()` detects `git+https://` URL and routes to `git_clone_fast()` with automatic submodule handling - Add `GitOptions.remove_dot_git` field (default `False`) and removes `.git` recursively including submodules - Change `resolver_provider()` in `_resolver.py` to accept `RequirementType | None` for consistency with all Provider constructors; removes need for `typing.cast` - Update existing test snapshots and add new tests Closes: python-wheel-build#1048 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Shanmukh Pawan <smoparth@redhat.com>
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