Repository navigation
fix(exporters): reopen a tracking run only on resume, and log each landed rollout once - #5029
Conversation
…log each rollout once NVIDIA-NeMo#4762 reopens the MLflow or W&B run recorded next to an output, so that a resumed or resubmitted run stays one tracking run. Three cases went wrong: - A fresh run into an output that held an earlier run reopened that run, merging two evaluations. A run now reopens only when it resumes its output (resume_from_cache); otherwise it starts a new run. - A resumed collection hands the exporters every rollout of its output, and MLflow's log_table appends, so a reopened run's table held the earlier rollouts twice. A reopened MLflow run now logs only rollouts its table does not hold yet. - A deleted MLflow run could not be reopened, so logging stopped for that output. A new run now takes its place. The new tests run against a real local MLflow store. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Adam Rajfer <arajfer@nvidia.com>
…empt A reopened MLflow run skipped every rollout whose task and rollout index its table already held, failed attempts included. A rollout that failed in one job and landed on resume therefore kept only its failure record in the table. Now only the landed rollouts the table holds are skipped; a failed attempt stays in the table as it was logged, next to the later landing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Adam Rajfer <arajfer@nvidia.com>
|
/ok to test 34f6464 |
svcnvidia-nemo-ci
added
area:evaluation
|
🌿 Preview your docs: https://nvidia-preview-pr-5029-37a1de64b574.docs.buildwithfern.com/nemo/gym |
|
/review |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
HOLD — one RISK: explicit resubmissions can start a new tracking run instead of reopening the evaluation they continue.
Reviewed all three changed files and traced exporter setup, rollout upload, and Slurm resubmission paths. The fresh-run separation, MLflow landed-rollout deduplication, and deleted-run replacement are otherwise consistent with the added tests. Static review only; tests were not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
A resubmitted job adds only resubmit=True to its config. Collection turns that into a resume, but the exporters open earlier, while the global config is parsed. The reopen gate therefore saw no resume and started a new MLflow or W&B run for the job's continuation. A resubmit now reopens the recorded run, as a resume does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Adam Rajfer <arajfer@nvidia.com>
|
/ok to test 71ddaf9 |
|
/review reset=true |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
HOLD — three RISK findings remain: a retried rollout can disappear from MLflow, append-mode recovery splits tracking history, and a cache-miss restart can retain stale rollout records instead of its new results.
The earlier resubmit finding is fixed: the shared reopen gate now honors resubmit, with regression cases for both backends.
Completed the broad review of all three changed files and traced collection, reverification, exporter lifecycle, and upload contracts. Static review only; tests were not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
…ntinues The reopen gate and the landed-rollout filter got three cases wrong: - A no-persist attempt is uploaded but written nowhere, yet it counted as landed, so a rollout that landed later for the same task was left out of the MLflow table. A rollout counts as landed now only without a failure class and without the no-persist flag. - Judge-failure recovery in append mode adds its rows to the output it recovers, but started a new tracking run. It reopens the recorded run now, as a resume does. - A resume that finds nothing to resume starts its output anew, after the exporters reopened the recorded run, so the MLflow filter dropped fresh rollouts whose keys were in the old table. Collection now tells the exporters: they leave the old run as the earlier job left it and track the output in a new run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Adam Rajfer <arajfer@nvidia.com>
|
/ok to test 3135e29 |
|
/review reset=true |
github-actions
Bot
added
the
sla:review-overdue
Signed-off-by: Adam Rajfer <arajfer@nvidia.com>
|
Fixed one additional reopen case in 45a868c: the exporter read continuation flags by raw truthiness, while collection and reverification parse them as booleans. For example, The exporter now uses the same boolean parsing for Validation: 112 exporter tests passed, including real MLflow file-store checks. The three new false-string regression cases fail on the parent commit and pass with this fix; true-string cases still reopen correctly. Existing review threads already have responses and fixes, so I have left those replies unchanged. |
|
Two notes, neither blocking. Orchestration leaking into core.
Relation to #4914. They're complementary: this PR is the live path (reopen on resume, no double logging), #4914 is the post-run path (one snapshot into a fresh run, never reopens). A submit can't enable both for the same backend ( |
Resubmitted Slurm jobs set resume_from_cache before exporters open. Normalize the resubmit command in older saved job scripts under the existing lock, preserving their tracking identity and rollout artifacts. Exporters now use only resume and append-recovery flags. Signed-off-by: Adam Rajfer <arajfer@nvidia.com>
|
Thanks, I took the smaller option in 37a1de6. Resubmits now set I checked the rendered arguments against both exporters and reran the local MLflow recovery coverage. I'll leave the marker distinction and post-run guard to your #4914 changes. |
prokotg
left a comment
prokotg
left a comment
There was a problem hiding this comment.
Approved, but will nudge about core complete decoupling soon
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/40551701498 |
Conflicts: - rollout_collection.py: #5029 tells the exporters to start a new tracking run when a resume finds nothing to resume. #3302 makes that decision in RolloutStore.start_or_resume, so the store now reports started_fresh and the collector calls start_over() after it when a resume was requested. - single_agent_turn_legacy tests: kept both new tests (#4949's skip_verification test and #3302's). Tests from main adapted to #3302's rules: - A run with history whose output or materialized inputs are missing refuses to resume rather than start over, so it keeps its tracking run. The start-over case is a resume with nothing saved. - The string-flag collection test supplies the running servers' configuration, which the run manifest fingerprints. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com>
Keep MLflow and W&B tracking aligned with the output that collection or reverification actually continues. Fresh runs create fresh tracking runs; resume, resubmit and append recovery continue the recorded run. Reopened MLflow runs filter already-landed rollout rows. W&B keeps its existing table-upload behavior.
Slurm resubmission now sets
resume_from_cache=Truebefore exporters open, including older savedjob.shscripts normalized under the existing resubmit lock. Exporters check only resume and append recovery, using the same boolean parsing as collection and reverification, including"false".flowchart TD A[Start collection or reverification] --> B{Continue existing output?} B -->|Yes| C[Reopen recorded tracking run] B -->|No| D[Create fresh tracking run] C --> E[Export metrics and rollouts] D --> EA reopened run holds new configuration until continuation is confirmed, so restarting output cannot overwrite the old run's parameters. Append-mode judge-failure recovery remains a continuation. Deleted MLflow runs are replaced; failed or no-persist attempts do not suppress a later landed result. Reverification continues to use saved answers.
Validation: focused resubmit, Slurm-script, exporter and restart suites pass, including local MLflow file-store coverage. The orchestration regressions fail against the prior methods and pass with this change; W&B lifecycle checks use mocks. Targeted pre-commit hooks pass. CI for the updated head is still running.
Marker cleanup remains best-effort: if deletion and replacement both fail, a warning identifies stale-marker risk. Direct exporter callers must export or tear down to flush pending configuration. When combined with #4914, its post-run no-resume guard must stay ahead of the live continuation checks; the shared marker's completion semantics are handled on the #4914 side.