Embed MLflow run ID in dashboard CSV and Slack notifications - #2 - #153
Open
Harshith-umesh wants to merge 19 commits into
Open
Embed MLflow run ID in dashboard CSV and Slack notifications - #2#153Harshith-umesh wants to merge 19 commits into
Harshith-umesh wants to merge 19 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[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 |
|
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>
config.project.set_config() is in-memory only and doesn't survive across pipeline steps (separate process invocations). Write the pre-created run_id to a __mlflow_precreated_run__.yaml marker file that the export step discovers by scanning the artifact tree. Co-authored-by: Cursor <cursoragent@cursor.com>
The pre-created run was landing in the wrong workspace/experiment (233 instead of 264) because MLFLOW_WORKSPACE was not set. The export step sets it, so the same experiment name resolved to a different experiment ID, and the export couldn't resume the pre-created run. Co-authored-by: Cursor <cursoragent@cursor.com>
The marker file approach was fragile — if _write_mlflow_precreated_run_marker() failed, the exception was silently caught and the export step created a new MLflow run instead of resuming the pre-created one. Two changes: - export.py: _discover_precreated_mlflow_run_id() now falls back to reading mlflow_run_id from __test_labels__.yaml in already-discovered run_dirs - test_phase.py: marker write is isolated in its own try/except so failures are logged separately from the pre-creation itself Co-authored-by: Cursor <cursoragent@cursor.com>
…un_id key config.project.set_config() raises KeyError when the key doesn't exist in the config schema. This was the root cause of the marker file never being written — the KeyError was caught by the outer try/except before the marker write could execute. The set_config calls are unnecessary since the run_id is communicated via test labels and the marker file, not in-memory config. Co-authored-by: Cursor <cursoragent@cursor.com>
Pre-created runs got MLflow's auto-generated name (e.g. rambunctious-fowl-43) because start_run() was called without run_name. The export step's resume path also skipped run_name when run_id was set (elif branch). Two fixes: - Pre-creation: pass FJOB_NAME as run_name to start_run() - Resume: pass both run_id and run_name so the name is updated on resume Co-authored-by: Cursor <cursoragent@cursor.com>
Addresses reviewer feedback: rhaiis orchestration/postprocess must not import caliper engine code directly. Moved to caliper/orchestration/export.py: - precreate_mlflow_run() (was _precreate_mlflow_run in test_phase.py) - write_mlflow_precreated_run_marker() (was _write_mlflow_precreated_run_marker) - build_mlflow_run_url() (was _build_mlflow_run_url in regression.py) rhaiis files now import from projects.caliper.orchestration.export. Co-authored-by: Cursor <cursoragent@cursor.com>
… helpers to caliper - Add send_success_notification() for clean pipeline completions - Wire tests.rhaiis.slack_notify_always config to trigger it - Add dashboard link (gated on csv_dashboard.enabled) and MLflow link - Move precreate_mlflow_run, write_mlflow_precreated_run_marker, and build_mlflow_run_url from rhaiis to caliper/orchestration/export.py Co-authored-by: Cursor <cursoragent@cursor.com>
The config keys caliper.export.mlflow_run_id/experiment_id don't exist in the schema, so get_config returns None and the URL is never built. Now reads from the __mlflow_precreated_run__.yaml marker file on disk. Co-authored-by: Cursor <cursoragent@cursor.com>
- mlflow_backend.py: set mlflow.runName tag explicitly when resuming a run with run_id, since some MLflow versions ignore run_name in start_run() during resume - export.py: wrap MLFLOW_WORKSPACE restoration in finally block so it executes even if set_experiment/start_run raises - export.py: validate tracking_uri with assert_tracking_uri_has_no_userinfo() before composing the Slack-visible MLflow URL Co-authored-by: Cursor <cursoragent@cursor.com>
…run_id The marker file is always written reliably; the test-labels fallback was redundant defensive code. Co-authored-by: Cursor <cursoragent@cursor.com>
Import was lost during rebase when build_mlflow_run_url() was rewritten to read from the marker file. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Revert agent_analysis.url change and workload rampup additions — these belong in a separate PR per reviewer feedback. Co-authored-by: Cursor <cursoragent@cursor.com>
- Create projects/caliper/public/file_export.py to expose engine functions; orchestration imports from public instead of engine - Refactor precreate_mlflow_run() to accept secrets_path, experiment, workspace from caller instead of reading project config - Combine precreate_mlflow_run() and write_mlflow_precreated_run_marker() into a single call - Refactor build_mlflow_run_url() to accept secrets_path and workspace from caller - Move config reads to rhaiis callers (test_phase.py, regression.py) - Save/restore tracking URI in finally block alongside MLFLOW_WORKSPACE - URL-encode workspace in MLflow URL query string - Add logger.warning for all failure return paths - Warn on invalid marker YAML object type - Remove bare except swallowing in _read_mlflow_ids_from_marker - Add docstring explaining MLFLOW_PRECREATED_RUN_MARKER purpose - Revert unrelated rhaiis changes (agent_analysis.url, workloads rampup) Co-authored-by: Cursor <cursoragent@cursor.com>
Harshith-umesh
force-pushed
the
mlflow-url-in-csv
branch
from
August 4, 2026 18:05
9731846 to
1325e18
Compare
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/orchestration/export.py`:
- Around line 355-363: Update the marker-scanning logic to catch file-read and
YAML parsing errors for each marker, log or skip the malformed entry, and
continue scanning so the empty-ID fallback remains reachable. In the marker
handling around the existing data mapping check, require a non-empty string
run_id before returning it, and only return a validated string experiment_id as
appropriate for the existing contract.
🪄 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: 969f9e5c-da09-43b8-a619-c6cbf8e8013c
📒 Files selected for processing (5)
projects/caliper/orchestration/export.pyprojects/caliper/public/__init__.pyprojects/caliper/public/file_export.pyprojects/rhaiis/orchestration/test_phase.pyprojects/rhaiis/postprocess/regression.py
🚧 Files skipped from review as they are similar to previous changes (2)
- projects/rhaiis/postprocess/regression.py
- projects/rhaiis/orchestration/test_phase.py
The slack_notify_always notification was inside _sync_postprocessed_dashboard_csv() which returns early when csv_dashboard.enabled is false. Move to a standalone function called from run() so it fires independently. Co-authored-by: Cursor <cursoragent@cursor.com>
…cess_notification Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 319-332: The export flow around mlflow.set_experiment and the
finally restoration block must preserve the caller’s active MLflow experiment.
Capture the current experiment before switching to experiment, then restore it
in finally alongside MLFLOW_WORKSPACE and the tracking URI, including the prior
default/no-experiment state.
- Around line 421-422: Update the exception handler around reading the MLflow
pre-created run marker to catch yaml.YAMLError without binding or logging the
exception text. Keep the generic warning and existing skip behavior, while
retaining OSError handling as appropriate.
🪄 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: 8d7573c5-2ff3-4864-bc3f-3d9372ba279e
📒 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/caliper/public/__init__.pyprojects/caliper/public/file_export.pyprojects/rhaiis/orchestration/analysis.pyprojects/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 (11)
- projects/rhaiis/postprocess/plugin.py
- projects/rhaiis/postprocess/csv_export.py
- projects/caliper/public/file_export.py
- projects/caliper/public/init.py
- projects/rhaiis/orchestration/config.yaml
- projects/caliper/engine/file_export/mlflow_backend.py
- projects/rhaiis/orchestration/analysis.py
- .gitignore
- projects/caliper/engine/file_export/artifacts_export_run.py
- projects/rhaiis/postprocess/regression.py
- projects/rhaiis/orchestration/test_phase.py
Move personal ignore patterns (fournos-job-*.yaml, kubeconfig*) to .git/info/exclude per reviewer feedback. Co-authored-by: Cursor <cursoragent@cursor.com>
Member
Author
|
/test fournos rhaiis nvidia |
🔴 Execution of
|
🔴 Submission of
|
Member
Author
|
/test fournos rhaiis nvidia |
🟢 Execution of
|
🟢 Submission of
|
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