feat(resolver): enhance resolver proposal and model#1228
Conversation
|
Warning Review limit reached
Next review available in: 53 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)
📝 WalkthroughWalkthroughThis PR replaces the generic Estimated code review effort: 3 (Moderate) | ~25 minutes Changes
Sequence Diagram(s)sequenceDiagram
participant Config
participant SourceResolver
participant HookSDistResolver
participant HookPrebuiltResolver
Config->>SourceResolver: provider hook-sdist or hook-prebuilt
SourceResolver->>HookSDistResolver: resolver_provider(ctx, req, req_type)
SourceResolver->>HookPrebuiltResolver: resolver_provider(ctx, req, req_type)
HookSDistResolver-->>Config: NotImplementedError
HookPrebuiltResolver-->>Config: NotImplementedError
Related issues: None specified. Related PRs: None specified. Suggested labels: documentation, enhancement Suggested reviewers: None specified. Poem: 🚥 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: 3
🧹 Nitpick comments (1)
src/fromager/packagesettings/_resolver.py (1)
31-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
versionaddeddirectives to the new public resolvers/enum.
DownloadKind,HookSDistResolver, andHookPrebuiltResolverare user-facing (surfaced in the config reference docs) but their docstrings lack version directives.As per coding guidelines: "Use Sphinx
versionadded,versionremoved,versionchangeddirectives for user-facing changes".Also applies to: 585-619
🤖 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 31 - 39, Add Sphinx versionadded directives to the user-facing docstrings for DownloadKind, HookSDistResolver, and HookPrebuiltResolver, since these are surfaced in the config reference. Update the class/enum docstrings in _resolver.py so each newly public resolver/enum clearly documents when it was introduced, following the project’s version directive convention.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 58-60: The base resolver API is inconsistent because
resolves_prebuilt_wheel is currently a method in the resolver class while
subclasses expose it as a boolean attribute, so attribute access on base
resolvers yields a bound method instead of a falsey value. Update
resolves_prebuilt_wheel on the resolver class to be a property so it matches the
subclass API and keeps r.resolves_prebuilt_wheel behaving as a boolean across
all resolver types.
In `@tests/test_packagesettings_resolver.py`:
- Around line 472-475: The resolver_provider test only checks the generic “not
available” text, so the new NotAvailableResolver behavior that includes req in
the error message is unverified. Strengthen test_resolver_provider in the
resolver_provider path by asserting the raised ValueError message includes the
specific requirement value from _REQ (and still matches the unavailable text),
using the existing _parse, tmp_context, and resolver_provider call to confirm
the req is surfaced in the error.
- Around line 95-97: The `AbstractResolver.resolves_prebuilt_wheel` API is still
a method, so the current assertions in this test are checking a bound method
object instead of a boolean. Update the base resolver API to a `@property` if
that is the intended contract, or change the test to call
`resolves_prebuilt_wheel()` consistently; use `AbstractResolver`,
`supports_override_hooks`, and `resolves_prebuilt_wheel` to locate the relevant
code and keep the assertion style aligned with the actual interface.
---
Nitpick comments:
In `@src/fromager/packagesettings/_resolver.py`:
- Around line 31-39: Add Sphinx versionadded directives to the user-facing
docstrings for DownloadKind, HookSDistResolver, and HookPrebuiltResolver, since
these are surfaced in the config reference. Update the class/enum docstrings in
_resolver.py so each newly public resolver/enum clearly documents when it was
introduced, following the project’s version directive convention.
🪄 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: 6440c7b0-89f1-429a-8feb-fc176fd15f8b
📒 Files selected for processing (5)
docs/proposals/new-resolver-config.mddocs/reference/config-reference.rstsrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_resolver.pytests/test_packagesettings_resolver.py
d1f7a73 to
850bd2f
Compare
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
This looks good and aligns with what we discussed
|
Tick the box to add this pull request to the merge queue (same as
|
|
|
@Mergifyio rebase |
🛑 The pull request rule doesn't match anymoreDetailsThis action has been cancelled. |
Address some short-comings and issues found by Shanmukh. The `resolver_provider` method now accepts a `req` argument, so it can retriebe a PBI from the context. Resolvers have a `download_kind`, `resolves_prebuilt_wheel`, and `supports_override_hooks` attribute. Consumers can use the attributes to figure out what kinds of artifacts a resolver downloads. Split `HookResolver` into `HookSDistResolver` (hook-sdist) and `HookPrebuiltResolver` (hook-prebuilt) for source vs pre-built wheel artifacts. Before the code assumed that hooks were limited to sdists. Clarify that git clone operations use `git_clone_fast`, which retrieves all git submodules recursively. Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Christian Heimes <cheimes@redhat.com>
850bd2f to
5a9149d
Compare
3 participants
Pull Request Description
What
The
resolver_providermethod now accepts areqargument, so it can retriebe a PBI from the context.Resolvers have a
download_kind,resolves_prebuilt_wheel, andsupports_override_hooksattribute. Consumers can use the attributes to figure out what kinds of artifacts a resolver downloads.Split
HookResolverintoHookSDistResolver(hook-sdist) andHookPrebuiltResolver(hook-prebuilt) for source vs pre-built wheel artifacts. Before the code assumed that hooks were limited to sdists.Clarify that git clone operations use
git_clone_fast, which retrieves all git submodules recursively.Why
Address some short-comings and issues found by Shanmukh.