fix: layering issue in caliper - #155
Conversation
|
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 (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change centralizes Caliper MLflow export helpers, adds multi-run artifact export and run discovery, wires workspace configuration through the CLI and orchestration layers, and updates tests. It also changes MCP gateway verifier and directive parsing configuration. ChangesArtifact export flow
MCP gateway orchestration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant OrchestrationExport
participant RunMultiRunArtifactsExport
participant MLflow
participant StatusOutput
CLI->>OrchestrationExport: provide export settings and workspace
OrchestrationExport->>RunMultiRunArtifactsExport: pass source and run directories
RunMultiRunArtifactsExport->>MLflow: log parent and nested child artifacts
MLflow-->>RunMultiRunArtifactsExport: return export result
RunMultiRunArtifactsExport->>StatusOutput: write status and select exit code
🚥 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 |
![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/caliper/engine/file_export/artifacts_export_run.py (1)
300-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated secrets/config loading and status/exit-code logic within the same file.
Lines 300-315 (secrets/config load and validation) and lines 397-410 (status echo, status YAML write, and failure exit code) are identical to the equivalent blocks already present in
run_artifacts_exportin this same file. Now that both functions live in one module, extract these two blocks into small shared helpers (for example_load_and_validate_mlflow_inputs(...)and_finalize_export_results(...)) and call them from bothrun_artifacts_exportandrun_multi_run_artifacts_export.This reduces the maintenance burden of keeping two copies of the same validation and exit-code logic in sync.
Also applies to: 397-410
🤖 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/file_export/artifacts_export_run.py` around lines 300 - 315, Extract the duplicated MLflow input loading/validation and export-result finalization logic into shared private helpers, such as _load_and_validate_mlflow_inputs and _finalize_export_results. Update both run_artifacts_export and run_multi_run_artifacts_export to call these helpers, preserving the existing validation error messages, status YAML writing, status output, and failure exit codes.
🤖 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/file_export/artifacts_export_run.py`:
- Around line 292-298: Update the multi-run backend validation to reject any
list containing a backend other than “mlflow”, rather than only rejecting lists
that omit “mlflow”. Preserve the existing error message and return behavior for
unsupported backend combinations.
---
Nitpick comments:
In `@projects/caliper/engine/file_export/artifacts_export_run.py`:
- Around line 300-315: Extract the duplicated MLflow input loading/validation
and export-result finalization logic into shared private helpers, such as
_load_and_validate_mlflow_inputs and _finalize_export_results. Update both
run_artifacts_export and run_multi_run_artifacts_export to call these helpers,
preserving the existing validation error messages, status YAML writing, status
output, and failure exit codes.
🪄 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: 3ff17a3a-41d4-4d37-8ddf-9b97bbdafb6a
📒 Files selected for processing (3)
projects/caliper/engine/file_export/artifacts_export_run.pyprojects/caliper/orchestration/export.pyprojects/caliper/tests/test_multi_run_export.py
| if "mlflow" not in backends: | ||
| click.echo( | ||
| f"only 'mlflow' backend export is supported for multi-run " | ||
| f"(got '{' '.join(backends)}').", | ||
| err=True, | ||
| ) | ||
| return 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Backend check does not reject extra unsupported backends for multi-run.
The condition "mlflow" not in backends only rejects requests missing mlflow. If a caller passes backend=["mlflow", "s3"], the check passes silently, and only the mlflow export runs. The caller gets no warning that s3 was ignored for the multi-run path.
Tighten the check to reject any backend other than mlflow.
🐛 Proposed fix
- if "mlflow" not in backends:
+ if set(backends) != {"mlflow"}:
click.echo(
f"only 'mlflow' backend export is supported for multi-run "
f"(got '{' '.join(backends)}').",
err=True,
)
return 1📝 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 "mlflow" not in backends: | |
| click.echo( | |
| f"only 'mlflow' backend export is supported for multi-run " | |
| f"(got '{' '.join(backends)}').", | |
| err=True, | |
| ) | |
| return 1 | |
| if set(backends) != {"mlflow"}: | |
| click.echo( | |
| f"only 'mlflow' backend export is supported for multi-run " | |
| f"(got '{' '.join(backends)}').", | |
| err=True, | |
| ) | |
| return 1 |
🤖 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/file_export/artifacts_export_run.py` around lines 292
- 298, Update the multi-run backend validation to reject any list containing a
backend other than “mlflow”, rather than only rejecting lists that omit
“mlflow”. Preserve the existing error message and return behavior for
unsupported backend combinations.
|
/test fournos mcp_gateway demo |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos mcp_gateway demo |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos mcp_gateway demo |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos mcp_gateway demo |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/file_export/artifacts_export_run.py`:
- Around line 111-117: Update the exception handler around
load_mlflow_secrets_yaml and validate_mlflow_secrets to avoid interpolating the
caught exception into terminal output. Keep handling the existing exception
types and return code, but emit a fixed sanitized message for invalid MLflow
secrets files so parser details or secret values are never displayed.
- Around line 111-117: Sanitize exception-derived output in
projects/caliper/engine/file_export/artifacts_export_run.py at lines 111-117 and
153-162: in the load_mlflow_secrets_yaml/validate_mlflow_secrets handling,
replace rendered exception text with a fixed secret-file error, and update the
exception callers around the result/status persistence path to emit only
sanitized details without str(e) or traceback output. Ensure both terminal
output and status YAML never include raw exception content.
- Around line 349-355: Update the loaded-input handling around
_load_mlflow_inputs to validate the returned secret data with
validate_mlflow_secrets before projecting any fields into mlflow_connection.
Preserve the existing integer error return and configuration handling, and
return the validator’s error result when secret validation fails.
In `@projects/core/dsl/utils/k8s.py`:
- Around line 405-412: The no-worker-node case currently appears compliant;
update projects/core/dsl/utils/k8s.py lines 405-412 in ensure_node_labels to
return a distinct no-matching-nodes result or raise an explicit error, then
update projects/mcp_gateway/orchestration/preflight_phase.py lines 205-209 in
check_node_labels to convert that outcome into PreflightError rather than
logging successful compliance.
🪄 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: 2f0df750-c141-4f66-8780-c5dd26e92e97
📒 Files selected for processing (3)
projects/caliper/engine/file_export/artifacts_export_run.pyprojects/core/dsl/utils/k8s.pyprojects/mcp_gateway/orchestration/preflight_phase.py
| if not result: | ||
| logger.warning("No nodes found with role '%s'", node_role) | ||
| return [] | ||
|
|
||
| nodes = result.get("items", []) | ||
| if not nodes: | ||
| logger.warning("No nodes found with role '%s'", node_role) | ||
| return [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail preflight when no worker nodes match.
ensure_node_labels returns [] when no worker nodes exist. check_node_labels treats that result as compliance and passes preflight. The configured node_selector then remains unenforced, and later test pods can stay unschedulable.
projects/core/dsl/utils/k8s.py#L405-L412: Return a distinct no-matching-nodes result, or raise an explicit error.projects/mcp_gateway/orchestration/preflight_phase.py#L205-L209: Convert that result intoPreflightErrorinstead of logging that all worker nodes comply.
📍 Affects 2 files
projects/core/dsl/utils/k8s.py#L405-L412(this comment)projects/mcp_gateway/orchestration/preflight_phase.py#L205-L209
🤖 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/core/dsl/utils/k8s.py` around lines 405 - 412, The no-worker-node
case currently appears compliant; update projects/core/dsl/utils/k8s.py lines
405-412 in ensure_node_labels to return a distinct no-matching-nodes result or
raise an explicit error, then update
projects/mcp_gateway/orchestration/preflight_phase.py lines 205-209 in
check_node_labels to convert that outcome into PreflightError rather than
logging successful compliance.
🟢 Execution of
|
🟢 Submission of
|
I wonder where this duplication comes from ? 🤔 for this PR, could you
and for this one or a subsequent one, could you
I'd like to have two presets: |
🔴 Execution of
|
🔴 Submission of
|
cd66e6a to
7574b60
Compare
|
/test fournos mcp_gateway demo |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos mcp_gateway demo |
🟢 Execution of
|
🔴 Submission of
|
|
FYI I don't know where that comes from exactly. Must be related to the merge of my PR |
|
@ashtarkb , can you make sure that this: correctly exports the artifacts with the multi-run layout? cherry-pick these commits from #157 for the command to work |
|
/test fournos mcp_gateway demo |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos llm_d janus cpt |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
projects/mcp_gateway/orchestration/pr_args.py (1)
58-62: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not omit matching
/test fournoslines from persistence.
apply_pr_directives()appends non-/helpparsed directive lines topr_config.txt. Matching/_parse_test_line()lines are only logged as presets, so commands like/test fournos mcp_gateway smokeleave no recorded artifact. Keep these lines inparsed_directivesor document that only configuration directives such as/versionare persisted.🤖 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/orchestration/pr_args.py` around lines 58 - 62, Update apply_pr_directives() so every line recognized by _parse_test_line(), including /test fournos commands, is appended to parsed_directives before continuing, while preserving the existing preset logging. Ensure these matched lines are written to pr_config.txt alongside other non-/help directives.projects/caliper/cli/commands.py (5)
1024-1031: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward the dedicated MLflow runtime arguments.
run_artifacts_export()inprojects/caliper/engine/file_export/artifacts_export_run.py, Lines [173-309], readsmlflow_insecure_tlsandupload_workersfrom explicit parameters. This call supplies neither. The matching keys insidefinal_configdo not populate those parameters. Non-default TLS and worker settings are ignored.Pass the values explicitly.
Proposed argument forwarding
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_insecure_tls=final_config["insecure_tls"], + upload_workers=upload_workers, 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 1024 - 1031, Update the run_artifacts_export call in the CLI command to pass mlflow_insecure_tls and upload_workers explicitly from the parsed runtime options, rather than relying on values inside final_config. Preserve the existing final_config forwarding and ensure both dedicated arguments retain their configured non-default values.
1024-1031: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate the export return code.
run_artifacts_export()returns an integer exit code. This command discards it and exits successfully when no exception is raised. A failed export can therefore appear successful to the shell and orchestration.Capture the return value and propagate nonzero codes.
Proposed return-code handling
- run_artifacts_export( + export_code = 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_config_data=final_config, ) + if export_code: + sys.exit(export_code)🤖 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 1024 - 1031, Update the command invoking run_artifacts_export to capture its integer return code, then propagate any nonzero code through the command’s existing exit or return mechanism. Preserve the current arguments and successful behavior while ensuring failed exports are reported as failures to the shell.
1024-1031: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winForward the MLflow secret file path into
run_artifacts_export().
run_artifacts_export()only merges credential data whenmlflow_secrets_pathis passed to_load_mlflow_inputs(). This call stores it asfinal_config["secrets_path"]but does not passmlflow_secrets_path, so only a--mlflow-secretsCLI argument or orchestration path is currently honored. Add the explicit value here and useinsecure_tlsvalidation only if the inline config contract allows it.Proposed secret-path forwarding
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=( + Path(final_config["secrets_path"]) + if final_config.get("secrets_path") + else None + ), 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 1024 - 1031, Update the run_artifacts_export call in the command handler to pass the MLflow secret file path through its mlflow_secrets_path parameter, using the value captured from the CLI/config flow. Ensure insecure_tls validation remains conditional on the inline configuration contract rather than applying it unconditionally.
525-533: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact all diagnostics before public output.
These handlers expose raw exception data through stderr or status files. A status file can be placed in
env.ARTIFACT_DIR, andanalyze_kpis()returns rawstr(e)values. Secret values can therefore reach public artifacts or logs.
projects/caliper/cli/commands.py#L525-L533: replace raw traceback and exception text with redacted fields.projects/caliper/cli/commands.py#L739-L743: sanitizeresult["error"]before storing it.projects/caliper/cli/commands.py#L778-L780: sanitizeanalysis_report_error.projects/caliper/cli/commands.py#L785-L790: print only the sanitized error.projects/caliper/cli/commands.py#L1032-L1038: redact export exceptions and tracebacks before stderr output.As per coding guidelines,
projects/**/*.pymust not write sensitive data to files or logs or include secret values in error messages.🤖 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 525 - 533, Redact exception messages and tracebacks before any public output, artifact/status-file write, or log in projects/caliper/cli/commands.py: update the handler around lines 525-533, sanitize result["error"] at lines 739-743, sanitize analysis_report_error at lines 778-780 and print only that sanitized value at lines 785-790, and redact export exceptions and tracebacks at lines 1032-1038. Reuse the established redaction utility or introduce one shared sanitizer so no raw secret values reach stderr, status files, or error messages.Source: Coding guidelines
733-737: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the analysis outcome fields.
analyze_kpis()inprojects/caliper/engine/kpi/analyze.py, Lines [680-755], returnsstatus,message, andregressions_detected. This projection drops those fields. Callers can receive onlysuccess: trueand lose the warning or regression state.Copy these fields into
status_databefore adding report details.Proposed status projection
status_data = { "success": result.get("success", False), "completed_at": result.get("completed_at"), } +for key in ("status", "message", "regressions_detected"): + if key in result: + status_data[key] = result[key]🤖 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 733 - 737, Update the status_data projection in the CLI command to preserve the analysis outcome fields returned by analyze_kpis(): status, message, and regressions_detected. Include these values alongside success and completed_at before adding report details, using the existing result data and defaults consistent with the analysis response.
🧹 Nitpick comments (1)
projects/caliper/cli/commands.py (1)
481-515: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid running the parser twice.
run_parse()inprojects/caliper/engine/parse.py, Lines [29-137], parses and caches each test base.run_kpi_generate()inprojects/caliper/engine/kpi/generate.py, Lines [13-52], calls it again. This repeats discovery, cache validation, and parameter-matrix work.Use
discover_test_bases()for the count, or pass the firstUnifiedRunModelinto KPI generation.🤖 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 481 - 515, Update the KPI-generation flow around run_parse and run_kpi_generate to avoid parsing the artifact tree twice. Prefer using discover_test_bases() to determine whether test directories exist before invoking run_kpi_generate, or extend run_kpi_generate to accept and reuse the first UnifiedRunModel; preserve the existing no-directories status and exit behavior.
🤖 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.
Outside diff comments:
In `@projects/caliper/cli/commands.py`:
- Around line 1024-1031: Update the run_artifacts_export call in the CLI command
to pass mlflow_insecure_tls and upload_workers explicitly from the parsed
runtime options, rather than relying on values inside final_config. Preserve the
existing final_config forwarding and ensure both dedicated arguments retain
their configured non-default values.
- Around line 1024-1031: Update the command invoking run_artifacts_export to
capture its integer return code, then propagate any nonzero code through the
command’s existing exit or return mechanism. Preserve the current arguments and
successful behavior while ensuring failed exports are reported as failures to
the shell.
- Around line 1024-1031: Update the run_artifacts_export call in the command
handler to pass the MLflow secret file path through its mlflow_secrets_path
parameter, using the value captured from the CLI/config flow. Ensure
insecure_tls validation remains conditional on the inline configuration contract
rather than applying it unconditionally.
- Around line 525-533: Redact exception messages and tracebacks before any
public output, artifact/status-file write, or log in
projects/caliper/cli/commands.py: update the handler around lines 525-533,
sanitize result["error"] at lines 739-743, sanitize analysis_report_error at
lines 778-780 and print only that sanitized value at lines 785-790, and redact
export exceptions and tracebacks at lines 1032-1038. Reuse the established
redaction utility or introduce one shared sanitizer so no raw secret values
reach stderr, status files, or error messages.
- Around line 733-737: Update the status_data projection in the CLI command to
preserve the analysis outcome fields returned by analyze_kpis(): status,
message, and regressions_detected. Include these values alongside success and
completed_at before adding report details, using the existing result data and
defaults consistent with the analysis response.
In `@projects/mcp_gateway/orchestration/pr_args.py`:
- Around line 58-62: Update apply_pr_directives() so every line recognized by
_parse_test_line(), including /test fournos commands, is appended to
parsed_directives before continuing, while preserving the existing preset
logging. Ensure these matched lines are written to pr_config.txt alongside other
non-/help directives.
---
Nitpick comments:
In `@projects/caliper/cli/commands.py`:
- Around line 481-515: Update the KPI-generation flow around run_parse and
run_kpi_generate to avoid parsing the artifact tree twice. Prefer using
discover_test_bases() to determine whether test directories exist before
invoking run_kpi_generate, or extend run_kpi_generate to accept and reuse the
first UnifiedRunModel; preserve the existing no-directories status and exit
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a9c26f92-9832-4d30-9930-6ce47f7ce902
📒 Files selected for processing (6)
projects/caliper/cli/commands.pyprojects/caliper/engine/file_export/artifacts_export_run.pyprojects/caliper/orchestration/export.pyprojects/caliper/tests/test_multi_run_export.pyprojects/mcp_gateway/orchestration/config.yamlprojects/mcp_gateway/orchestration/pr_args.py
🚧 Files skipped from review as they are similar to previous changes (3)
- projects/caliper/orchestration/export.py
- projects/caliper/tests/test_multi_run_export.py
- projects/caliper/engine/file_export/artifacts_export_run.py
🟢 Execution of
|
🟢 Submission of
|
|
@kpouget let me know what do you think |
|
/test fournos mcp_gateway matrix-demo-1 |
🟢 Execution of
|
🟢 Submission of
|
… export to core and orchestration is importing it
…tants These were module-level constants for simple string literals that are unlikely to change. Inline them to reduce noise. Co-authored-by: Cursor <cursoragent@cursor.com>
…plicate /test directive - Sanitize yaml.YAMLError and secret validation error messages to prevent leaking secret file content (use e.__class__.__name__ instead of str(e)) - Extract _load_mlflow_inputs, _resolve_tracking_uri, _finalize_results helpers to eliminate ~40 lines of duplication between single and multi-run export paths - Remove /test line from mcp_gateway parsed_directives to prevent duplicate entries in GitHub notification (same pattern as /pipeline dedup in nightly) - Revert ensure_node_labels and check_node_labels (deferred to separate PR) Co-authored-by: Cursor <cursoragent@cursor.com>
4f7274e to
c50c2b1
Compare
|
thanks Avi, let's merge this |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kpouget 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
moved the export multi-run experiment export to core and orchestration is importing it
Summary by CodeRabbit
--mlflow-workspaceoption, with command-line values taking precedence over configuration files.