Repository navigation
feat: add MLflow integration with postprocessing, KPIs, and PSAP payload for rhaiis - #74
Conversation
- Create rhaiis postprocess plugin (projects/rhaiis/postprocess/) with 22 KPIs for model-furnace parity (12 caliper baseline + 10 new: p99 percentiles, request counts, concurrency, duration, prompt tokens) - Parser extends shared GuideLLMParser with p99 and request_totals extracted directly from benchmarks.json (no shared code changes) - PSAP payload generator creates model-furnace compatible JSON with experiment metadata (model, vllm image/args, accelerator, timestamps) - Wire run_and_postprocess() into test_phase.py with __test_labels__.yaml - Set MLflow tags dynamically: model, accelerator, vllm image/version, workload, rates, guidellm config — persisted via config.set_config() - Fix broken import: caliper_export_command → caliper_export_entrypoint - Add caliper.postprocess config section with rhaiis plugin module E2E tested on fournos-fg (L40S, qwen3-0.6B, rates 1,5,10, 60s max). MLflow export verified with 17 tags and 66 KPI records at: https://mlflow.apps.aperdomo-lab.ibm.rhperfscale.org/#/experiments/57
Handle both old (create_fournos_resolve_command) and new (create_fournos_resolve_entrypoint) names for backward compatibility with different forge-core container versions.
- Read gpu_type from config (rhaiis.gpu_types) instead of hardcoding h200 - Pass-through if user already set gpuType in FournosJob spec - Return empty hardware if no gpu_type configured (exclusive cluster lock) - Add gpu_types mapping to config.d/rhaiis.yaml
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Rhaiis postprocessing pipeline comprising a ChangesRhaiis Postprocessing Pipeline and Orchestration Wiring
Sequence Diagram(s)sequenceDiagram
participant CLI
participant do_test
participant RhaiisParser
participant generate_psap_payload
participant RhaiisKpiHandler
participant MLflow
CLI->>do_test: run(model_key, workload_key, namespace)
do_test->>do_test: deploy + benchmark lifecycle
do_test->>generate_psap_payload: locate benchmarks.json, build payload
generate_psap_payload-->>do_test: PSAP payload written to ARTIFACT_DIR
do_test->>MLflow: _set_mlflow_metadata (guidellm_* tags, vllm_version)
MLflow-->>do_test: config.project tags set
Note over do_test: run_and_postprocess triggers caliper pipeline
do_test->>RhaiisParser: parse(base_dir, nodes)
RhaiisParser-->>do_test: enriched ParseResult
do_test->>RhaiisKpiHandler: compute_kpis(model)
RhaiisKpiHandler-->>do_test: KPI result dicts → kpis.jsonl
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🤖 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 29-35: The early return when hardware_spec.get("gpuType") is true
prevents the derivation and assignment of gpuCount based on tensor-parallel-size
from the vllm_args. Instead of returning immediately when gpuType is present,
continue processing to ensure both gpuType and gpuCount are properly set in the
hardware_spec before returning. Modify the logic so that the gpuCount derivation
from tp_size occurs regardless of whether gpuType was pre-specified, ensuring
the returned hardware_spec contains both values consistently.
In `@projects/rhaiis/orchestration/test_phase.py`:
- Around line 125-138: The calls to _generate_psap_payload and
_set_mlflow_metadata are post-benchmark side-effects that should not cause the
entire benchmark run to fail if they encounter errors. Wrap both function calls
in try-except blocks to catch and handle any exceptions gracefully, allowing the
benchmark to be marked as successful even if these post-processing steps fail.
Log any caught exceptions appropriately but ensure execution continues without
re-raising.
In `@projects/rhaiis/postprocess/parser.py`:
- Around line 81-83: The code always uses `benchmarks[0]` to extract metrics,
which in multi-rate runs causes all UnifiedResultRecord entries to incorrectly
use metrics from the first benchmark entry instead of their corresponding rate's
benchmark. Fix this by determining which benchmark entry corresponds to the
current record being processed (likely based on a rate index or iteration
identifier) and use that benchmark entry instead of always using `benchmarks[0]`
when assigning the bench variable and extracting metrics. Apply this same fix to
all locations where metrics are extracted from benchmarks, including the code
block mentioned at lines 95-103.
🪄 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: 5799d5ca-4111-46b2-ad25-562b6a7babe7
📒 Files selected for processing (10)
projects/rhaiis/orchestration/ci.pyprojects/rhaiis/orchestration/config.d/rhaiis.yamlprojects/rhaiis/orchestration/config.yamlprojects/rhaiis/orchestration/runtime_config.pyprojects/rhaiis/orchestration/test_phase.pyprojects/rhaiis/postprocess/__init__.pyprojects/rhaiis/postprocess/kpis.pyprojects/rhaiis/postprocess/parser.pyprojects/rhaiis/postprocess/plugin.pyprojects/rhaiis/postprocess/psap_payload.py
| if hardware_spec.get("gpuType"): | ||
| return hardware_spec | ||
|
|
||
| model_key = runtime_config.get_test_model_key() | ||
| model = runtime_config.get_model(model_key) | ||
| vllm_args = model.get("vllm_args", {}) | ||
| tp_size = int(vllm_args.get("tensor-parallel-size", 1)) |
There was a problem hiding this comment.
Preserve gpuCount when gpuType is pre-specified.
At Line 29, the early return skips the gpuCount derivation from tensor-parallel-size (Line 35). That makes the resolved hardware payload inconsistent depending on caller input and can leave an incomplete GPU request.
Suggested fix
def resolve_hardware_request(hardware_spec: dict) -> dict:
test_rhaiis.init()
- if hardware_spec.get("gpuType"):
- return hardware_spec
-
model_key = runtime_config.get_test_model_key()
model = runtime_config.get_model(model_key)
vllm_args = model.get("vllm_args", {})
tp_size = int(vllm_args.get("tensor-parallel-size", 1))
+
+ if hardware_spec.get("gpuType"):
+ hardware_spec.setdefault("gpuCount", tp_size)
+ return hardware_spec📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if hardware_spec.get("gpuType"): | |
| return hardware_spec | |
| model_key = runtime_config.get_test_model_key() | |
| model = runtime_config.get_model(model_key) | |
| vllm_args = model.get("vllm_args", {}) | |
| tp_size = int(vllm_args.get("tensor-parallel-size", 1)) | |
| model_key = runtime_config.get_test_model_key() | |
| model = runtime_config.get_model(model_key) | |
| vllm_args = model.get("vllm_args", {}) | |
| tp_size = int(vllm_args.get("tensor-parallel-size", 1)) | |
| if hardware_spec.get("gpuType"): | |
| hardware_spec.setdefault("gpuCount", tp_size) | |
| return hardware_spec |
🤖 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/ci.py` around lines 29 - 35, The early return
when hardware_spec.get("gpuType") is true prevents the derivation and assignment
of gpuCount based on tensor-parallel-size from the vllm_args. Instead of
returning immediately when gpuType is present, continue processing to ensure
both gpuType and gpuCount are properly set in the hardware_spec before
returning. Modify the logic so that the gpuCount derivation from tp_size occurs
regardless of whether gpuType was pre-specified, ensuring the returned
hardware_spec contains both values consistently.
| bench = benchmarks[0] | ||
| metrics = bench.get("metrics", {}) | ||
|
|
There was a problem hiding this comment.
Map benchmark-derived metrics to the current record, not always the first benchmark entry.
Line 81 always uses benchmarks[0]. In multi-rate runs, this copies p99/request-total fields from the first rate into every UnifiedResultRecord, producing incorrect downstream KPI values for other rates.
Also applies to: 95-103
🤖 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/postprocess/parser.py` around lines 81 - 83, The code always
uses `benchmarks[0]` to extract metrics, which in multi-rate runs causes all
UnifiedResultRecord entries to incorrectly use metrics from the first benchmark
entry instead of their corresponding rate's benchmark. Fix this by determining
which benchmark entry corresponds to the current record being processed (likely
based on a rate index or iteration identifier) and use that benchmark entry
instead of always using `benchmarks[0]` when assigning the bench variable and
extracting metrics. Apply this same fix to all locations where metrics are
extracted from benchmarks, including the code block mentioned at lines 95-103.
| finally: | ||
| _capture_and_cleanup(deployment_name, namespace) | ||
|
|
||
| _generate_psap_payload(model_cfg, accelerator, vllm_image, vllm_args, workload_key) |
There was a problem hiding this comment.
this is already parsing/transforming the guidellm file. I think it's better to have it all in the post-processing phase, it makes it easier to share code between projects using guidellm
wdyt?
There was a problem hiding this comment.
I added it because wanted have parity with how MF post processes results (and metadata). But right now only rhaiis uses it. Adding it to core is premature until llm_d also needs it. We can add it later.
There was a problem hiding this comment.
not to core, to projects/rhaiis/postprocess/parser.py
(nothing related to rhaiis/llmd/... should go to core, "ever")
There was a problem hiding this comment.
The parser's job is to read benchmarks.json and extract metrics. PSAP payload generation writes a new file by combining benchmarks.json with config metadata. That is why I kept it separate. Do you want me to move the implementation of _generate_psap_payload to parser.py from psap_payload?
There was a problem hiding this comment.
this function
is doing a parsing task (it extracts values, builds a new object, ...)
my idea is to defer that work into the post-processing step, to have a clear separation between what's the testing (=gathering data into files) and what's the post-processing (transforming data files into meaningful plots/reports/KPIs)
…-steps, move PSAP to parser - Remove unnecessary try/except for create_fournos_resolve_entrypoint import - Wrap _generate_psap_payload and _set_mlflow_metadata in try/except so post-benchmark failures don't mark the run as failed - Move generate_psap_payload and write_psap_payload from psap_payload.py into parser.py, delete psap_payload.py - Update README with postprocessing, MLflow, PSAP, Fournos, and presets docs E2E tested on mehulvalidation (A100) and fournos-fg (L4, IBM VPC block).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
projects/rhaiis/postprocess/parser.py (1)
179-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle digest-pinned image references before tag splitting.
_split_image_tagcurrently breaks image digests (e.g.,repo/image@sha256:...) by parsing the digest suffix as a tag. This produces incorrectcontainer_image/container_image_tagvalues in PSAP payloads consumed downstream.Suggested fix
def _split_image_tag(full_image: str) -> tuple[str, str]: + # Digest-pinned image: no mutable tag semantics + if "`@sha256`:" in full_image: + return full_image, "digest" if ":" in full_image: parts = full_image.rsplit(":", 1) return parts[0], parts[1] return full_image, "latest"🤖 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/postprocess/parser.py` around lines 179 - 183, The _split_image_tag function incorrectly parses image digests (like repo/image@sha256:abc123) by treating the digest suffix after the last colon as a tag, producing wrong container_image and container_image_tag values. To fix this, check for the presence of "@" symbol in the full_image string first to detect digest-pinned references. If "@" is found, extract and remove the digest portion before attempting to split the remaining image name by colon to find the tag. This ensures digests are handled separately and not confused with tags.
🤖 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.
Duplicate comments:
In `@projects/rhaiis/postprocess/parser.py`:
- Around line 179-183: The _split_image_tag function incorrectly parses image
digests (like repo/image@sha256:abc123) by treating the digest suffix after the
last colon as a tag, producing wrong container_image and container_image_tag
values. To fix this, check for the presence of "@" symbol in the full_image
string first to detect digest-pinned references. If "@" is found, extract and
remove the digest portion before attempting to split the remaining image name by
colon to find the tag. This ensures digests are handled separately and not
confused with tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aea3dca6-7675-4ce0-90a7-4d144cd01917
📒 Files selected for processing (4)
projects/rhaiis/README.mdprojects/rhaiis/orchestration/ci.pyprojects/rhaiis/orchestration/test_phase.pyprojects/rhaiis/postprocess/parser.py
✅ Files skipped from review due to trivial changes (1)
- projects/rhaiis/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- projects/rhaiis/orchestration/ci.py
- projects/rhaiis/orchestration/test_phase.py
|
/test fournos rhaiis |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 22 minutes 07 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos rhaiis |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 22 minutes 09 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos |
|
/test fournos |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 21 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos rhaiis |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 22 minutes 10 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos rhaiis |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 22 minutes 09 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
- Add ci-quick workload profile (2 rates, 60s max, small tokens) to fit within CI 22-minute timeout - Add ci-quick preset in presets.d/presets.yaml - Add pr_args.py for parsing /test fournos rhaiis <preset> directives Usage: /test fournos rhaiis ci-quick
|
/test fournos rhaiis ci-quick |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
projects/rhaiis/orchestration/pr_args.py (1)
48-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMultiple test directives overwrite each other.
If a PR comment contains multiple
/test fournos rhaiislines, only the last one's arguments will be stored inconfig_overrides["ci_job.args"](line 51 overwrites on each iteration), but all matching lines are logged and added toparsed_directives. This inconsistency could confuse users who see multiple directives logged but only the last one taking effect.Consider either:
- Accumulating all args across multiple directives, or
- Logging a warning when multiple directives are detected, or
- Documenting this last-wins behavior explicitly
🤖 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/pr_args.py` around lines 48 - 53, The current implementation overwrites config_overrides["ci_job.args"] on each loop iteration when processing multiple test directives, meaning only the last directive's arguments are stored even though all directives are logged and added to parsed_directives. To fix this inconsistency, modify the code to either accumulate all arguments from multiple directives into config_overrides["ci_job.args"] (combining them into a list or string), or add a warning log when multiple directives are detected to inform users that only the last one will be used. Choose the approach based on the desired behavior: whether multiple directives should contribute their arguments together or if only one should be applied.
🤖 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/rhaiis/orchestration/pr_args.py`:
- Around line 48-53: The current implementation overwrites
config_overrides["ci_job.args"] on each loop iteration when processing multiple
test directives, meaning only the last directive's arguments are stored even
though all directives are logged and added to parsed_directives. To fix this
inconsistency, modify the code to either accumulate all arguments from multiple
directives into config_overrides["ci_job.args"] (combining them into a list or
string), or add a warning log when multiple directives are detected to inform
users that only the last one will be used. Choose the approach based on the
desired behavior: whether multiple directives should contribute their arguments
together or if only one should be applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b960488d-2902-491f-80e9-7bc9114ab5f6
📒 Files selected for processing (3)
projects/rhaiis/orchestration/config.d/workloads.yamlprojects/rhaiis/orchestration/pr_args.pyprojects/rhaiis/orchestration/presets.d/presets.yaml
|
/test fournos rhaiis ci-quick |
|
🟢 Test of 'rhaiis test' succeeded after 00 hours 03 minutes 31 seconds 🟢 • Link to the test results. • Caliper postprocess completed but no reports generated. Test configuration: |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 05 minutes 02 seconds 🟢 • Link to the test results. • No reports generated... Test configuration: |
- Wrap test execution with env.NextArtifactDir("testing") for scoped
artifact directories (supports future multi-test runs)
- Add explicit return 0 after post-benchmark steps so caliper treats
the test as successful (caliper interprets None as failure)
|
@kpouget - can this PR be merged? CI test seems to have completed. |
|
can you please relaunch the test with the latest changes? (with the unnecessary file removed) |
|
/test fournos rhaiis ci-quick |
|
🟢 Test of 'rhaiis test' succeeded after 00 hours 05 minutes 11 seconds 🟢 • Link to the test results. • Caliper postprocess completed but no reports generated. Test configuration: |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 06 minutes 27 seconds 🟢 • Link to the test results. • No reports generated... Test configuration: |
|
Completed the last run |
|
great, let's merge this one |
2 participants
Summary
run_and_postprocess()into test phase with MLflow tag metadatagpu_typeconfigurable in hardware resolver for multi-GPU-type clusterscaliper_export_command→caliper_export_entrypoint,create_fournos_resolve_command→create_fournos_resolve_entrypoint)Changes
New files (
projects/rhaiis/postprocess/)plugin.py— rhaiis PostProcessingPlugin, reuses shared GuideLLMParserkpis.py— 22 KPIs: 12 caliper baseline + 10 model-furnace extras (p99, concurrency, request counts, duration, prompt tokens)parser.py— extends shared parser with p99 percentiles, request_totals, concurrency from raw benchmarks.jsonpsap_payload.py— generates model-furnace compatiblePSAP_perf_*.jsonwith experiment metadataModified files
ci.py— fix broken imports with backward-compatible try/except, configurablegpu_typein hardware resolverconfig.yaml— addcaliper.postprocesssection with rhaiis pluginconfig.d/rhaiis.yaml— addgpu_typesmapping for hardware resolverruntime_config.py— addget_gpu_type()accessortest_phase.py— wrap withrun_and_postprocess(), write__test_labels__.yaml, set MLflow tags, generate PSAP payloadKPI catalog (22 metrics)
MLflow tags (17)
model_key, hf_model_id, accelerator, tensor_parallel_size, vllm_image, vllm_version, workload_key, rates, max_seconds, guidellm_image, guidellm_backend_type, guidellm_rate_type, namespace, deployment_name, project
Test plan
CLI E2E (fournos-fg cluster, 2x L40S)
Fournos E2E (fournos-fg cluster via FournosJob)
forge-rhaiis(experiment ID 57)MLflow verification
Known limitations
__test_labels__.yamlartifact. Fix planned as follow-up.psap-forge-notificationsvault required but unused for export — can be made optional in future.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
gpuTypeand avoids overwriting when GPU type can’t be determined.Documentation