feat(resolver): wire source_resolver into resolver and download pipelines#1202
feat(resolver): wire source_resolver into resolver and download pipelines#1202smoparth wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 35 minutes and 35 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR wires the previously defined source resolver model layer into the runtime resolver and download pipelines. In Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
![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 (1)
src/fromager/packagesettings/_pbi.py (1)
180-186: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd
versionaddedmetadata to the new publicsource_resolverproperty.This accessor is a new user-facing API surface and should carry a Sphinx
.. versionadded:: 0.85directive for release-doc consistency.Suggested patch
def source_resolver(self) -> SourceResolver | None: """Effective source resolver for this package and variant. + .. versionadded:: 0.85 + Returns the variant-level ``source`` override if set, otherwise the package-level ``source``, or ``None`` when neither is configured. """As 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 `@src/fromager/packagesettings/_pbi.py` around lines 180 - 186, The `source_resolver` property in the _pbi.py file is a new public API but is missing the required Sphinx version metadata. Add a `.. versionadded:: 0.85` directive to the docstring of the `source_resolver` property to document when this accessor was introduced and maintain consistency with coding guidelines for user-facing API 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/resolver.py`:
- Around line 129-138: The code uses typing.cast to convert req_type to
RequirementType without enforcing that req_type is actually non-None at runtime.
Since source.resolver_provider() requires a non-optional RequirementType
argument, add a runtime validation check before the return statement in the
source resolver block to ensure req_type is not None. If it is None, either
raise an appropriate error or handle it appropriately so that None is never
passed to the source.resolver_provider() call.
In `@src/fromager/sources.py`:
- Around line 207-223: The `_parse_git_url` function returns clone URLs that may
still contain query parameters and fragment identifiers (such as
`#subdirectory`=...), which are invalid for git cloning. When reconstructing the
clone URL in the `_parse_git_url` function using the `_replace()` method on the
parsed URL object before calling `geturl()`, you need to also clear the query
and fragment components by setting them to empty strings in addition to updating
the path. This ensures the returned clone URL contains only the protocol, host,
and path components needed for git operations.
In `@tests/test_source_resolver_wiring.py`:
- Around line 74-77: Replace all real domain names in test URL fixtures with
their `.test` equivalents throughout the test file. Specifically, change domains
like github.com to github.test, pypi.org to pypi.test, and any other real
domains to their corresponding .test TLD versions. This applies to test URL
literals used in test methods including test_is_git_url_ssh and other test
methods that construct URLs as part of their fixtures, ensuring consistent
adherence to test isolation conventions.
---
Nitpick comments:
In `@src/fromager/packagesettings/_pbi.py`:
- Around line 180-186: The `source_resolver` property in the _pbi.py file is a
new public API but is missing the required Sphinx version metadata. Add a `..
versionadded:: 0.85` directive to the docstring of the `source_resolver`
property to document when this accessor was introduced and maintain consistency
with coding guidelines for user-facing API 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: 0a276ad7-135e-47ad-af4f-351f66a07123
📒 Files selected for processing (6)
src/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/resolver.pysrc/fromager/sources.pytests/test_packagesettings.pytests/test_source_resolver_wiring.py
| pbi = ctx.package_build_info(req) | ||
| source = pbi.source_resolver | ||
| if source is not None: | ||
| logger.info( | ||
| "%s: using source resolver provider %r", | ||
| req.name, | ||
| source.provider, | ||
| ) | ||
| return source.resolver_provider(ctx, typing.cast(RequirementType, req_type)) | ||
|
|
There was a problem hiding this comment.
Avoid passing optional req_type through a type cast in the source-resolver path.
Line 137 casts req_type but does not enforce it at runtime; None can still flow into source.resolver_provider(...) (non-optional contract), causing runtime failures in provider logic.
Suggested fix
if source is not None:
+ if req_type is None:
+ raise ValueError(
+ f"req_type must be set when using source resolver for {req.name}"
+ )
logger.info(
"%s: using source resolver provider %r",
req.name,
source.provider,
)
- return source.resolver_provider(ctx, typing.cast(RequirementType, req_type))
+ return source.resolver_provider(ctx, req_type)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pbi = ctx.package_build_info(req) | |
| source = pbi.source_resolver | |
| if source is not None: | |
| logger.info( | |
| "%s: using source resolver provider %r", | |
| req.name, | |
| source.provider, | |
| ) | |
| return source.resolver_provider(ctx, typing.cast(RequirementType, req_type)) | |
| pbi = ctx.package_build_info(req) | |
| source = pbi.source_resolver | |
| if source is not None: | |
| if req_type is None: | |
| raise ValueError( | |
| f"req_type must be set when using source resolver for {req.name}" | |
| ) | |
| logger.info( | |
| "%s: using source resolver provider %r", | |
| req.name, | |
| source.provider, | |
| ) | |
| return source.resolver_provider(ctx, req_type) |
🤖 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/resolver.py` around lines 129 - 138, The code uses typing.cast
to convert req_type to RequirementType without enforcing that req_type is
actually non-None at runtime. Since source.resolver_provider() requires a
non-optional RequirementType argument, add a runtime validation check before the
return statement in the source resolver block to ensure req_type is not None. If
it is None, either raise an appropriate error or handle it appropriately so that
None is never passed to the source.resolver_provider() call.
| def _parse_git_url(url: str) -> tuple[str, str | None]: | ||
| """Split a VCS URL into clone URL and optional ref. | ||
|
|
||
| ``git+https://host/repo@ref`` -> ``(https://host/repo, ref)`` | ||
| ``git+https://host/repo`` -> ``(https://host/repo, None)`` | ||
| """ | ||
| clone_url = url | ||
| if clone_url.startswith("git+"): | ||
| clone_url = clone_url[len("git+") :] | ||
|
|
||
| parsed = urlparse(clone_url) | ||
| ref: str | None = None | ||
| if "@" in parsed.path: | ||
| new_path, _, ref = parsed.path.rpartition("@") | ||
| clone_url = parsed._replace(path=new_path).geturl() | ||
|
|
||
| return clone_url, ref |
There was a problem hiding this comment.
Strip query/fragment from git clone URL in _parse_git_url.
Line 221 can return clone URLs that still contain query/fragment data (for example #subdirectory=...), which is not a valid clone target and can break download_git_source().
Suggested fix
def _parse_git_url(url: str) -> tuple[str, str | None]:
@@
- parsed = urlparse(clone_url)
+ parsed = urlparse(clone_url)
+ # Not part of git clone target
+ parsed = parsed._replace(params="", query="", fragment="")
ref: str | None = None
if "@" in parsed.path:
new_path, _, ref = parsed.path.rpartition("@")
clone_url = parsed._replace(path=new_path).geturl()
+ else:
+ clone_url = parsed.geturl()
return clone_url, ref🤖 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/sources.py` around lines 207 - 223, The `_parse_git_url`
function returns clone URLs that may still contain query parameters and fragment
identifiers (such as `#subdirectory`=...), which are invalid for git cloning. When
reconstructing the clone URL in the `_parse_git_url` function using the
`_replace()` method on the parsed URL object before calling `geturl()`, you need
to also clear the query and fragment components by setting them to empty strings
in addition to updating the path. This ensures the returned clone URL contains
only the protocol, host, and path components needed for git operations.
…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>
smoparth
force-pushed
the
feat/wire-source-resolver
branch
from
fac1b7f to
55aca35
Compare
|
@tiran PTAL |
| clones that rely on ``.git`` for version detection (e.g. via | ||
| setuptools-scm). | ||
|
|
||
| .. versionadded:: 0.85 |
There was a problem hiding this comment.
I think you meant 0.86 ?
|
|
||
| submodules: False | ||
| submodule_paths: [] | ||
| remove_dot_git: True |
There was a problem hiding this comment.
Nitpick : The class docstring example shows remove_dot_git: True, but the default is False , so we should change the dc string to false
|
The PR looks good to me overall. Lets see if we get review from @tiran . |
2 participants
Activate the
sourcefield onPackageSettingsandVariantInfoso YAML-configured providers are used by the resolver and download pipelines. This is the wiring layer for the model classes introduced in PR #1052.source: SourceResolver | NoneinPackageSettingsandVariantInfoPackageBuildInfo.source_resolverproperty (variant overrides package level)default_resolver_provider()checkspbi.source_resolverbefore returning the defaultPyPIProviderdefault_download_source()detectsgit+https:///git+ssh://URLs and routes todownload_git_source()GitOptions.remove_dot_gitfield (defaultTrue) and wire it intodownload_git_source()Closes: #1048
Pull Request Description
What
Why