feat(orchestration): return and persist a record of every submission by wprazuch · Pull Request #3188 · NVIDIA-NeMo/Gym

wprazuch · 2026-09-09T13:05:51Z

Problem

gym eval submit submits and forgets. SlurmExecutor.run computes its run directory internally, stages one directory per driver.benchmarks entry, sbatches each, prints the job ids, and returns None. Nothing is persisted anywhere, and the run directory is never surfaced — so a caller that submits a suite has no way to find those runs again.

It also pairs benchmarks to job ids positionally (zip(benchmark_names, job_ids)). Commands are piped to bash with no set -e, so a sbatch that fails part-way down the list contributes no output line and does not raise — and every benchmark after it is attributed its neighbour's job id.

What this does

submit() now returns a SubmissionRecord, persisted as a gym-job.json manifest in the remote run directory and in a local index under the user's cache dir, and emitted by gym eval submit --json.

nemo_gym/orchestration/jobs.py (new) — the output schema and both stores. Imports nothing from executors/, so a future gym eval status can read a record without pulling in Slurm and SSH.
Per-benchmark sbatch results. Each command reports its own benchmark, exit status and output on a marker line, so a failure lands on the benchmark that caused it. sbatch --parsable replaces the output regex. A benchmark whose submission failed is recorded with job_id: null and its error; its neighbours are unaffected, and the manifest still covers everything that queued.
Both connections now mean the same thing. LocalConnection.run piped through shlex.split, so it could only run a single simple command; it now pipes to bash -s as SSHConnection already did. Connection.write_text is added so a manifest can be written over either transport.
Run-directory identity. The directory was named gym-job-<timestamp> to the second, and the copy rsyncs with --delete, so two submits in the same second silently clobbered each other. The name now carries a random suffix.
CLI. --json emits the record alone; the human output moves out of the executor into the CLI; a benchmark that failed to submit exits non-zero.

Notes

submit()'s experimental warning moves to stderr — on stdout it made --json unparseable.
The remote manifest and the local index are byte-identical; there is a test that fails if they drift.
gym eval status is the motivating follow-up and reads the same record. It is deliberately not in this PR.

Testing

tests/unit_tests/{test_orchestration_jobs,test_slurm_executor,test_connection,test_cli_eval_submit}.py — 74 tests. Coverage includes a sbatch failing mid-list (the regression test for the positional pairing), a successful sbatch that also emits a warning, a silent success, two submits in the same second, and a real bash byte-comparison of the two stores. Several tests drive a real LocalConnection against a fake sbatch on PATH rather than mocking the transport.

Full suite matches the pre-existing baseline on this machine.

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

…by position Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

… a local record even when the manifest write fails Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

Executors no longer print; `gym eval submit` now owns output. Add a plain `--json` flag (not the shared Hydra JSON flag, since this command composes its config through Hydra `compose()` rather than the global config dict) that emits the SubmissionRecord and nothing else, and human-readable rich output otherwise. Exit 1 when any benchmark failed to submit, after emitting output either way. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

…ts consumers `gym eval submit --json` promised a stream that parses. It did not. - The `@experimental` decorator printed its warning with `rich.print`, which writes to stdout, so every `--json` run emitted a warning line and then the record. A caller doing `json.loads(stdout)` got a JSONDecodeError and no record at all. The warning is a diagnostic and now goes to stderr, where redirecting the payload leaves it alone. `submit` is the only user of the decorator. - `SSHConnection.write_text` wrapped a payload that already ends in a newline in a heredoc, which supplies its own, so the remote manifest carried a trailing blank line the local index did not. The two stores are meant to be byte-identical; the SSH path is the only one that runs in production, since `compute.hostname` is an FQDN and never equals `socket.gethostname()`. - The human report rendered sbatch's error text as rich markup. An error containing square brackets was silently eaten or raised MarkupError. - `_validate_mounts` still special-cased `LocalConnection` with a comment claiming only SSH pipes commands to bash. Both connections have piped to bash since 40f300fd2, so the branch bought nothing and kept the local test path away from the shell program that actually runs. Tests: the existing CLI tests monkeypatch `submit_module.submit`, replacing the very function the decorator wraps -- which is why a warning on stdout was invisible to them. New tests drive `main()` with argv and a fake executor and parse the whole of stdout. The heredoc test now pipes the generated script through a real bash and compares the resulting file against `dumps()`; it writes to a path containing a space, so dropping `shlex.quote` fails it too. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

…gement

copy-pr-bot · 2026-09-10T13:16:53Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

prokotg

The structure looks okay. We might revisit some decisions on how records are used for retrieval but I think that could be realized in the MR that touches on loading data

…tory A gym-native run completed on HSG, exited 0, printed "NeMo Gym finished!" -- and produced nothing at all. No rollouts, no aggregate metrics, not even the preprocessed dataset. The driver writes everything relative to the job directory: build_sbatch_script hardcodes `+output_jsonl_fpath=artifacts/rollouts.jsonl`. But `#SBATCH --chdir` sets the cwd of the BATCH script on the host, and the driver runs under `srun --container-image`, where the cwd is whatever the image declares and the job directory is not visible at all unless it is mounted. Neither was true, so every artifact landed in the container's ephemeral overlay and was discarded on exit. Logs were unaffected, which is what makes this so easy to miss: srun resolves `--output=logs/driver.log` on the host, so a run that saved nothing still leaves a complete, healthy-looking log behind and reports success. The driver srun now mounts the job directory at its own path and sets --container-workdir to it. The comment on --chdir said relative paths "resolve correctly inside the job", which was true for the batch script and false inside the container; it now says which is which. Two existing tests encoded the old behaviour. The no-mounts-by-default test asserted the driver had no --container-mounts, which is precisely the defect, so it now checks that *services* have none and points at the new coverage. The env-ordering test pinned an exact srun flag layout while only caring that the env prefix precedes srun, so it anchors on the container image instead. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

…tory `gym eval submit` with driver.gym_install cloned Gym into the driver's working directory and then `cd`-ed into the clone for the rest of the step. Both halves break a real run. Cloning into the working directory puts a second copy of every built-in asset under the job directory, and Gym resolves a named asset against the cwd as well as the install root. The driver passes `--model-type openai_model` whenever driver.policy_model is set, so the run aborts before it starts: `--model-type openai_model` is ambiguous: it matches multiple configs (/opt/Gym/responses_api_models/openai_model/configs/openai_model.yaml, <job_dir>/gym/responses_api_models/openai_model/configs/openai_model.yaml) Staying inside the clone is worse because it is silent: the driver writes `+output_jsonl_fpath=artifacts/rollouts.jsonl`, a relative path, so every artifact would land under the clone instead of the job directory and be lost with it -- the same disappearance the container workdir was added to prevent, reintroduced by a `cd`. The clone now goes to a mktemp directory under /tmp and is installed by path, so no `cd` is needed at all. This is what NEL's install_on_the_fly already does. Two existing tests pinned the old command strings (`git checkout main`, `uv pip install -e . --system`) while only caring that the ref is checked out and the install happens; they assert that instead. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

Revises the previous fix. Mounting the job directory was right; overriding --container-workdir to it was not, and only a real run showed why. A benchmark's own paths -- prepare_script, jsonl_fpath -- are relative and resolved against the process cwd, NOT through Gym's install-root search that config_paths uses. So with cwd moved to the job directory, `gym eval prepare` reported The following benchmarks are missing a valid prepare script: - gpqa: benchmarks/gpqa/prepare.py for a file that was plainly present in the install, while `gym eval run` still resolved the benchmark config fine. Two resolution rules inside one command, and moving cwd satisfies only one of them. The prepare failure was also quiet: it exits through exit_cleanly_on_config_error, so there is no traceback, and the run failed later on a missing dataset that looked like a different problem. So cwd stays where the image puts it, and the driver's output path is made absolute under the job directory instead. That keeps every artifact out of the container's ephemeral overlay -- the defect the previous commit fixed -- without disturbing anything that resolves relative to cwd. The job-directory mount is retained, since an absolute path still needs somewhere real to land. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

Completes the pair with the previous two commits; the three only work together. A benchmark's prepare_script and jsonl_fpath are relative and resolved against cwd. A gym_runtime image bakes no Gym at all, so unless the driver runs from the clone there is no directory where they resolve, and `gym eval prepare` fails with "missing a valid prepare script" for a file that is present -- quietly, through exit_cleanly_on_config_error, leaving the run to die later on a missing dataset. The `cd` was there originally. It was removed two commits ago because, with the clone landing in the job directory and the output path relative, it redirected every artifact into the clone. Both of those causes are now gone: the clone is in /tmp, and the output path is absolute. With cwd and the install root the same directory, named assets also resolve once instead of ambiguously, which is what the in-job-directory clone broke. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

…efaults head_server defaults were all-or-nothing: supplying any part of the mapping suppressed the rest. A config that pins only the port -- to keep the head server inside an allocated range, which is what a scheduler-managed run needs -- lost the host default too, and then had to supply a host it cannot know, since it is the address of whichever node the job lands on. That is resolvable, and already resolved a few lines above: use_absolute_ip turns it into gethostbyname(gethostname()), evaluated where the job runs. The only reason a caller had to pass a host was that setting the port hid it. Filling the mapping key by key lets a caller pin what it cares about and inherit the rest. Callers that set both are unaffected, and one that sets neither still gets both. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

render_driver_entrypoint embeds the install/prepare preamble inside a single-quoted `bash -c '...'`. POSIX shells do not nest single quotes: an inner quote ends the outer string rather than nesting in it, so every prepare argument shlex.quote() had to quote -- which is every value containing a space -- broke out of the string and word-split. A real submission died on this. gdpval's prepare passes +multistage.stages=[{num_tasks: 45, partial_completion: {...}}, ...] and the shell handed Hydra only `+multistage.stages=[{num_tasks:`, which it rejected with "no viable alternative at input '[{num_tasks:'". The run command itself was unaffected because it is built as a bash array outside the quoted body; only prepare goes inside it. Escape embedded quotes with the usual end-quote / literal-quote / reopen-quote sequence. The regression test runs the rendered script through bash and checks the argument arrives as one word -- asserting on the rendered string would pass either way. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

`--model-type openai_model` was hardcoded whenever driver.policy_model is set, which turns out to be the wrong default for how these benchmarks are actually run, and unconditionally wrong for some. Gym permits exactly one entry under `policy_model.responses_api_models`. A benchmark that ships its own policy model server -- lmarena_v3 declares a vllm_model with uses_reasoning_parser, chat_template_kwargs and friends -- then gets a second one composed on top and fails validation before any server starts: "Dictionary should have at most 1 item after validation, not 2". Overrides keyed on `policy_model.responses_api_models.vllm_model.*` fare no better: they materialise a bare vllm_model entry that openai_model.yaml never filled in, and validation reports "vllm_model -> entrypoint: Field required". It is also not the type these benchmarks are certified on. Every certified run of gpqa, hle, mmlu-prox, scicode, aa-lcr, aa-omniscience, wmt24pp, gdpval and critpt serves the policy as vllm_model, and the two differ in ways that reach the model: return_token_id_information, uses_reasoning_parser, sequential_reasoning_allowed, chat_template_kwargs.enable_thinking. So expose it as driver.policy_model_type. The default stays openai_model, and "" composes no policy model config at all, for a benchmark whose own config already declares a complete one. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

… bash -lc block The quote-safety fix for the driver entrypoint covered one of three places that embed a rendered command inside a single-quoted `bash -c '...'`. The two multi-node service renderers still interpolated raw. That is worse for services than for the driver, because a vLLM command routinely carries single-quoted JSON -- `--hf-overrides '{"architectures":[...]}'`, `--limit-mm-per-prompt`, `--media-io-kwargs`. Each of those ends the block early and the rest of the invocation word-splits. A real mmlu-prox submission, the first to use number_of_instances > 1, died on it before the engine started: /usr/bin/env: Argument list too long policy died during startup. Factor the escape into escape_for_single_quoted_block() and apply it at all three sites: the driver entrypoint, the data-parallel head/worker branches, and the Ray symmetric-run wrapper (whose inner_cmd carries the same JSON flags). Only interpolated values are escaped -- the templates' own quoting is what the fix exists to preserve, and `$VAR` / `$(( ))` are untouched because the inner shell is the one meant to expand them. The regression test renders the data-parallel block, swaps `vllm serve` for a printf, and runs it through bash, asserting the JSON arrives as a single argument. Asserting on the rendered string would pass either way. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

…ands vLLM refuses `--api-server-count` together with `--headless` and exits before loading anything: ValueError: --api-server-count=1 cannot be used with --headless (no API servers are started in headless mode). Gym is what decides which nodes run headless, so Gym has to keep their command valid. The flag is legitimate on the head node and reaches the builder through the service's own extra_args, so it is stripped from the worker branch rather than rejected outright. Found on the first real multi-node data-parallel submission: all five mmlu-prox workers died on it in 75 seconds. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

Three files conflicted; each was combined rather than resolved to one side. render_driver_entrypoint: main replaced `uv pip install --system` with a real venv, because --system targets whatever interpreter is on the container's PATH and fails outright when that is older than nemo-gym's requires-python. This branch had moved the clone out of the job directory, so a Gym checkout and its .venv no longer land in every benchmark's rundir on lustre. Both are kept: an out-of-tree clone in /tmp, then `uv venv --seed .venv` inside it. main's `set -euo pipefail` is kept too, with this branch's single-quote escaping applied after it so the whole preamble is escaped. build_sbatch_script: main added _with_default_capture_dir, which is kept. The relative output path it came with is not -- the driver `cd`s into the checkout so a benchmark's own relative prepare_script resolves, which means a relative output path writes every artifact inside that checkout and the run exits 0 with nothing in the job directory. This branch's absolute path and configurable policy_model_type are kept. test_slurm_script: both sides' new tests are kept. The gym_install assertions now describe the combined implementation -- main's venv lines, and `checkout main` rather than `git checkout main`, since the clone is addressed with `git -C`. Two pre-existing failures in test_opensandbox_cleanup and one in test_enroot_provider reproduce on clean main and are unrelated to this merge. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

…t, not just intent The module docstring claimed `jobs.py` was executor-agnostic, but nothing enforced it and five things contradicted it: * `executor: Literal["slurm"]` -- a k8s or local executor could not write a record without editing this schema. Now a plain `str`, so adding an executor does not force a schema bump on every reader. The field's job is to tell a reader what to branch on; closing it over today's executors defeats that. * `BenchmarkJob.job_id` was documented as None "when its `sbatch` failed". `sbatch` is Slurm's word; the rule is that the executor failed to enqueue it. * `SubmissionRecord.hostname` was documented as "the submission ran on the login node itself". Executors with no remote-submission concept have no login node. * `new_gym_job_id` explained its random suffix in terms of `SSHConnection.copy` and rsync. The reason is general -- an executor that stages by mirroring deletes what it does not recognise -- so it is stated generally, with Slurm as the example. * `MANIFEST_NAME` described "the remote run directory"; the directory need not be remote. `test_jobs_module_is_executor_agnostic` now enforces the import half in a subprocess -- this test module imports executors itself, so checking the already-loaded sys.modules in-process would pass regardless. Verified it fails on a real violation and names the leaked modules. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

prokotg

Thank you so much! There are couple of items I would like to discuss before merging

prokotg · 2026-09-14T10:52:40Z

-            rich.print(f"[green]submitted[/green] {name} → Slurm job [bold]{job_id}[/bold]")
-        for name in benchmark_names[len(job_ids) :]:
-            rich.print(f"[green]submitted[/green] {name} (job ID unavailable)")
+                record = self._build_record(cluster, compute, gym_job_id, now, remote_run_dir, benchmark_names, output)


Does it make sense to split run() into submit() and persist()? That would touch on changing Executor API but it feels like persist() could be move to the abstract class?

added in 690b429, but maybe run and persist? This way we do not break API

ah I see.. I hoped the parent class would call something like:

def run(): record = submit() persist(record) return record # optional

so that each executor is guaranteed to have a persistent state ( we now have to manually put persist inside run(). I understand this is to optimize connection latency. If there's no way to abstract this away - my comment is not blocking

prokotg · 2026-09-14T10:57:19Z

+    # after validation, not 2", and overrides keyed on `vllm_model.*` land on a
+    # server that was never composed. Set to "" to compose no policy model config
+    # at all, for a benchmark whose own config already declares a complete one.
+    policy_model_type: str = "openai_model"


Let's discuss

@prokotg okay, I re-checked and this is important:

Gym needs to know how to talk to the model server. It has a few interchangeable adapters, each a small YAML file:

responses_api_models/openai_model/configs/openai_model.yaml → declares policy_model.responses_api_models.openai_model
responses_api_models/vllm_model/configs/vllm_model.yaml → declares policy_model.responses_api_models.vllm_model

gym eval run --model-type X means "load adapter X".

Gym allows exactly one adapter. Literally:

# config_types.py:637 responses_api_models: Dict[str, ...] = Field(min_length=1, max_length=1)

Before my change, gym-native always passed the same one:

extra_flags = ["--model-type openai_model"] if config.driver.policy_model else []

Two things went wrong:
(a) Wrong adapter. Every certified run used vllm_model. We were using openai_model. Same model, different adapter — and the adapters differ in things the model actually sees, like whether reasoning tokens are fed back between turns. So our scores weren't comparing like with like.
(b) Collision. benchmarks/lmarena_v3/config.yaml already declares its own vllm_model adapter.

After: it's a setting.

I see. Then there are 2 questions:

whether we want to promote policy_model_type to a root-level field. The policy_model is there because we want to connect appropriate service to the driver. From the perspective of orchestration policy_model_type would be a syntactic sugar for adding responses_api_models/vllm_model/configs/vllm_model.yaml to the config_paths of the run section in gym eval submit's config.

Should benchmarks even declare responses_api_models and what does it mean? does lmarena_v3 does not support sglang or something different? cc @marta-sd

Review feedback, all of it justified. Comment volume, flagged in three separate places. The `decorators.py` block is removed outright as asked; `global_config.py`'s head-server note goes from seven lines to one; `script_templates.py` drops from 27% comment lines to 14%. The `script_templates.py` trim turned up a real defect rather than just noise: a thirteen-line block still said "Clone outside the working directory, and never `cd` into it" while the code three lines below it does exactly that. The merge from main had left both the old comment and its replacement, so the file carried two explanations of the same decision and one of them was wrong. `SubmissionRecord.config_path` is removed: it was declared and nothing anywhere assigned it, which is why it was never set in a reviewer's runs. `gym_version()` goes too -- a one-line wrapper over `nemo_gym.__version__`, called once, while comparison/runner.py already reads `__version__` directly. Remaining review items (schema-version compatibility on upgrade, executor_metadata, moving _utc_now out of the Slurm executor, record methods on SubmissionRecord, splitting run() into submit()/persist()) are behaviour or API changes and follow separately. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

`load_record` required `schema_version` to equal SCHEMA_VERSION exactly, so a NEWER Gym could not read an OLDER record. Upgrading nemo-gym therefore stranded every job already on disk -- the opposite of what the check was for -- and the error told the user to "Upgrade nemo-gym to read this record", which in that case is the one thing that cannot help. Read the version in one direction instead: accept anything at or below what this Gym understands, refuse only what is newer. A newer record is the single case that genuinely cannot be read safely, since it may carry fields whose meaning this version does not know. That is only sound if the schema keeps its side of the bargain, so the module docstring now states it: within a schema version a field may only be added with a default, or removed. Repurposing a field or changing its meaning needs a version bump and an explicit migration, because an older record would otherwise be read as if it meant the new thing. A payload with no integer schema_version is rejected separately -- that is not an old record, it is not a record at all. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

…ta escape hatch Three review points, all about where behaviour belongs. `dumps`, `write_local_index` and `load_record` were module functions taking a record as their first argument; they are now `SubmissionRecord.dumps()`, `.write_local_index()` and the `SubmissionRecord.load()` classmethod. The import-boundary test still passes, so this costs nothing in executor-agnosticism and the schema now owns how it is written and read. `_utc_now` moves out of the Slurm executor into jobs.py as `utc_now`. It is the clock a run directory is named from, which is this module's concern and the same answer every executor needs -- nothing about it was Slurm's. `executor_metadata: dict[str, str]` is new. The record cannot grow a field per executor, so anything an executor needs that the shared fields cannot express -- a k8s namespace was the example raised -- goes here, keyed off `executor`. Empty for Slurm, which needs nothing beyond what the record already carries. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

Review suggestion: split run() so persistence can move to the abstract class. `BaseExecutor.persist()` is now concrete and every executor gets the same behaviour instead of reimplementing it. What moved is the policy, not the transport. The local index is written before the manifest precisely because it cannot fail the submit, so a failed manifest write still leaves a parseable record for the by-hand recovery its error asks for; and that error has to name the already-queued jobs, since by then they are running. Both of those are identical for any executor, and both now live in one place. The transport could not move with it. Slurm's manifest goes over the connection opened for the submit, and a k8s executor would use something else entirely, so `persist()` takes the writer as an argument rather than owning a connection it cannot know how to open -- `Connection.write_text` already matches the signature. That also keeps it callable while the connection is still open; reopening one inside persist() would add a second connection to every submit, which matters when a submission carries twenty benchmarks. run() therefore stays the entry point rather than becoming submit() + persist() in sequence: the manifest write has to happen inside the transport's scope, and a separate persist() phase after submit() returns would either reopen the connection or lose the index-first ordering. Two tests cover the behaviour the docstring claims, against a stub executor rather than through Slurm: the index survives a failing manifest write, and the resulting error names the queued jobs. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

prokotg

Thank you!

prokotg · 2026-09-14T16:29:07Z

-            rich.print(f"[green]submitted[/green] {name} → Slurm job [bold]{job_id}[/bold]")
-        for name in benchmark_names[len(job_ids) :]:
-            rich.print(f"[green]submitted[/green] {name} (job ID unavailable)")
+                record = self._build_record(cluster, compute, gym_job_id, now, remote_run_dir, benchmark_names, output)


ah I see.. I hoped the parent class would call something like:

def run(): record = submit() persist(record) return record # optional

so that each executor is guaranteed to have a persistent state ( we now have to manually put persist inside run(). I understand this is to optimize connection latency. If there's no way to abstract this away - my comment is not blocking

One conflict, in render_driver_entrypoint. main had factored the clone into render_repo_checkout() and added something worth keeping: it installs git when the image lacks it, which a runtime container may well do. The two sides disagreed only on where the clone lands. main clones into the current directory, i.e. the job directory; this branch clones to /tmp, because a Gym checkout plus its .venv inside every benchmark's rundir is slow to write on lustre and noise among the artifacts. Nothing depends on the clone surviving -- the driver's output path is absolute -- so the destination is the one part worth keeping from this side. Resolved by giving render_repo_checkout an optional `dest`, emitted verbatim so it can be a shell expression, defaulting to main's behaviour. The helper has a single caller, so this changes nothing for anyone else. test_gym_install_runs_from_the_install_root asserted that `cd "$GYM_SRC/gym"` stood on its own line; it is now &&-chained onto the checkout. The behaviour is unchanged, so the test now asserts the behaviour -- that cwd moves into the clone before the run, and that nothing else moves it -- rather than the layout. The 13 unit-test failures on this merge are all in the e2b, enroot and opensandbox sandbox providers, and all 13 reproduce on clean main. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

wprazuch · 2026-09-15T09:10:53Z

/ok to test 1a2ade0

…block A wrong-typed `head_server` value (typically a `++head_server.port=<not an int>` override) was only validated when `ServerClient.load_head_server_config` built a `BaseServerConfig`, deep inside the command. `BaseServerConfig` is not a CLI config, so the router re-raised the pydantic error: `gym eval run` ended in a raw traceback, `gym env start` initialised Ray before failing, and `gym env validate` / `gym env resolve` accepted the value and exited 0. Validate the block against `BaseServerConfig` in `GlobalConfigDictParser.parse()` right after its defaults are filled, raising the new `HeadServerConfigMalformedError(ConfigError, ValueError)` with an actionable message, the same way the parser handles `config_paths`. Every affected command loads the config first and is wrapped in `exit_cleanly_on_config_error`, so all of them now print one `Error:` line and exit 1 before any server starts. The router's re-raise for non-CLI models is intentionally unchanged (`test_non_config_validation_error_is_reraised`); the dropped-host half of the issue was already fixed by NVIDIA-NeMo#3188. Closes NVIDIA-NeMo#2686 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Tsvetan (Sven) Rankov <tsvetan.rankov@gmail.com>

wprazuch added 8 commits September 9, 2026 15:04

feat(orchestration): add the submission record schema and its stores

dd28151

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

fix(orchestration): use dumps() in write_local_index for consistency

e720b32

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

feat(orchestration): give both connections the same shell semantics

942672f

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

fix(orchestration): identify sbatch results per benchmark instead of …

13cfe06

…by position Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

feat(orchestration): return and persist a record of every submission

fd9661f

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

fix(orchestration): validate job ids and benchmark names, and persist…

fc46dd4

… a local record even when the manifest write fails Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

copy-pr-bot Bot temporarily deployed to public September 9, 2026 13:06 Inactive

copy-pr-bot Bot temporarily deployed to public September 9, 2026 13:08 Inactive

yaoyu-33 added area:orchestration Job submission, deployment, services, and lifecycle management feature New capabilities, enhancements, or enablement work needs-review PR is ready for code review and waiting on a reviewer labels Sep 9, 2026

linear-code Bot mentioned this pull request Sep 10, 2026

Oncall queue report - 2026-09-15 PDT #3208

Closed

wprazuch requested a review from prokotg September 10, 2026 11:30

prokotg reviewed Sep 10, 2026

View reviewed changes

Comment thread nemo_gym/orchestration/jobs.py Outdated

prokotg reviewed Sep 10, 2026

View reviewed changes

Comment thread nemo_gym/orchestration/executors/base.py Outdated

Merge remote-tracking branch 'github/main' into wprazuch/gym-job-mana…

fe39800

…gement

prokotg reviewed Sep 10, 2026

View reviewed changes

Comment thread nemo_gym/orchestration/executors/slurm.py Outdated

prokotg previously approved these changes Sep 10, 2026

View reviewed changes

wprazuch dismissed prokotg’s stale review via 26962c9 September 10, 2026 17:17

wprazuch added 5 commits September 11, 2026 01:04

wprazuch added 2 commits September 11, 2026 14:26

style: ruff-format the new entrypoint quoting test

6a454f7

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>

prokotg linked an issue Sep 11, 2026 that may be closed by this pull request

[orchestration] Job management: database structure #3059

Closed

wprazuch added 4 commits September 14, 2026 09:45

prokotg requested changes Sep 14, 2026

View reviewed changes

wprazuch added 4 commits September 14, 2026 14:14

yaoyu-33 added the complexity:high Cross-component or shared-contract change with a large review surface label Sep 15, 2026

prokotg previously approved these changes Sep 15, 2026

View reviewed changes

wprazuch dismissed prokotg’s stale review via 1a2ade0 September 15, 2026 08:52

copy-pr-bot Bot deployed to public September 15, 2026 09:11 Active

copy-pr-bot Bot deployed to public September 15, 2026 09:13 Active

wprazuch enabled auto-merge September 15, 2026 09:54

prokotg approved these changes Sep 15, 2026

View reviewed changes

wprazuch added this pull request to the merge queue Sep 15, 2026

Merged via the queue into main with commit 256d9af Sep 15, 2026
39 checks passed

wprazuch deleted the wprazuch/gym-job-management branch September 15, 2026 11:26

sven-rankov mentioned this pull request Sep 20, 2026

fix(config): fail fast with a clean error on a malformed head_server block #3561

Open

7 tasks

Conversation

wprazuch commented Sep 9, 2026

Problem

What this does

Notes

Testing

Uh oh!

Uh oh!

Uh oh!

copy-pr-bot Bot commented Sep 10, 2026

Uh oh!

Uh oh!

prokotg left a comment

Choose a reason for hiding this comment

Uh oh!

prokotg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prokotg Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

wprazuch Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

prokotg Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

prokotg Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

wprazuch Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prokotg Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

prokotg left a comment

Choose a reason for hiding this comment

Uh oh!

prokotg Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

wprazuch commented Sep 15, 2026

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wprazuch Sep 14, 2026 •

edited

Loading