[caliper] solidify the metrics to mlflow - #158
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds schema-v2 KPI conversion into per-run artifacts, updates MLflow handling for scalar and 2D metrics, removes parser-generated metric and parameter files from MCP Gateway, and adjusts replot configuration behavior. ChangesKPI export pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant KPIOrchestration
participant generate_metrics_from_kpis
participant ArtifactRunDirectories
participant MetricsJson
participant ParametersJson
KPIOrchestration->>generate_metrics_from_kpis: Convert kpis.json
generate_metrics_from_kpis->>ArtifactRunDirectories: Match test runs by marker files
generate_metrics_from_kpis->>MetricsJson: Write scalar and 2D KPI values
generate_metrics_from_kpis->>ParametersJson: Write test labels
generate_metrics_from_kpis-->>KPIOrchestration: Return status and warnings
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Move metrics.json generation from project-specific parsers into caliper as a generic mechanism. After kpis.json is produced, caliper now automatically writes per-run metrics.json and parameters.json files using KPI ids as metric keys. The MLflow export backend picks these up via _log_metrics_and_params_from_tree without any project-specific code. - Add caliper/engine/kpi/metrics_from_kpis.py with generate_metrics_from_kpis() - Wire into postprocess pipeline after kpis.json generation step - Remove mcp_gateway-specific metrics.json/parameters.json writing - Simplify mcp_gateway notifications MLflow metric key lookup - Update tests to reflect new ownership Co-authored-by: Cursor <cursoragent@cursor.com>
c66dfd6 to
5080b5e
Compare
|
/test fournos mcp_gateway matrix-demo-1 |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos llm_d janus cpt |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos llm_d janus cpt |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos llm_d janus cpt |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/caliper/cli/commands.py`:
- Around line 1032-1038: Update the exception handler around the artifacts
export failure to stop printing the raw exception and full traceback via
click.echo. Emit only a sanitized, generic user-facing error to stderr, and
route detailed diagnostics through the project’s protected logging mechanism
with secrets redacted.
- Around line 1030-1031: Update the callback invoking run_artifacts_export to
capture its returned exit code, and call ctx.exit(exit_code) whenever the value
is non-zero. Preserve successful completion when the result is zero and leave
exception handling unchanged.
- Around line 1030-1031: Update the call to artifacts_export in the CLI command
to pass mlflow_secrets_path as its dedicated argument, in addition to
mlflow_config_data=final_config. Ensure run_artifacts_export receives the
configured secrets path through that parameter so MLflow credentials are loaded
correctly.
In `@projects/caliper/engine/kpi/metrics_from_kpis.py`:
- Around line 121-124: Restrict the parameter construction in the labels
handling block to an explicit allowlist of known public parameter keys, and pass
only those entries to _write_json. Exclude all unknown labels and sensitive
values from parameters.json while preserving the existing string conversion for
allowed keys.
In `@projects/mcp_gateway/postprocess/tests/test_mcp_gateway_plugin.py`:
- Around line 68-75: Update test_parse_does_not_write_metrics_json and every
other MCPGatewayParser.parse invocation in this file to pass only the nodes
collection, such as [node], removing the tmp_path argument while preserving each
test’s existing assertions.
🪄 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 Plus
Run ID: d2dd2fc1-9c30-4580-b9ae-df0fe69c7db9
📒 Files selected for processing (7)
projects/caliper/cli/commands.pyprojects/caliper/engine/file_export/artifacts_export_run.pyprojects/caliper/engine/kpi/metrics_from_kpis.pyprojects/caliper/orchestration/postprocess.pyprojects/mcp_gateway/orchestration/notifications.pyprojects/mcp_gateway/postprocess/mcp_gateway/parsing/parsers.pyprojects/mcp_gateway/postprocess/tests/test_mcp_gateway_plugin.py
| mlflow_config_data=final_config, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)commands\.py$' | sed -n '1,50p'
echo "== inspect relevant lines =="
sed -n '980,1055p' projects/caliper/cli/commands.py
echo "== find run_artifacts_export definitions/usages =="
rg -n "def run_artifacts_export|run_artifacts_export\\(" projects/caliper projects -g '*.py' | sed -n '1,120p'Repository: openshift-psap/forge
Length of output: 3509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== artifacts_export_run outline/lines =="
wc -l projects/caliper/engine/file_export/artifacts_export_run.py
sed -n '1,180p' projects/caliper/engine/file_export/artifacts_export_run.py
echo "== orchestration export caller =="
wc -l projects/sbin/caliper/orchestration/export.py
sed -n '220,260p' projects/sbin/caliper/orchestration/export.pyRepository: openshift-psap/forge
Length of output: 6879
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant artifacts_export_run lines =="
sed -n '180,254p' projects/caliper/engine/file_export/artifacts_export_run.py
echo "== locate orchestration export.py files =="
git ls-files | rg '(^|/)export\.py$' | sed -n '1,80p'
echo "== run_artifacts_export callers =="
rg -n "run_artifacts_export\\(" -g '*.py' . | sed -n '1,120p'Repository: openshift-psap/forge
Length of output: 3752
Propagate the export exit code.
run_artifacts_export returns 1 or 4 for configuration and export failures. This callback ignores the returned value and exits successfully unless an exception is raised. Capture the result and call ctx.exit(exit_code) when it is non-zero.
🤖 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/caliper/cli/commands.py` around lines 1030 - 1031, Update the
callback invoking run_artifacts_export to capture its returned exit code, and
call ctx.exit(exit_code) whenever the value is non-zero. Preserve successful
completion when the result is zero and leave exception handling unchanged.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pass mlflow_secrets_path separately.
artifacts_export receives mlflow_secrets_path, but this call stores it only in final_config["secrets_path"]. run_artifacts_export loads the secrets file only from its separate mlflow_secrets_path argument at Lines 137-140 of projects/caliper/engine/file_export/artifacts_export_run.py. The CLI can therefore omit the configured MLflow credentials.
Pass the dedicated argument to preserve secret loading.
Proposed fix
run_artifacts_export(
from_path=from_path,
backend=list(backend) if backend else ["mlflow"],
dry_run=dry_run,
verbose=verbose,
status_yaml_path=status_yaml_path,
+ mlflow_secrets_path=mlflow_secrets_path,
mlflow_config_data=final_config,
)📝 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.
| mlflow_config_data=final_config, | |
| ) | |
| run_artifacts_export( | |
| from_path=from_path, | |
| backend=list(backend) if backend else ["mlflow"], | |
| dry_run=dry_run, | |
| verbose=verbose, | |
| status_yaml_path=status_yaml_path, | |
| mlflow_secrets_path=mlflow_secrets_path, | |
| mlflow_config_data=final_config, | |
| ) |
🤖 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/caliper/cli/commands.py` around lines 1030 - 1031, Update the call
to artifacts_export in the CLI command to pass mlflow_secrets_path as its
dedicated argument, in addition to mlflow_config_data=final_config. Ensure
run_artifacts_export receives the configured secrets path through that parameter
so MLflow credentials are loaded correctly.
| labels = test_entry.get("labels", {}) | ||
| if labels: | ||
| params = {str(k): ("" if v is None else str(v)) for k, v in labels.items()} | ||
| _write_json(run_dir / PARAMETERS_FILE, params) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not persist every test label.
labels can contain sensitive values from project configuration. This code writes every value to parameters.json.
Allowlist known public parameter keys before calling _write_json. Do not serialize unknown labels.
As per coding guidelines, “Never write secrets or sensitive data—including passwords, tokens, pull secrets, certificates, API keys, or Secret data/stringData—to env.ARTIFACT_DIR, logs, or files.”
🤖 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/caliper/engine/kpi/metrics_from_kpis.py` around lines 121 - 124,
Restrict the parameter construction in the labels handling block to an explicit
allowlist of known public parameter keys, and pass only those entries to
_write_json. Exclude all unknown labels and sensitive values from
parameters.json while preserving the existing string conversion for allowed
keys.
Source: Coding guidelines
| def test_parse_does_not_write_metrics_json(self, tmp_path: Path): | ||
| """Parser no longer writes metrics.json — caliper handles it generically.""" | ||
| node = _make_test_node(tmp_path, "run-a", SAMPLE_STATS_CSV, TEST_LABELS) | ||
| parser = MCPGatewayParser() | ||
|
|
||
| parser.parse(tmp_path, [node]) | ||
|
|
||
| metrics_file = tmp_path / "run-a" / "metrics.json" | ||
| assert metrics_file.exists() | ||
| data = json.loads(metrics_file.read_text()) | ||
| assert data["total_requests"] == 1000 | ||
| assert data["requests_per_second"] == 31.5 | ||
|
|
||
| def test_parse_writes_parameters_json(self, tmp_path: Path): | ||
| node = _make_test_node(tmp_path, "run-a", SAMPLE_STATS_CSV, TEST_LABELS) | ||
| parser = MCPGatewayParser() | ||
|
|
||
| parser.parse(tmp_path, [node]) | ||
|
|
||
| params_file = tmp_path / "run-a" / "parameters.json" | ||
| assert params_file.exists() | ||
| data = json.loads(params_file.read_text()) | ||
| assert data["preset"] == "smoke" | ||
| assert data["target"] == "gateway" | ||
| assert data["users"] == "16" | ||
| assert not (tmp_path / "run-a" / "metrics.json").exists() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Call MCPGatewayParser.parse with nodes only.
Line 73 passes tmp_path and [node]. MCPGatewayParser.parse accepts only nodes.
Update this call and the other test calls in this file to pass [node] only.
Proposed fix
- parser.parse(tmp_path, [node])
+ parser.parse([node])📝 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.
| def test_parse_does_not_write_metrics_json(self, tmp_path: Path): | |
| """Parser no longer writes metrics.json — caliper handles it generically.""" | |
| node = _make_test_node(tmp_path, "run-a", SAMPLE_STATS_CSV, TEST_LABELS) | |
| parser = MCPGatewayParser() | |
| parser.parse(tmp_path, [node]) | |
| metrics_file = tmp_path / "run-a" / "metrics.json" | |
| assert metrics_file.exists() | |
| data = json.loads(metrics_file.read_text()) | |
| assert data["total_requests"] == 1000 | |
| assert data["requests_per_second"] == 31.5 | |
| def test_parse_writes_parameters_json(self, tmp_path: Path): | |
| node = _make_test_node(tmp_path, "run-a", SAMPLE_STATS_CSV, TEST_LABELS) | |
| parser = MCPGatewayParser() | |
| parser.parse(tmp_path, [node]) | |
| params_file = tmp_path / "run-a" / "parameters.json" | |
| assert params_file.exists() | |
| data = json.loads(params_file.read_text()) | |
| assert data["preset"] == "smoke" | |
| assert data["target"] == "gateway" | |
| assert data["users"] == "16" | |
| assert not (tmp_path / "run-a" / "metrics.json").exists() | |
| def test_parse_does_not_write_metrics_json(self, tmp_path: Path): | |
| """Parser no longer writes metrics.json — caliper handles it generically.""" | |
| node = _make_test_node(tmp_path, "run-a", SAMPLE_STATS_CSV, TEST_LABELS) | |
| parser = MCPGatewayParser() | |
| parser.parse([node]) | |
| assert not (tmp_path / "run-a" / "metrics.json").exists() |
🤖 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/mcp_gateway/postprocess/tests/test_mcp_gateway_plugin.py` around
lines 68 - 75, Update test_parse_does_not_write_metrics_json and every other
MCPGatewayParser.parse invocation in this file to pass only the nodes
collection, such as [node], removing the tmp_path argument while preserving each
test’s existing assertions.
🟢 Execution of
|
🟢 Submission of
|
- commands.py: remove duplicate mlflow_config_data kwarg, keep upload_workers from upstream - artifacts_export_run.py: adopt upstream's _resolve_tracking_uri refactor over inline URI resolution Co-authored-by: Cursor <cursoragent@cursor.com>
openshift-ci
Bot
added
the
needs-rebase
KPIs with is_2d=true (e.g. throughput curves, latency vs load) are now
extracted as {x,y} data points in metrics.json and logged as stepped
MLflow metrics so the UI renders them as curves.
Co-authored-by: Cursor <cursoragent@cursor.com>
openshift-ci
Bot
removed
the
needs-rebase
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/caliper/engine/kpi/metrics_from_kpis.py`:
- Around line 47-65: Reject nonintegral 2D x coordinates at both ingestion
points so MLflow never receives a truncated step: update _extract_2d_points in
projects/caliper/engine/kpi/metrics_from_kpis.py and the metrics.json loading
path in projects/caliper/engine/file_export/mlflow_backend.py to validate that
each 2D point’s x is an integer-valued scalar before accepting it. Keep the
existing sorting and point-shaping behavior for valid data, and filter or return
None for any payload containing a nonintegral x so _log_2d_metrics only sees
safe step values.
🪄 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: a6ed6d68-aa4a-4cae-89cf-d086e3ba2377
📒 Files selected for processing (6)
projects/caliper/cli/commands.pyprojects/caliper/engine/file_export/mlflow_backend.pyprojects/caliper/engine/kpi/metrics_from_kpis.pyprojects/caliper/orchestration/postprocess.pyprojects/mcp_gateway/postprocess/mcp_gateway/parsing/parsers.pyprojects/mcp_gateway/postprocess/tests/test_mcp_gateway_plugin.py
🚧 Files skipped from review as they are similar to previous changes (4)
- projects/caliper/orchestration/postprocess.py
- projects/mcp_gateway/postprocess/tests/test_mcp_gateway_plugin.py
- projects/mcp_gateway/postprocess/mcp_gateway/parsing/parsers.py
- projects/caliper/cli/commands.py
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos llm_d janus cpt-xks |
The click is_flag=True default (False) prevented the config fallback caliper.replot.keep from ever being consulted, so /var overrides had no effect and the download directory was always cleaned up — deleting the metrics.json files before the export step could read them. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
- Fix line length in replot.py for ruff format compliance. - Raise ValueError in _log_2d_metrics when x values are non-integer or x/y are non-numeric, instead of silently skipping. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@kpouget take a look at the results. |
Cleaner than the or-None workaround: None default means "not passed" so the config fallback is used, while --keep-download / --no-keep-download explicitly override it. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
kpouget
changed the title
| if not kpis_json_path.is_file(): | ||
| logger.warning("kpis.json not found at %s, skipping metrics generation", kpis_json_path) | ||
| return |
There was a problem hiding this comment.
this should be an error (and not here, in the engine, see next comment)
Consistent with the source_to_destination naming convention used elsewhere: artifacts_to_kpis, kpis_to_csv, artifacts_to_ai_data. Co-authored-by: Cursor <cursoragent@cursor.com>
| return | ||
|
|
||
| try: | ||
| from projects.caliper.engine.kpi.metrics_from_kpis import generate_metrics_from_kpis |
There was a problem hiding this comment.
this is breaking the encapsulation 🙃
can you add a command entrypoint for that, same as the existing transform steps
that makes Caliper easier to use independently from the Python code
| else: | ||
| logger.warning("kpis-to-metrics: %s", result.get("error", "unknown error")) | ||
| except Exception as e: | ||
| logger.warning("kpis-to-metrics conversion failed (non-fatal): %s", e) |
There was a problem hiding this comment.
should be fatal, and the export.py workflow should determine what to do with the failure
Replace direct engine import in postprocess.py with fork/exec subprocess call via the new `caliper kpi kpis-to-mlflow` CLI command, matching the pattern used by all other Caliper orchestration steps. - Add kpis-to-mlflow click command in cli/commands.py - Add build_kpis_to_mlflow_command in cli_builder.py - Register command under kpi_group in cli/main.py - Orchestrator catches step failure and records status, continuing remaining steps - kpis_to_mlflow.py raises FileNotFoundError instead of swallowing Co-authored-by: Cursor <cursoragent@cursor.com>
|
/test fournos llm_d janus cpt-xks |
🟢 Execution of
|
🟢 Submission of
|
Self-contained failure handling in _run_kpis_to_metrics_step, consistent with all other _run_*_step methods. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashtarkb 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
Test plan
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Changes