[llm-d] Add dry-run capability#132
Conversation
📝 WalkthroughWalkthroughThe deployment and test orchestration flows now support KServe dry-run execution, propagate manifest-derived service metadata through task context, and skip cluster-dependent operations. Smoke pod diagnostics write command output directly through the ChangesKServe dry-run orchestration
Smoke pod debug capture
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TestPhase
participant DeployInferenceService
participant DeployLLMISVC
participant CheckDryRun
TestPhase->>DeployInferenceService: start with runtime.kserve_dry_run
DeployInferenceService->>DeployLLMISVC: pass dry_run and manifest
DeployLLMISVC->>CheckDryRun: evaluate dry-run flag
CheckDryRun-->>DeployLLMISVC: return prepared manifest path
DeployLLMISVC-->>DeployInferenceService: manifest path
DeployInferenceService-->>TestPhase: skip endpoint-dependent steps
Possibly related PRs
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 |
|
/test fournos llm_d smoke |
🔴 Submission of
|
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d smoke |
🟢 Execution of
|
🟢 Submission of
|
…build the llmisvc src
|
/test fournos llm_d smoke |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/guidellm/toolbox/run_smoke_request/main.py (1)
241-250: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the pod YAML artifact and failure status.
The
oc get pod -o yamlresult is now discarded. Becausecheck=False, command failures also do not raise, so this path neither writes the YAML artifact nor reports capture failure. Retain the result/write logic or pass a supported artifact destination and inspectreturncode.🤖 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/guidellm/toolbox/run_smoke_request/main.py` around lines 241 - 250, Update the oc("get", "pod", ...) call in the smoke-request flow to retain its result, write the pod YAML artifact as before, and inspect returncode when check=False. Preserve failure reporting so unsuccessful capture is surfaced instead of silently discarded.
🧹 Nitpick comments (1)
projects/llm_d/orchestration/test_phase.py (1)
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the dry-run config lookup to a helper.
The configuration value for
runtime.kserve_dry_runis retrieved individually in four different functions across this file (here,do_test,deploy_inference_service, andcleanup_test_resources). Consider abstracting it into a module-level helper function to reduce duplication and centralize the fallback logic.♻️ Proposed refactor
+def _is_dry_run() -> bool: + return config.project.get_config("runtime.kserve_dry_run", False) + def run() -> int: """Main test function that wraps do_test() with outcome postprocessing.""" - dry_run = config.project.get_config("runtime.kserve_dry_run", False) + dry_run = _is_dry_run()🤖 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/test_phase.py` at line 56, Extract the repeated runtime.kserve_dry_run lookup into a module-level helper and update do_test, deploy_inference_service, and cleanup_test_resources to use it. Preserve the existing False fallback and configuration behavior while centralizing this logic.
🤖 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 `@projects/guidellm/toolbox/run_smoke_request/main.py`:
- Around line 253-260: Update the oc invocation in the debug-info task to ensure
stdout_dest reaches the underlying command runner: either extend oc to accept
and forward stdout_dest, or call shell.run directly with the existing arguments.
Preserve writing the pod description artifact to artifacts_dir and avoid the
current TypeError.
---
Outside diff comments:
In `@projects/guidellm/toolbox/run_smoke_request/main.py`:
- Around line 241-250: Update the oc("get", "pod", ...) call in the
smoke-request flow to retain its result, write the pod YAML artifact as before,
and inspect returncode when check=False. Preserve failure reporting so
unsuccessful capture is surfaced instead of silently discarded.
---
Nitpick comments:
In `@projects/llm_d/orchestration/test_phase.py`:
- Line 56: Extract the repeated runtime.kserve_dry_run lookup into a
module-level helper and update do_test, deploy_inference_service, and
cleanup_test_resources to use it. Preserve the existing False fallback and
configuration behavior while centralizing this logic.
🪄 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: 500dd02a-7e8b-4cbe-a501-f807242783a1
📒 Files selected for processing (4)
projects/guidellm/toolbox/run_smoke_request/main.pyprojects/kserve/toolbox/deploy_llmisvc/main.pyprojects/llm_d/orchestration/config.d/runtime.yamlprojects/llm_d/orchestration/test_phase.py
| oc( | ||
| "describe", | ||
| "pod", | ||
| ctx.pod_name, | ||
| "-n", | ||
| args.namespace, | ||
| check=False, | ||
| stdout_dest=artifacts_dir / f"{ctx.pod_name}.description.txt", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A30 'def oc\(' projects/core/dsl/utils/k8s.py
rg -n 'stdout_dest' projects/core/dsl/utils/k8s.py projects/guidellm/toolbox/run_smoke_request/main.pyRepository: openshift-psap/forge
Length of output: 1204
🏁 Script executed:
#!/bin/bash
sed -n '220,290p' projects/guidellm/toolbox/run_smoke_request/main.py
printf '\n---\n'
sed -n '1,120p' projects/core/dsl/utils/k8s.py
printf '\n---\n'
rg -n "except .*TypeError|TypeError|stdout_dest" projects/guidellm/toolbox/run_smoke_request/main.py projects/core/dsl/utils/k8s.py projects/core/dsl/utils -g '!**/__pycache__/**'Repository: openshift-psap/forge
Length of output: 5114
Pass stdout_dest through oc or call shell.run directly projects/guidellm/toolbox/run_smoke_request/main.py:253-260 passes stdout_dest into oc, but projects/core/dsl/utils/k8s.py:19-48 doesn’t accept or forward that keyword, so this raises TypeError and the debug-info task silently skips writing the pod description artifact.
🤖 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/guidellm/toolbox/run_smoke_request/main.py` around lines 253 - 260,
Update the oc invocation in the debug-info task to ensure stdout_dest reaches
the underlying command runner: either extend oc to accept and forward
stdout_dest, or call shell.run directly with the existing arguments. Preserve
writing the pod description artifact to artifacts_dir and avoid the current
TypeError.
🟢 Execution of
|
🟢 Submission of
|
kpouget
changed the title
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: albertoperdomo2 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
2 participants
First step to be able to compare LLMISVCs with reference files
Summary by CodeRabbit
New Features
Bug Fixes
Tests