fix(opensandbox): let a caller launch the cleanup job from anywhere - #2791
Merged
Conversation
The launcher locates the run-scoped cleanup job's script under the working directory, which assumes it was invoked from a Gym checkout. A caller that launches from a scheduler run directory has no such tree there, so the submission fails and no cleanup job is scheduled. NEMO_GYM_REPO_ROOT names the checkout and joins the other declared inputs at the top of the launcher. The connection comes from OPENSANDBOX_DOMAIN and OPENSANDBOX_API_KEY, the same two variables the run resolves opensandbox.yaml from, so the launcher no longer reads an env.yaml or forwards --connection-config; the flag stays for a manual run. A failed cleanup submission no longer strands the main job: it printed no job ID and exited 1, so a caller read a live 4-node job as a failed submission and resubmitted it. Signed-off-by: Piotr Laszkiewicz <plaszkiewicz@nvidia.com> Co-authored-by: Grzegorz Chlebus <gchlebus@nvidia.com>
Contributor
Author
|
/ok to test b0d6431 |
Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
… are read" This reverts commit 3a1726c. Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
Contributor
Author
|
/ok to test 6dbd525 |
A caller that exports SBATCH_GPUS_PER_NODE for the main job has the cleanup job inherit it, so a one-CPU job asks four GPUs of a partition that has none and sbatch refuses it. --gres=none does not override that variable. Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
Contributor
Author
|
/ok to test a9dc9a0 |
Review: no environment variables in Gym. The cleanup script now takes --domain, --api-key and --protocol; --connection-config stays as the alternative and the two cannot be combined. The environment fallback is gone. The launcher no longer reads NEMO_GYM_REPO_ROOT either: it resolves the checkout from its own location, so a caller launching from a run directory needs to set nothing. Its own $@ is forwarded to gym eval, so the checkout could not become a flag on the launcher. Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
Contributor
Author
|
/ok to test 29450b8 |
bxyu-nvidia
requested changes
Aug 26, 2026
Review follow-up. OPENSANDBOX_DOMAIN present means the caller exports the connection, so it goes to the cleanup script as --domain/--api-key/--protocol; otherwise the checkout's own env.yaml is read, as it was before. The branch is built as an array, so neither form depends on word splitting. Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
A caller that runs a copy of this script cannot resolve the tree it came from, so it names it. Consumed before the rest is handed to gym eval, and it still defaults to the script's own tree, so a launch from a checkout is unchanged. Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
bxyu-nvidia
approved these changes
Aug 26, 2026
Contributor
|
/ok to test b1f741b |
Contributor
Author
|
/ok to test 45c8547 |
1 similar comment
Contributor
Author
|
/ok to test 45c8547 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
benchmarks/nemotron_3.5_super/sbatch_external_vllm.shlocates the run-scoped cleanup job's script undersubmit_dir=$(pwd -P), which assumes the launcher is invoked from a Gym checkout. A caller that launches from a scheduler run directory has no such tree there, so the submission fails outright:No cleanup job is scheduled and sandboxes are never reaped. This is how NVIDIA's eval-factory benchmarking runner drives the launcher: it stages a copy of the script into a per-run directory and
cds there, because that is whereslurm-logs/andresults/live.Changes
NEMO_GYM_REPO_ROOTnames the checkout, alongside the launcher's other declared inputs at the top of the file. It defaults to the working directory, so a launch from a checkout is unchanged.OPENSANDBOX_DOMAINandOPENSANDBOX_API_KEY— the same two variables the run itself resolvesopensandbox.yamlfrom. The launcher no longer looks for anenv.yamland no longer forwards--connection-config; the flag stays on the script for a manual run.Submitted batch job <id>, warns that its sandboxes need reaping by hand, and exits 0.Dropping the
env.yamlroute is deliberate: that file is merged over every config a run names, so a connection read from it can silently override the evaluated config, and no hash covers it.Testing
pytest tests/unit_tests/test_opensandbox_cleanup.py— 40 passed, 3 skipped.ruff checkandruff format --checkclean on both changed files.Supersedes
Replaces #2788, rebased onto current
mainand squashed. Same change, opened from a branch whose commits carry a sign-off.