Repository navigation
feat(eval): export a finished run to EvalRDL with gym eval export evalrdl - #4965
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. |
|
/review |
|
/ok to test 5228263 |
|
🌿 Preview your docs: https://nvidia-preview-pr-4965-8f42e3fa859d.docs.buildwithfern.com/nemo/gym |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP WITH CARE — one RISK, no BLOCKER findings.
Reviewed all 10 changed files and their directly affected CLI, artifact, and exporter contracts. Expected filesystem failures during staging currently escape instead of returning a failed export result.
Validation was static only; no tests, SDK installation, or service calls were executed.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
5228263 to
c31638b
Compare
|
/ok to test c31638b |
c31638b to
c457915
Compare
A credential pasted into a URL under an ordinary key (base_url, a DSN, a
tracking URI) was written to the saved config and to the exporters in plain
text. The password in scheme://user:password@host is now masked and the rest
of the URL stays. This is only a precaution; the docs say secrets belong
behind ${oc.env:NAME} or env.yaml.
Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
The detector reads scheme://user:password@host in the tests, the docstring and the docs row as credentials. Reword the prose so it no longer carries the pattern, and allowlist the example URLs in the tests. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…s path One place that names every sidecar of a run (failures, materialized inputs, aggregate metrics, saved config, per-exporter run id marker) so exporters do not each rebuild paths. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…e first exporter gym eval export now takes an optional exporter name (default atif) and key=value settings. The ATIF conversion runs as an Exporter that returns an ExportResult; --format is deprecated. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…stem errors in gym eval export A captured override now follows the translated flags, hydra.* settings are not given a + prefix, and the ATIF exporter returns a failed result for OSError instead of raising. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
… flag Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…other commands gym eval export no longer adds + to bare key=value overrides, so it follows the Hydra convention the CLI reference describes (+key=value). The --format deprecation goes through the logger, and the atif settings docstring names the exporter instead of the deprecated flag. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
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>
… its own venv The SDK and its dependencies stay out of Gym's environment. The main process validates the config and stages files; a stdlib-only harness runs the exporter's worker in a locked, requirements-keyed venv, with the JSON payload on stdin and the result in a file. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
Anchor the venv root, interpreter and worker file to absolute paths before the launch changes into the staging directory, put the exporter venv first on the worker's PATH, register the worker module in sys.modules so dataclasses and postponed annotations work, fail a worker that reports a result but then exits non-zero, and return a failed result when the worker cannot be launched. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
… the worker's run time mlflow.py and wandb.py sit beside the harness, so a worker importing those SDKs would have loaded Gym's modules instead. An exporter can now give its worker a time limit; a worker that outlives it is killed and the export fails. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…lrdl The run is staged as one folder and uploaded by the EvalRDL SDK from the exporter's own environment. committed is succeeded, deduped is skipped, an upload without a verdict fails with exit code 3 and a refused one with exit code 4. The worker harness no longer puts its own directory on sys.path, which would have let evalrdl.py shadow the SDK. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…iled result Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
… staging named files A run is more than a fixed list of files (health reports, model-call captures, benchmark-specific files), so the export uploads the directory the rollouts are in as it is. Nothing is copied. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…e upload on the run's own directory With wait=false the SDK has no verdict to give, so an accepted upload is the expected outcome. Upload from the rollouts file's directory without following a symlink, refuse a directory that holds another run's rollouts file, bound the worker's run time, say when a stored run needs binding, and name what to do about an SDK that is too old or a missing token. Document the token variables, the service's package index and that evalrdl is not on PyPI. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
|
/ok to test 8f42e3f |
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — no remaining eligible findings in this follow-up.
The prior staging-copy RISK is resolved: build_payload no longer stages or copies files, eliminating the reported copy2 failure path. The current regression tests cover directory selection, symlink handling, neighboring-run rejection, and prerequisite failures.
Checked the identifiable subsequent changes and directly affected paths: accepted uploads without waiting, preserved wait/refusal exit codes, diagnostics, worker time limits, isolated-environment remediation, and associated documentation/tests. No new regression qualifying above the historical moderate-impact ceiling was established.
This was a follow-up, not a fresh whole-PR review. The previously reviewed tree was unavailable, limiting direct inter-review comparison. Validation was static only; no tests, SDK installation, or service calls were 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.
agronskiy
left a comment
There was a problem hiding this comment.
Review by Alex Gronskiy (EvalRDL side). All addressed, thanks. I checked the current head; the --with_evalrdl_sdk check passes against the released SDK 0.0.241. Approving so the train can merge.
Follow-up on the split check: the reason it was blocked no longer holds. SDK 0.0.241 (released yesterday) exports a public upload_plan from evalrdl.sdk.api. I reproduced the problem with it: a directory holding a_rollouts.jsonl plus other/logs/x.json is planned as one upload of other/ only, so the worker would report succeeded without uploading the Gym run. Suggested fix, fine as a separate PR:
- in the worker, before
upload():plans = upload_plan(run_dir), and fail unlesslen(plans) == 1 and plans[0]["dir"] == run_dir; - default
requirementtoevalrdl>=0.0.241.
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/40689158747 |
Summary
gym eval export evalrdl +rollouts_jsonl_fpath=... +label= +tags= +meta.k=v +parallel= +wait= +timeout= +url= +requirement=uploads one finished run to EvalRDL.EvalRDLExporteris anIsolatedExporter(feat(eval): add IsolatedExporter to run an exporter's backend work in its own venv #4915), so theevalrdlSDK lives in its own venv.Client().uploads.upload(...):committedsucceededdedupedskipped(nothing new stored)accepted(no verdict before the timeout)failed, exit 3; with+wait=falseit issucceededand the upload token is in the resultIntakeRefusedfailed, exit 4, with stage and reasonfailed, exit 1$EVALRDL_TOKEN,$GITLAB_TOKENor$GITLAB_PRIVATE_TOKEN, thenglab(usually missing on Slurm, so set a variable); there is no login cache. No package index is hard-coded: use uv's settings (such asUV_EXTRA_INDEX_URL) with the service's own<service URL>/v1/client/simple/index, sinceevalrdlis not on PyPI.requirement=pins the version. The environment is built once and the SDK is not upgraded; aClientTooOldfailure names the venv to delete or+requirement=evalrdl==VERSION.+timeoutplus an hour for the upload itself (just the hour with+wait=false). The run directory is taken with.absolute(), so a symlinked rollouts file does not move the upload to its target's directory, and an export is refused when the directory holds anotherrollouts.jsonlor*_rollouts.jsonl, because EvalRDL reads a directory as one run. A stored run that reportsneeds_bindinglogs a line.unexportable_reason. (Thesys.pathfix, which stopsevalrdl.pyshadowing the SDK, is in feat(eval): add IsolatedExporter to run an exporter's backend work in its own venv #4915.)--with_evalrdl_sdk, like--with_downloads. CLI reference 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 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
evalrdlpackage with the SDK's names and signatures: config parsing, the run directory being uploaded as it is, every setting reaching the SDK, each state and exit code, refusal and accepted messages, run detail,wait=false,needs_binding,ClientTooOldand a missing token, a symlinked rollouts file, a directory holding another run's rollouts, and the worker time limit. 32 tests pass (1 opt-in skipped), and each of the new ones fails without its fix.pre-commit: passed.tags=[a,b] meta.k=v ...: all arguments parsed and the exporter reached the venv build, where it reported a cleanfailed(no SDK index here).test_the_real_sdk_still_has_the_interface_the_worker_calls(pytest --with_evalrdl_sdk) checksupload()still takes the arguments the worker passes. It has NOT been run against the real SDK, only to confirm the flag works.Manual test plan
End-to-end check of the
atif,mlflowandevalrdlexporters on one small run. Run it from the top of this stack. I have not run the prepare, run or real-model export steps myself, because they need network access to HuggingFace and a model key (and, for EvalRDL, the SDK and its package index); the exporters themselves are covered by the tests above.aime24is used because it is small and needs no judge model (should_use_judge: false). It exercises the MLflow and EvalRDL exporters; ATIF needs a different input, see its subsection.Prepare dataset
For a benchmark with a
prepare_script, end-to-endgym eval rundoes not prepare the data, so this step is required. It needs network access.Run
One command starts the servers, collects the rollouts and shuts the servers down.
Exporter: ATIF
ATIF export is strict: it rejects any rollout whose
ng_trajectoryrecords a coverage gap.simple_agent(used byaime24) does not publish semantic turns, so its rollouts carry aturns_unavailablegap and are rejected by design:failed, exit 1, nothing written. A real run converts only when its agent publishes turns (for examplescicode_agent,verifiers_agentorterminus_2_sandboxed_agent) and rollout evidence is enabled (+observability_enabled=true +model_call_capture_dir=<dir>ongym eval run).Model-free check with a canonical sample run, from the Gym checkout (uses
.venvbecause the helper imports pytest):succeeded,trajectory_count: 3and amanifest.jsonl.Output path already exists(exit 1).gym eval export atif +rollouts_jsonl_fpath=$R ...on theaime24run fails withsource coverage gaps are not supported by strict exporton every row.Exporter: MLflow
Use a tracking server, not MLflow's local file store: the file store turns each
/in a metric name into a folder, so it cannot hold both<agent>/<metric>/no_answerand<agent>/<metric>/no_answer/std_dev_across_runs, whichaime24produces. A local server runs in its own throwaway environment; the first start downloads MLflow and takes about a minute. For a shared server, use its URL asmlflow_tracking_uriand putmlflow_tracking_tokenin env.yaml.succeededand a run id. In the UI (http://127.0.0.1:5055):<agent>/mean/rewardandkey_metrics/...metrics, params from the saved config, and theglobal_config.jsonandrollouts.jsonartifacts.skipped(already exported) and creates no new run. Delete<stem>_mlflow_export_doneto export again; add+upload_rollouts=falsefor a run withoutrollouts.json.gym eval runwith+mlflow_tracking_uri=... +mlflow_experiment_name=... +mlflow_run_name=...into the same output, then export. Expect a second run, a warning that names the live run id, andlive_run_idin the result (the export is not skipped)._aggregate_metrics.jsonaway and export, expectingfailed: Aggregate metrics not foundand no marker file. A run without_resolved_config.yamlfails withSaved run config not found.Exporter: EvalRDL
The package index is the EvalRDL service's own (
<service URL>/v1/client/simple/), set through your uv settings. The token comes from$EVALRDL_TOKEN,$GITLAB_TOKEN,$GITLAB_PRIVATE_TOKENorglab;evalrdl loginorevalrdl doctorverify it.succeededwith a run id and upload token; the first run builds the exporter environment.skipped(deduped) with exit 0.+wait=false, expectsucceededand an upload token without waiting for a verdict.*rollouts.jsonlin the run's directory fails with a message saying to keep each run in a directory of its own. With no token set (and noglab), expect a failed result that names the three token variables.pytest tests/unit_tests/test_evalrdl_exporter.py -k real_sdk --with_evalrdl_sdk.Known gaps
succeeded. Detecting that needsupload_plan(not in the released SDK) or a private SDK function, so it is not checked.Rollout evidence
N/A: no agent, verifier or model behavior is touched.