[proposal] wheel_build_tag hook for unique wheel file names#1066
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a new proposal document for a Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/proposals/wheel-build-tag-hook.md`:
- Around line 121-124: The example hook uses the variable parts when appending
CUDA tag but the accumulator defined earlier is named result, causing an
undefined variable error; update the CUDA branch to append to result (i.e.,
replace parts.append(...) with result.append(...)) and verify other branches
consistently use the same accumulator (referencing ctx.variant and Version(...)
in the function).
🪄 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: 243b235f-4791-4b2e-bff6-6ba1ef0f1db7
📒 Files selected for processing (3)
docs/proposals/index.rstdocs/proposals/wheel-build-tag-hook.mddocs/spelling_wordlist.txt
20d94ef to
f323ece
Compare
rd4398
left a comment
There was a problem hiding this comment.
This is a solid design and will really help us downstream as well!
I left a few comments, after addressing those we can merge this.
92d123b to
4460182
Compare
rd4398
left a comment
There was a problem hiding this comment.
This looks good to me! I will wait for @LalatenduMohanty and @dhellmann to take a look as well and give feedback.
|
This pull request has merge conflicts that must be resolved before it can be merged. |
4460182 to
42ad37b
Compare
dhellmann
left a comment
There was a problem hiding this comment.
This looks good to me. I have a one clarifying question, but nothing to really hold it up.
42ad37b to
985f2ac
Compare
|
I have re-designed the hook. It's now a single hook that is configured in global |
985f2ac to
f0ed498
Compare
dhellmann
left a comment
There was a problem hiding this comment.
This direction makes sense to me. I'll leave the PR open for other reviewers to have a chance to read it before we merge it.
f0ed498 to
ba7d51f
Compare
|
@dhellmann @rd4398 @LalatenduMohanty Could you please review the design again? Thanks! |
|
This looks good to me! |
LalatenduMohanty
left a comment
There was a problem hiding this comment.
I feel this proposal is little verbose. We can make it concise without loosing important context.
Add proposal document for a new stevedore hook point that injects platform, accelerator, and ABI suffixes into wheel build tags. See: python-wheel-build#1059 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Christian Heimes <cheimes@redhat.com>
ba7d51f to
9de0f8b
Compare
I have shortened the proposal and moved the example to a separate file. The file is included when Sphinx renders the markdown file. |
This is an old review as per the latest comment from Rohan.
3119c93
into
python-wheel-build:main
Pull Request Description
What
Add proposal document for a new hook point that injects platform, accelerator, and ABI suffixes into wheel build tags.
Why
See: #1059