Repository navigation
feat(eval): export a finished run to MLflow or W&B with gym eval export - #4914
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test 5f0d7c1 |
|
/review |
|
🌿 Preview your docs: https://nvidia-preview-pr-4914-0e4dcd944e7a.docs.buildwithfern.com/nemo/gym |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
HOLD — four RISK findings affect export completeness, configuration handling, retryability, and W&B authentication. One NOTE corrects the credential-source documentation.
Reviewed all 10 changed files and traced the affected CLI, artifact-writing, backend, and regression-test paths. This was a static review; tests and backend calls were not executed. Suggested patches and proposed checks require verification before adoption.
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.
5f0d7c1 to
88ecef4
Compare
|
/ok to test 88ecef4 |
88ecef4 to
d9d5c32
Compare
|
/ok to test d9d5c32 |
a742002 to
60b400b
Compare
60b400b to
44ea31d
Compare
AdamRajfer
left a comment
There was a problem hiding this comment.
Two targeted comments on export completion state and integration with #5029. The second concerns inherited live behavior.
| marker = artifacts.exporter_run_id_fpath(self.name) | ||
| if marker.exists(): | ||
| return ExportResult.skipped(reason="already exported", run_id=marker.read_text().strip()) |
There was a problem hiding this comment.
This marker proves that a tracking run was opened, not that its uploads completed. If live rollout upload fails, evaluation can still finish with valid artifacts, but a later gym eval export mlflow returns 'already exported' without attempting the missing upload. I reproduced that with a fake backend; removing the marker lets export succeed. Could we distinguish resumable run identity from completion evidence for the requested export? The live ID still needs to be written during setup for resume.
There was a problem hiding this comment.
Good catch, thanks for reproducing it. A post-run export now has its own marker, <stem>_<name>_export_done (written atomically, only after everything was sent) and doesn't touch <stem>_<name>_run_id anymore, so the live path keeps the run id file to itself. Fixed in bbcf507.
One behavior change: a live run id no longer makes gym eval export skip. The export goes ahead as a separate run, logs a warning and returns live_run_id. So a run logged live and then exported by hand can show up twice, which I think beats a silent skip, since the live id only says the run opened. gym eval submit already refuses live + the same export for a backend, so it can't hit this. Making the live path record real completion would mean touching every export site in collection and reverification, so I left that out of this PR.
Covered by tests, and I ran live-then-export, export-then-live-resume, re-export and failed-then-retried upload against a real local MLflow store.
| if self._exported_artifacts is not None: | ||
| return None |
There was a problem hiding this comment.
On the live path this still reopens an existing ID when resume_from_cache=False, so a fresh evaluation at the same output path logs into the previous evaluation. #5029 fixes that decision, including resubmit and cache-miss fallback. I suggest merging #5029 first and rebasing this stack on main. Here, retain the post-hoc return None first, then apply #5029's continuation check to live calls; keep pending post-hoc ID publication separate.
There was a problem hiding this comment.
Done, the stack is rebased and merged with it the way you described: post-run return None first, your continuation check for live calls, and the pending post-run id kept separate. On top of that the post-run export no longer shares the run id file with the live path (see the other thread).
|
Blocked by #5029 (live exporters: reopen the tracking run only on resume, log each landed rollout once). Both PRs rewrite Removing |
|
/review |
BaseExporter gains export(artifacts): it logs the run's saved config, aggregate metrics and rollouts through the same hooks live runs use, skips a run it already exported, and writes the run id marker only after the whole run is sent. Live and post-run metrics share one flattener. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…inished runs Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…mplete Refuse an aggregate file with a failed agent instead of exporting a partial result, resolve only the upload setting of an interpolated saved config, publish the already-exported marker atomically, keep a WANDB_API_KEY from the environment when the command sets none, and name env.yaml in the docs. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…ide and the upload filter Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…y users Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…re not uploaded The rollouts file is now only required when the rollouts are uploaded, and the freshness check compares against it only when it is there. The error for a missing file says how to skip the upload. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…tead of writing an empty marker Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…'s run id A live run writes its run id when the backend opens, so that file shows the run opened, not that every upload landed. Sharing it as the already-exported marker made gym eval export skip a run whose live upload had failed, and made a later live resume reopen the export's run. A post-run export now writes <stem>_<name>_export_done and ignores the live run id, logging and reporting it when there is one. The live path reads and writes only <stem>_<name>_run_id. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
|
/ok to test 0e4dcd9 |
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — no eligible findings remain in this follow-up.
Verified against current source and regression assertions:
- Failed-agent aggregates are rejected before backend setup; repaired aggregates export both agents. Selective upload-setting resolution preserves canonical-key precedence and unrelated unresolved expressions (implementation, regressions).
- Completion-marker publication remains atomic, with partial-write failure leaving no marker and allowing retry. Post-run completion markers are separate from live resumable IDs (publication, regressions).
- W&B retains an inherited API key unless command configuration overrides it (setup, credential assertions). The documentation names
env.yaml, matching the parser.
Also checked optional-rollout export, missing backend IDs, command/live setup separation, and live-resume isolation along their directly affected paths. No eligible introduced regression was established.
This was a static follow-up of review 5663786908, not a fresh whole-PR review. The previously reviewed tree was unavailable, limiting exact inter-review comparison. One delegated report was unavailable; its artifact-readiness/upload topic was independently inspected by the parent. Tests and backend calls 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 findings-only review. Request a full review with /review reset=true.
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/40685324258 |
Summary
gym eval export mlflow|wandbsends a finished run to the backend: its saved config, aggregate metrics and (optionally) rollouts.BaseExporterbecomes anExporterwithexport(artifacts), reusing the live exporters' hooks.<stem>_<exporter>_export_doneexists the run isskippedwithout contacting the backend. A post-run export writes it only after the whole run is sent (atomically, holding the backend run id), so a failed export can be retried and a failed write leaves none. It is separate from the<stem>_<exporter>_run_ida live run records for resume. A live run id does not make the export skip, because it only shows that the run opened, not that every upload landed: the export goes ahead as a separate run, logs a warning and returnslive_run_id.gym eval submitalready rejects a live exporter plus the same export for one backend, so only a manual export after a live run is affected. A post-run export never reads or writes the live run id, so a later live resume does not reopen the export's run._aggregate_metrics.jsonexists, is not older than the rollouts and records no failed agent (a partial result is never exported), and the saved config exists.upload_rolloutsis false (only that value is resolved, so an interpolated setting works);upload_rollouts=on the command overrides. Settings and credentials come from the export command, never from the masked saved config.flatten_aggregate_metricsreplaces two duplicate blocks in collection and reverification.WANDB_API_KEYin the environment is used when the command sets no key.skip_exportersparser option, so the export command does not also open a live exporter; a missing SDK error names the extra to install. CLI reference and the live exporter settings page updated.Part of #2977 (export-aware evaluation runs).
PR plan
Separate PRs; steps 1-8 are stacked (stack #4910). Goal: make
gym eval exportpluggable so a finished run can go to tracking systems without live exporters, andgym eval submitcan export after each benchmark.gym eval run(feat(eval): save the masked config beside rollouts from gym eval run #4908)RunArtifactsand path derivation helpers (feat(eval): add RunArtifacts to derive a run's files from its rollouts path #4909)Exporterbase class and registry, starting withatif, plus the positional exporter CLI argument (--formatdeprecated) (feat(eval): add the Exporter base class and registry, with atif as the first exporter #4911)mlflowandwandbexport: shared metrics flattening,BaseExporter.export()and the already-exported guardgym eval submitintegration: anexportmap with job-level defaults and per-benchmark overrides, validation, install extras, and a job-script tail (Slurm only at first) (feat(orchestration): run exports after each benchmark of gym eval submit #4974)Validation
test_record_exporters.py: export flow, done-marker timing and atomic write, its own file apart from the live run id, a live run id reported and not skipping, skip, failure and retry, preconditions (including a failed agent in the aggregate file), upload intent (including interpolated settings), W&B key from the environment,skip_exporters, missing-SDK error, mocked MLflow and W&B exports.pre-commit: passed.MLFLOW_ALLOW_FILE_STORE=true): one FINISHED run with the flattened metrics, params from the saved config, andglobal_config.jsonandrollouts.jsonartifacts; a second invocation printedskipped. The same real local store also covered: a live run then an export (two runs, the live marker untouched), a live resume (reopens the live run, not the export's), a fresh live run, and a failed upload then a retry (no marker, the retry succeeds). W&B is mocks only.Known gaps
coverage/*metrics; they are not in the aggregate file, so post-run export omits them.Rollout evidence
N/A: no agent, verifier or model behavior is touched. Live logging behaves as before; only the metrics flattening moved into a shared function.