feat(mlflow): load harness-snapshot from disk and MLflow - #157
astefanutti merged 6 commits into
Conversation
Disk handoff at log_results; fetch_harness_snapshot for later readers using the same run-name join as inputs/ artifacts. Co-authored-by: Cursor <cursoragent@cursor.com>
Vertical-slice check: fullsend-shaped harness-snapshot.json projects to Provenance/MLflow tag names, including MLflow fetch round-trip. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds CI context collection from local harness snapshots, MLflow artifacts, or CI environment variables. Maps snapshot and provenance fields into MLflow tags, supports source precedence and protected configuration merging, and logs discovered snapshots as MLflow artifacts. Updates MLflow configuration documentation and adds tests for mapping, discovery, retrieval, precedence, fallback, merging, malformed input, and unsafe run-name validation. Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
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: 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 `@agent_eval/ci_context.py`:
- Around line 130-139: Make harness snapshot loading best-effort by catching
invalid or truncated JSON errors at each caller: collect_from_snapshot_dir, the
HARNESS_SNAPSHOT_PATH branch in from_disk, and the load_harness_snapshot(path)
call in fetch_harness_snapshot. On failure, preserve the existing fallback/None
behavior so collect_ci_context continues logging tags, artifacts, traces, and
feedback instead of propagating the snapshot parsing exception.
- Around line 174-192: Replace the broad silent exception handlers in the MLflow
run lookup and artifact-download loop with targeted exception handling that logs
the failure details through the existing logging mechanism, while preserving the
current None/continue fallback behavior. Also ensure load_harness_snapshot
failures are handled consistently with the companion error-handling path rather
than remaining unguarded.
- Around line 169-173: Sanitize eval_run_id before constructing the
filter_string passed to search_runs: escape or reject quote and backslash
characters so caller-controlled values cannot alter the MLflow tag-filter
expression. Keep the existing run-name lookup behavior unchanged for valid
identifiers.
In `@skills/eval-mlflow/scripts/log_results.py`:
- Around line 287-301: Update the collect_ci_context call in the MLflow tag
setup to forward the already-computed experiment_id, enabling the documented
MLflow-artifact fallback. Wrap this context collection and tag-application block
in the same appropriate exception-handling pattern used by nearby trace-search,
link, and feedback calls, so malformed harness snapshots do not abort artifact
logging or later run processing.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c0b8744c-cb93-407c-8975-43099071f12c
📒 Files selected for processing (4)
agent_eval/ci_context.pyskills/eval-analyze/references/eval-yaml-template.mdskills/eval-mlflow/scripts/log_results.pytests/test_ci_context.py
Best-effort JSON load so corrupt snapshots do not abort log_results; forward experiment_id for artifact fallback; reject unsafe run-name filter chars; warn on MLflow search/download failures. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed CodeRabbit review:
On the CWE-532 note about |
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 `@skills/eval-mlflow/scripts/log_results.py`:
- Around line 290-300: Ensure the tag-merging logic around merge_mlflow_tags and
the mlflow.set_tag loop always preserves the mlflow.runName value established by
mlflow.start_run() from args.run_id, ignoring any
config.mlflow.tags["mlflow.runName"] override. Add coverage for this override
case and document that mlflow.runName is immutable because provenance consumers
depend on it.
In `@tests/test_ci_context.py`:
- Around line 203-214: Update test_fetch_rejects_unsafe_eval_run_id to provide a
mock search_runs callable, retain its reference, and assert it was never called
after fetch_harness_snapshot receives the unsafe run ID. Keep the existing None
result and client.download_artifacts assertion.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f3f6182b-5804-44f8-b1e4-95d6860abfc0
📒 Files selected for processing (3)
agent_eval/ci_context.pyskills/eval-mlflow/scripts/log_results.pytests/test_ci_context.py
Join consumers look up runs by start_run's name; ignore config overrides and assert unsafe eval_run_id never hits search_runs. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_ci_context.py (1)
185-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert preservation of an existing
mlflow.runName.This test only verifies that configuration cannot add the key. It would still pass if
merge_mlflow_tagsaccidentally discarded an existingmlflow.runName, despite the artifact lookup contract requiring preservation.Proposed test adjustment
merged = merge_mlflow_tags( - {"eval_run_id": "r1"}, + {"eval_run_id": "r1", "mlflow.runName": "original"}, {"mlflow.runName": "hijacked", "team": "ml"}, ) - assert "mlflow.runName" not in merged + assert merged["mlflow.runName"] == "original" assert merged["team"] == "ml"🤖 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 `@tests/test_ci_context.py` around lines 185 - 194, Update test_merge_mlflow_tags_preserves_run_name to include an existing mlflow.runName in the base tags, then assert the merged result retains that original value while still rejecting the configuration override and preserving unrelated 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.
Nitpick comments:
In `@tests/test_ci_context.py`:
- Around line 185-194: Update test_merge_mlflow_tags_preserves_run_name to
include an existing mlflow.runName in the base tags, then assert the merged
result retains that original value while still rejecting the configuration
override and preserving unrelated tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e54bedb7-1c9a-46da-af51-89cb38a752fa
📒 Files selected for processing (4)
agent_eval/ci_context.pyskills/eval-analyze/references/eval-yaml-template.mdskills/eval-mlflow/scripts/log_results.pytests/test_ci_context.py
🚧 Files skipped from review as they are similar to previous changes (3)
- skills/eval-analyze/references/eval-yaml-template.md
- skills/eval-mlflow/scripts/log_results.py
- agent_eval/ci_context.py
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Refreshed onto current |
astefanutti
left a comment
astefanutti
left a comment
There was a problem hiding this comment.
Thanks @HofniGartner!
2 participants
Summary
Optional join-key support for eval runs: if the agent runtime left
harness-snapshot.jsonunder the run/case output,log_resultsprojectsfields to MLflow tags and uploads the file as an artifact. Later readers
fetch that artifact from MLflow (same run-name join as
inputs/), not byscraping CI env.
Changes
agent_eval/ci_context.py: find/load snapshot on disk; map join fieldsto tags (
commit_sha←ref_revision,harness_fingerprint←harness_content_sha);fetch_harness_snapshot(experiment_id, eval_run_id);CI env only as last-resort fallback
log_results.pyto collect tags from the run dir andmlflow.log_artifactthe snapshot when presenteval-yaml-template.mdproducer-shaped snapshot → join tags round-trip
Test plan
python -m pytest tests/test_ci_context.py -qlog_resultsagainst a run dir that containscases/<id>/harness-snapshot.jsonand confirm tags + artifact on theMLflow run
Notes
behavior is unchanged aside from optional CI-env tag fallback
Made with Cursor
Summary by CodeRabbit