feat: Enable skip GPU readiness - #166
Conversation
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe platform configuration adds ChangesGPU readiness control
KServe readiness target
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant PlatformConfig
participant PrepareSequence
participant NFDGPUPreparation
participant GPUNodeVerification
PlatformConfig->>PrepareSequence: provide skip_gpu_readiness
alt skip_gpu_readiness is enabled
PrepareSequence->>PrepareSequence: log skipped GPU readiness
else skip_gpu_readiness is disabled
PrepareSequence->>NFDGPUPreparation: prepare NFD and GPU operators
PrepareSequence->>GPUNodeVerification: verify GPU nodes
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
thanks, |
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
…forge into feat/skip-gpu-readiness
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test fournos llm_d llama-33-70b-rhoai-release |
🔴 Submission of
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
projects/llm_d/orchestration/prepare_sequence.py (2)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the skip log match all skipped operations.
The later condition also skips
prepare_phase.verify_gpu_nodes(), but the current message names only NFD and GPU Operator. Include GPU node verification in the message.Proposed log update
- logger.info("Skipping GPU readiness (NFD + GPU operator): skip_gpu_readiness is enabled") + logger.info( + "Skipping GPU readiness (NFD + GPU operator + GPU node verification): " + "skip_gpu_readiness is enabled" + )🤖 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 `@projects/llm_d/orchestration/prepare_sequence.py` at line 18, Update the skip_gpu_readiness log message in the preparation flow to explicitly mention GPU node verification alongside NFD and the GPU operator, so it accurately describes every skipped operation.
17-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both configuration values.
When
platform.cluster.skip_gpu_readinessistrue, assert thatprepare_phase.prepare_nfd(),prepare_phase.prepare_gpu_operator(), andprepare_phase.verify_gpu_nodes()are not called. When it isfalse, assert that the first two calls run and GPU node verification remains enabled for each active run specification.Run the requested acceptance case:
fournos llm_d llama-33-70b-rhoai-releaseonathena-firewithforge-prepare-only,ReadWriteMany,nfs-rwx, RHOAI imagequay.io/rhoai/rhoai-fbc-fragment@sha256:29c909eb7d499b4d1dfb11086c5cd554a257c0477d013f4837f08aa9e4385164, channelstable-3.5, andplatform.cluster.skip_gpu_readiness: true.Also applies to: 34-35
🤖 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 `@projects/llm_d/orchestration/prepare_sequence.py` around lines 17 - 22, The prepare_sequence regression coverage should test both values of platform.cluster.skip_gpu_readiness: when true, assert prepare_nfd, prepare_gpu_operator, and verify_gpu_nodes are not called; when false, assert the first two are called and GPU verification remains enabled for every active run specification. Add the specified athena-fire acceptance case with the provided fournos, image, channel, forge-prepare-only, ReadWriteMany, and nfs-rwx settings.
🤖 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.
Nitpick comments:
In `@projects/llm_d/orchestration/prepare_sequence.py`:
- Line 18: Update the skip_gpu_readiness log message in the preparation flow to
explicitly mention GPU node verification alongside NFD and the GPU operator, so
it accurately describes every skipped operation.
- Around line 17-22: The prepare_sequence regression coverage should test both
values of platform.cluster.skip_gpu_readiness: when true, assert prepare_nfd,
prepare_gpu_operator, and verify_gpu_nodes are not called; when false, assert
the first two are called and GPU verification remains enabled for every active
run specification. Add the specified athena-fire acceptance case with the
provided fournos, image, channel, forge-prepare-only, ReadWriteMany, and nfs-rwx
settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd4975a1-d1eb-4179-8cbc-e007acb4f34a
📒 Files selected for processing (2)
projects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/prepare_sequence.py
|
/test fournos llm_d llama-33-70b-rhoai-release |
🔴 Submission of
|
|
/test fournos llm_d cpt-release-testing-llama-33-70b |
🟢 Execution of
|
🟢 Submission of
|
🔴 Execution of
|
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
…forge into feat/skip-gpu-readiness
🔴 Execution of
|
🔴 Execution of
|
2 participants
Summary by CodeRabbit
New Features
Bug Fixes