Repository navigation
docs(contribute): add a guide to writing an exporter for gym eval export - #5077
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. |
a872db5 to
07e8f05
Compare
07e8f05 to
eb77d40
Compare
eb77d40 to
e6b3022
Compare
e6b3022 to
91bf6bc
Compare
900da59 to
f38079e
Compare
f38079e to
f4dbd30
Compare
f4dbd30 to
4be5e3f
Compare
4be5e3f to
316696f
Compare
e0980e3 to
5e743f2
Compare
|
/ok to test 5e743f2 |
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP
LGTM — no remaining eligible findings in this follow-up.
Verified all four previously reported failures are resolved in current source:
- The summary exporter distinguishes the reported same-filename runs, publishes through a temporary sibling, and returns expected input/output filesystem failures as
FAILED. - Acme preparation now guards precondition checks, directory creation, and copying before environment construction.
Checked the regression and preservation assertions, mirrored guide blocks, and directly affected exporter/worker contracts. The worker-environment stub accepts the interpreter argument used by production. The summary-I/O topic was independently checked because its delegated report was unavailable.
This was a targeted follow-up, not a fresh whole-PR review. The previously reviewed tree was unavailable, limiting inter-review comparison to supplied historical evidence and current source. No eligible new regression was established. Static inspection only; tests and the Fern build 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.
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP
LGTM — no remaining eligible findings in this follow-up.
Verified the four previously reported failures remain resolved in current source:
- The summary exporter distinguishes the reported same-filename runs, publishes only complete output, and returns input/output filesystem failures as
FAILED. - Acme preparation guards precondition checks, directory creation, and copying before environment construction.
Checked the relevant regression and preservation assertions, mirrored guide examples, and directly affected contracts for submit settings, credentials, worker launch/timeouts, and completion markers. The summary and Acme/worker topics were checked directly because two delegated reports were unavailable.
This was a targeted follow-up, not a fresh whole-PR review. The previously reviewed tree was unavailable, limiting inter-review comparison to supplied historical evidence and current source. No eligible introduced regression was established. Static inspection only; tests and the Fern build 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.
|
/ok to test f8cdcc7 |
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP
LGTM — no remaining eligible findings in this follow-up.
Verified that the four previously reported failures remain fixed:
- The summary exporter distinguishes the reported same-filename runs, publishes only complete output, and returns expected input/output filesystem failures as
FAILED. - Acme preparation guards precondition checks, directory creation, and copying before environment construction.
Checked the relevant regression and preservation assertions, mirrored examples, and affected contracts for submit settings, credential handling, worker launch/timeouts, completion markers, and per-export result retention. The summary and Acme/worker topics were checked directly because their delegated reports were unavailable.
This was a targeted follow-up, not a fresh whole-PR review. The previously reviewed tree was unavailable, limiting inter-review comparison to supplied historical evidence and current source. No eligible introduced regression was established. Static inspection only; tests and the Fern build were not executed.
Review generated by codex. This was a findings-only review. Request a full review with /review reset=true.
|
/ok to test 4f2d01c |
One page with the Exporter contract, registration, in-process versus isolated exporters, and a worked example of each. The example code lives in tests/unit_tests/exporter_guide, is run by tests, and a test checks that the page quotes it exactly. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…mple Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…er example Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
… run id Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…ide the returned-failure boundary Name the summary after the whole run path so runs sharing a rollouts filename do not skip each other, publish it atomically so a failed write is retried instead of skipped, and guard every input and preparation filesystem call so an OSError returns a failed result. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…terpreter Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…d isolated-exporter changes Say that submit credentials go through driver.env and not export settings, that an isolated exporter can bound its worker with worker_timeout_s, what the worker launch guarantees, how submit passes string settings to Hydra, that a settings model can define is_available, that ExportResult.failed takes exit_code as a keyword, that the example test replaces the worker environment to add PYTHONPATH, and that a post-run export keeps its own done marker. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…ugh its own export step Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…uide Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
|
/ok to test 240efbc |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/41672241110 |
Summary
gym eval export:contribute/exporters.mdx, with a card on the Contribute overview.Exportercontract (RunArtifacts, the settings model,ExportResultstates and exit codes), registration inrun_exporter_registry.pyand whatgym eval submitadds, when to choose an in-process or an isolated exporter, testing, and credential handling.summaryexporter (named per run, published atomically, every filesystem error returned asfailed), and an isolatedacmeexporter with its worker file.atifandmlfloware pointed to as real references.tests/unit_tests/exporter_guide/. Tests run it (the isolated one against a fakeacme_sdkthrough the real worker), and a test checks that the page's code blocks match those files exactly.Docs only; no behavior change. 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 guard (feat(eval): export a finished run to MLflow or W&B with gym eval export #4914)gym 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_exporter_guide.py: 18 passed (the examples also cover run-specific output names, atomic publishing and filesystem errors returned asfailed).pre-commit: passed.gym eval export summary ...succeeded and a second run reportedskipped(the temporary registry entries were reverted).Rollout evidence
N/A: documentation and tests only.