Embed MLflow run ID in dashboard CSV and Slack notifications - #2 - #153
Merged
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
📝 WalkthroughWalkthroughThe change pre-creates and propagates MLflow run metadata through RHAIIS benchmarks, exports, CSV output, and Slack notifications. It also adds MLflow URL generation, configurable success notifications, public Caliper secret utilities, and ignore rules for generated files. ChangesMLflow reporting flow
Configuration hygiene
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestPhase
participant MLflow
participant Marker
participant Export
participant Slack
TestPhase->>MLflow: precreate and finish run
TestPhase->>Marker: store run and experiment IDs
Export->>Marker: discover run ID
Export->>MLflow: export artifacts with run ID
TestPhase->>Slack: send success or regression notification with MLflow link
Possibly related PRs
🚥 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 |
kpouget
reviewed
Jul 31, 2026
kpouget
reviewed
Jul 31, 2026
kpouget
reviewed
Jul 31, 2026
kpouget
reviewed
Jul 31, 2026
kpouget
reviewed
Jul 31, 2026
kpouget
reviewed
Jul 31, 2026
Harshith-umesh
marked this pull request as ready for review
August 3, 2026 15:41
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![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
🧹 Nitpick comments (2)
projects/rhaiis/orchestration/test_phase.py (1)
622-667: 🎯 Functional Correctness | 🔵 TrivialDuplicate success-notification setup vs.
run_regression_check.When
compare_version/versionare both set,run_regression_check()(inanalysis.py) handles the no-regression case internally and already sends a success notification with the same model/accelerator/tp/dp/slack_user computation. When they are not both set, this block (lines 644-667) re-implements the identical model/accelerator/engine-args/tp/dp lookup and callssend_success_notificationagain. See consolidated comment for the shared fix.🤖 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/test_phase.py` around lines 622 - 667, The success-notification setup is duplicated between the compare-version path and the unconditional slack-notification block. Consolidate the shared model, accelerator, engine-args, tensor/data-parallel, and notification behavior around run_regression_check and send_success_notification so the no-regression path cannot send duplicate notifications while preserving notifications when comparison is not requested.projects/rhaiis/orchestration/analysis.py (1)
168-172: 🎯 Functional Correctness | 🔵 TrivialDuplicate success-notification setup vs.
_sync_postprocessed_dashboard_csv.This branch (lines 200-214) computes
tp/dp/slack_userand callssend_success_notificationwith the same shape of arguments used intest_phase.py's_sync_postprocessed_dashboard_csv. See consolidated comment for the shared fix.Also applies to: 189-214
🤖 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/analysis.py` around lines 168 - 172, Remove the duplicated tensor/data parallel argument and Slack-user setup from the branch around send_success_notification, and reuse the existing shared notification logic used by _sync_postprocessed_dashboard_csv. Preserve the current argument values and success-notification behavior while centralizing construction of the notification inputs in one helper or shared path.
🤖 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/mlflow_backend.py`:
- Around line 416-420: Update log_artifacts() and log_multi_run_artifacts() so
that when resuming with run_id and a resolved run_name, they set the
mlflow.runName tag inside the active MLflow run context before applying other
metadata. Preserve the existing start_kw handling and apply this behavior
consistently in both artifact logging paths.
In `@projects/caliper/orchestration/export.py`:
- Around line 364-403: Update build_mlflow_run_url() to validate tracking_uri
with the existing assert_tracking_uri_has_no_userinfo() guard before composing
the Slack-visible URL, preserving the current empty-string fallback when
validation fails. Do not log or include the URI or any validation error details,
and leave the scheme check and URL construction unchanged for valid
credential-free URIs.
- Around line 267-335: Ensure the MLflow workspace restoration around
precreate_mlflow_run is executed in a finally block, including when
mlflow.set_tracking_uri, mlflow.set_experiment, mlflow.start_run, or another
operation inside the connection context raises. Keep the existing restoration
behavior for both previously set and unset MLFLOW_WORKSPACE values, while
preserving the function’s current return and logging flow on success.
---
Nitpick comments:
In `@projects/rhaiis/orchestration/analysis.py`:
- Around line 168-172: Remove the duplicated tensor/data parallel argument and
Slack-user setup from the branch around send_success_notification, and reuse the
existing shared notification logic used by _sync_postprocessed_dashboard_csv.
Preserve the current argument values and success-notification behavior while
centralizing construction of the notification inputs in one helper or shared
path.
In `@projects/rhaiis/orchestration/test_phase.py`:
- Around line 622-667: The success-notification setup is duplicated between the
compare-version path and the unconditional slack-notification block. Consolidate
the shared model, accelerator, engine-args, tensor/data-parallel, and
notification behavior around run_regression_check and send_success_notification
so the no-regression path cannot send duplicate notifications while preserving
notifications when comparison is not requested.
🪄 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: bf88813e-4411-4f6b-b653-01f70cb71772
📒 Files selected for processing (10)
.gitignoreprojects/caliper/engine/file_export/mlflow_backend.pyprojects/caliper/orchestration/export.pyprojects/rhaiis/orchestration/analysis.pyprojects/rhaiis/orchestration/config.d/rhaiis.yamlprojects/rhaiis/orchestration/config.yamlprojects/rhaiis/orchestration/test_phase.pyprojects/rhaiis/postprocess/csv_export.pyprojects/rhaiis/postprocess/plugin.pyprojects/rhaiis/postprocess/regression.py
Contributor
|
@Harshith-umesh #155 has been merged, so you can rebase on top of |
openshift-ci
Bot
added
the
needs-rebase
Harshith-umesh
force-pushed
the
mlflow-url-in-csv
branch
from
August 4, 2026 00:16
a38dd81 to
883eb59
Compare
openshift-ci
Bot
removed
the
needs-rebase
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/caliper/orchestration/export.py`:
- Around line 405-406: Update the URL construction near the tracking URI return
to URL-encode the workspace value before inserting it into the query string,
preserving the existing omission of the query parameter when workspace is empty.
Use the module’s existing URL-encoding utility or import the standard
query-string encoder rather than interpolating the raw workspace value.
- Around line 295-300: Update run_from_orchestration_config() to resolve and
load the file-based backend.mlflow.config YAML before looking up experiment and
workspace or creating the run. Ensure both inline mappings and YAML paths
populate the existing experiment and workspace settings consistently for resumed
runs.
- Around line 312-331: Update the MLflow run setup around
mlflow.set_tracking_uri to capture the current tracking URI before applying
tracking_uri, then restore that saved URI in the existing finally block
alongside MLFLOW_WORKSPACE restoration. Preserve the current behavior when no
tracking_uri is provided.
🪄 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: 6fcd1721-fbce-4f74-8fad-230aecc26972
📒 Files selected for processing (12)
.gitignoreprojects/caliper/engine/file_export/artifacts_export_run.pyprojects/caliper/engine/file_export/mlflow_backend.pyprojects/caliper/orchestration/export.pyprojects/rhaiis/orchestration/analysis.pyprojects/rhaiis/orchestration/config.d/rhaiis.yamlprojects/rhaiis/orchestration/config.d/workloads.yamlprojects/rhaiis/orchestration/config.yamlprojects/rhaiis/orchestration/test_phase.pyprojects/rhaiis/postprocess/csv_export.pyprojects/rhaiis/postprocess/plugin.pyprojects/rhaiis/postprocess/regression.py
🚧 Files skipped from review as they are similar to previous changes (9)
- projects/rhaiis/postprocess/plugin.py
- projects/rhaiis/postprocess/regression.py
- projects/caliper/engine/file_export/mlflow_backend.py
- projects/rhaiis/orchestration/config.d/rhaiis.yaml
- projects/rhaiis/orchestration/analysis.py
- .gitignore
- projects/rhaiis/orchestration/config.yaml
- projects/rhaiis/orchestration/test_phase.py
- projects/rhaiis/postprocess/csv_export.py
kpouget
reviewed
Aug 4, 2026
kpouget
reviewed
Aug 4, 2026
kpouget
reviewed
Aug 4, 2026
kpouget
reviewed
Aug 4, 2026
kpouget
reviewed
Aug 4, 2026
kpouget
reviewed
Aug 4, 2026
kpouget
reviewed
Aug 4, 2026
kpouget
reviewed
Aug 4, 2026
kpouget
reviewed
Aug 4, 2026
kpouget
reviewed
Aug 4, 2026
kpouget
reviewed
Aug 4, 2026
kpouget
reviewed
Aug 4, 2026
…k notifications Pre-create the MLflow run during the test step (before CSV generation) so the run_id and experiment_id are available for: - Dashboard CSV columns (mlflow_run_id, mlflow_experiment_id) - Slack notifications (MLflow run URL constructed at runtime from vault) The export step resumes the pre-created run instead of creating a new one, via the existing run_id parameter on both single-run and multi-run paths. Co-authored-by: Cursor <cursoragent@cursor.com>
kpouget
reviewed
Aug 6, 2026
…quires - Use @requires decorator for config access, matching mlflow_verifier pattern - Return full mlflow_destination dict (run_id, experiment_id, workspace) - Remove function from rhaiis test_phase.py; import from caliper - Mark mlflow_destination and experiment_id as optional in docs Co-authored-by: Cursor <cursoragent@cursor.com>
Member
Author
|
/test fournos rhaiis nvidia benchmark hera ci-quick |
🟢 Execution of
|
🟢 Submission of
|
The KPI run_path points deep inside the benchmark results, several levels below where __test_labels__.yaml is written. Walk up from run_path instead of only checking the immediate parent. Co-authored-by: Cursor <cursoragent@cursor.com>
Member
Author
|
/test fournos rhaiis nvidia benchmark hera ci-quick |
🟢 Execution of
|
🟢 Submission of
|
run_path in KPI records is relative, so the CSV plugin subprocess cannot resolve __test_labels__.yaml on disk. Instead, extract mlflow_destination from test nodes during compute_kpis and include mlflow_run_id/mlflow_experiment_id in KPI record labels. Co-authored-by: Cursor <cursoragent@cursor.com>
Member
Author
|
/test fournos rhaiis nvidia benchmark hera ci-quick |
🟢 Execution of
|
🟢 Submission of
|
Contributor
|
/lgtm |
The config-aware wrapper only uses caliper config keys, so it belongs in caliper/orchestration/export.py where all projects can reuse it. regression.py now imports and calls build_mlflow_run_url_from_config(). Co-authored-by: Cursor <cursoragent@cursor.com>
|
[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 |
Member
Author
|
/test fournos rhaiis nvidia benchmark hera ci-quick |
🟢 Execution of
|
🟢 Submission of
|
Contributor
|
/lgtm |
Member
Author
|
/approved |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pre-creates the MLflow run during the test step so its
run_idandexperiment_idare available before CSV generation. The export step resumes this run instead of creating a new one, ensuring the dashboard CSV and Slack notifications link to the correct MLflow entry.Changes
Pre-create MLflow run (
test_phase.py): New_precreate_mlflow_run()helper creates an MLflow run early in the test phase, capturingrun_idandexperiment_id. SetsMLFLOW_WORKSPACEandFJOB_NAMEto match the export step's context. Writes a marker file (__mlflow_precreated_run__.yaml) for cross-step persistence.Resume pre-created run on export (
export.py,mlflow_backend.py):_discover_precreated_mlflow_run_id()looks for the marker file first, then falls back to readingmlflow_run_idfrom__test_labels__.yaml(already discovered by the export step). Bothlog_artifacts()andlog_multi_run_artifacts()now passrun_namealongsiderun_idso the run name is set correctly on resume.Dashboard CSV (
csv_export.py,plugin.py): Addedmlflow_run_idandmlflow_experiment_idcolumns toFIELDNAMESand_extract_row(). The postprocess plugin maps these from test labels into CSV rows.Slack notifications (
regression.py): New_build_mlflow_run_url()constructs the MLflow URL at runtime from vault secrets (tracking URI) and config (run ID, experiment ID, workspace). Both regression and failure notifications include a clickable MLflow link.Test plan
forge-full) and verify:mlflow_run_idandmlflow_experiment_idFJOB_NAME(not auto-generated)pd.concathandles schema evolution (old CSV rows get NaN for new columns)Summary by CodeRabbit