[rhaiis] Rename engine_args to vllm_args and add explicit TP to all models - #167
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:
📝 WalkthroughWalkthroughRHAIIS replaces model-level ChangesRHAIIS configuration and orchestration
Caliper result handling
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
![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
🧹 Nitpick comments (1)
projects/rhaiis/orchestration/runtime_config.py (1)
127-146: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression tests for the renamed fallback path.
Test
merge_engine_argsfor model and workloadvllm_args, non-vLLM translation, engine-specific blocks, and final override precedence. These cases protect the cross-engine configuration contract.🤖 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/rhaiis/orchestration/runtime_config.py` around lines 127 - 146, Extend the test coverage for merge_engine_args to verify model and workload vllm_args fallback, _translate_args usage for non-vLLM engines, engine-specific argument blocks, and precedence of overrides over workload and model values. Add regression cases covering both vLLM and non-vLLM paths while preserving the expected merged configuration.
🤖 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/rhaiis/orchestration/ci.py`:
- Around line 100-101: Update projects/rhaiis/orchestration/ci.py lines 100-101
and projects/rhaiis/orchestration/analysis.py lines 243-244 to derive
tensor-parallel size from the effective arguments returned by merge_engine_args,
rather than directly from model["vllm_args"]. Ensure launch, notification,
regression-check, and agent-analysis paths all use the merged tensor-parallel
configuration, including Fournos overrides.
---
Nitpick comments:
In `@projects/rhaiis/orchestration/runtime_config.py`:
- Around line 127-146: Extend the test coverage for merge_engine_args to verify
model and workload vllm_args fallback, _translate_args usage for non-vLLM
engines, engine-specific argument blocks, and precedence of overrides over
workload and model values. Add regression cases covering both vLLM and non-vLLM
paths while preserving the expected merged configuration.
🪄 Autofix
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 Plus
Run ID: 8af293be-8328-4124-b923-fac0fe13f85f
📒 Files selected for processing (5)
projects/rhaiis/README.mdprojects/rhaiis/orchestration/analysis.pyprojects/rhaiis/orchestration/ci.pyprojects/rhaiis/orchestration/config.d/models.yamlprojects/rhaiis/orchestration/runtime_config.py
The engine_args field was vLLM-specific but named generically. Rename to vllm_args for clarity and add explicit tensor-parallel-size to every model entry. Co-authored-by: Cursor <cursoragent@cursor.com>
df2c3d0 to
2e0f8dd
Compare
…alysis PR openshift-psap#163 changed run_kpi_analysis to return (status_dict, report) but the tests were not updated, causing TypeError on dict key access. Co-authored-by: Cursor <cursoragent@cursor.com>
…gent analysis resolve_hardware_request and run_agent_analysis were reading tensor-parallel-size directly from model vllm_args, missing Fournos configOverrides. Now both use merge_engine_args output. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/test fournos rhaiis nvidia benchmark hera ci-quick |
🟢 Execution of
|
🟢 Submission of
|
…ions include the link Moved precreate_mlflow_run_if_configured() to before wait_isvc_ready and write early test labels with mlflow_destination. This ensures the MLflow URL is available in Slack failure notifications even when the InferenceService never becomes ready. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/rhaiis/orchestration/analysis.py`:
- Line 232: Make engine_args a required parameter in run_agent_analysis and
remove the optional fallback before extracting tp. Preserve the configured tp
value supplied by current callers instead of defaulting to 1 when arguments are
absent.
🪄 Autofix
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 Plus
Run ID: 07284010-c02e-442f-9735-ee49b692b226
📒 Files selected for processing (3)
projects/rhaiis/orchestration/analysis.pyprojects/rhaiis/orchestration/ci.pyprojects/rhaiis/orchestration/test_phase.py
…k preset - Added sglang_args block to nemotron3super-120b-fp8 model definition - Split ci-quick into ci-test (full CI config) and ci-quick (workload only) Co-authored-by: Cursor <cursoragent@cursor.com>
|
/test fournos rhaiis nvidia sglang nemotron3super-120b ci-quick hera |
🟢 Execution of
|
Added warmup, version, slack_notify_always, and other defaults to ci-quick preset so it works standalone without ci-test. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/test fournos rhaiis nvidia benchmark hera ci-quick |
🟢 Execution of
|
🟢 Submission of
|
Write mlflow_destination to a separate _mlflow_destination.yaml marker instead of an empty __test_labels__.yaml that the post-processor picks up as a test base. Add fallback in _build_mlflow_run_url to read the marker file for failure notifications. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
/test fournos rhaiis nvidia sglang nemotron3super-120b ci-quick hera |
🟢 Execution of
|
🟢 Submission of
|
Co-authored-by: Cursor <cursoragent@cursor.com>
|
/test fournos rhaiis nvidia sglang nemotron3super-120b ci-quick hera |
🟢 Execution of
|
🟢 Submission of
|
🟢 Execution of
|
|
/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
Summary
engine_argstovllm_argsin model definitions to clarify that theseare vLLM-specific flags, not engine-agnostic configuration.
tensor-parallel-sizeto every model entry (placeholder0wherethe correct value needs to be filled in).
runtime_config.py,ci.py,analysis.py) andREADME documentation to use the new field name.
Motivation
The
engine_argsname was misleading — these are vLLM CLI flags that getauto-translated for other engines (SGLang, TRT-LLM) via
_translate_args.Renaming makes the engine-specificity explicit and reduces confusion when
adding support for engine-native arguments.
Testing
merge_engine_argsreads fromvllm_argswith thesame fallback/merge semantics as before.
Summary by CodeRabbit
vllm_argsfor runtime overrides.ci-quickpreset toci-test.ci-quickpreset for quick validation runs.vllm_argsand its precedence.