Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
dd28151
feat(orchestration): add the submission record schema and its stores
wprazuch Sep 9, 2026
e720b32
fix(orchestration): use dumps() in write_local_index for consistency
wprazuch Sep 9, 2026
942672f
feat(orchestration): give both connections the same shell semantics
wprazuch Sep 9, 2026
13cfe06
fix(orchestration): identify sbatch results per benchmark instead of …
wprazuch Sep 9, 2026
fd9661f
feat(orchestration): return and persist a record of every submission
wprazuch Sep 9, 2026
fc46dd4
fix(orchestration): validate job ids and benchmark names, and persist…
wprazuch Sep 9, 2026
2883ebf
feat(cli): report the submission record from gym eval submit
wprazuch Sep 9, 2026
265940b
fix(orchestration): make the submission record actually readable by i…
wprazuch Sep 9, 2026
fe39800
Merge remote-tracking branch 'github/main' into wprazuch/gym-job-mana…
wprazuch Sep 10, 2026
26962c9
fix(orchestration): give the driver a workdir and a mounted job direc…
wprazuch Sep 10, 2026
53c0725
fix(orchestration): install gym_install's clone outside the job direc…
wprazuch Sep 10, 2026
24007ad
fix(orchestration): keep artifacts by absolute path, not by moving cwd
wprazuch Sep 11, 2026
952ff1b
fix(orchestration): run the driver from the gym_install clone
wprazuch Sep 11, 2026
7f58ff2
fix(config): complete a partial head_server instead of skipping its d…
wprazuch Sep 11, 2026
5b35304
fix(orchestration): quote-safe driver entrypoint body
wprazuch Sep 11, 2026
6a454f7
style: ruff-format the new entrypoint quoting test
wprazuch Sep 11, 2026
bbbf908
feat(orchestration): make the driver's policy model type configurable
wprazuch Sep 11, 2026
c23f316
fix(orchestration): escape both multi-node service commands for their…
wprazuch Sep 14, 2026
f0fbf5a
fix(orchestration): drop --api-server-count from headless worker comm…
wprazuch Sep 14, 2026
f1ec6bd
Merge branch 'main' into wprazuch/gym-job-management
wprazuch Sep 14, 2026
e9b302a
refactor(orchestration): make the job record executor-agnostic in fac…
wprazuch Sep 14, 2026
c84d60c
refactor(orchestration): trim comments and drop two unused pieces
wprazuch Sep 14, 2026
8f4bbb6
fix(orchestration): keep older job records readable across an upgrade
wprazuch Sep 14, 2026
5f3debd
refactor(orchestration): give the record its own methods and a metada…
wprazuch Sep 14, 2026
690b429
refactor(orchestration): lift record persistence into BaseExecutor
wprazuch Sep 14, 2026
1a2ade0
Merge branch 'main' into wprazuch/gym-job-management
wprazuch Sep 15, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion nemo_gym/cli/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -567,9 +567,11 @@ def _reject_scratch_namespace_additions(overrides: list[str]) -> None:
def _eval_submit(args: argparse.Namespace, overrides: list[str]) -> None:
from pathlib import Path

import rich
from hydra import compose, initialize_config_dir
from hydra.core.global_hydra import GlobalHydra
from omegaconf import OmegaConf
from rich.markup import escape

from nemo_gym.orchestration.api import SubmitConfig
from nemo_gym.orchestration.submit import submit
Expand All @@ -585,7 +587,31 @@ def _eval_submit(args: argparse.Namespace, overrides: list[str]) -> None:
resolved = OmegaConf.to_container(composed, resolve=True)
scratch_keys = {key for key in resolved if key.startswith("_")}
config = SubmitConfig.model_validate({key: value for key, value in resolved.items() if key not in scratch_keys})
submit(config, dry_run=args.dry_run)

record = submit(config, dry_run=args.dry_run)
if record is None:
return

if args.json:
# The record alone, so the output parses.
print(record.model_dump_json(indent=2))
else:
rich.print(f"Run directory: [bold]{record.run_dir}[/bold]")
for benchmark in record.benchmarks:
if benchmark.job_id is None:
# sbatch's message is not ours to format: `escape` disables markup
# for it, so an error carrying square brackets is printed as
# written instead of being swallowed or raising MarkupError.
rich.print(f"[red]failed[/red] {benchmark.benchmark}: {escape(benchmark.error or '')}")
else:
rich.print(
f"[green]submitted[/green] {benchmark.benchmark} → Slurm job [bold]{benchmark.job_id}[/bold]"
)

if record.failed:
names = ", ".join(b.benchmark for b in record.failed)
print(f"Error: {len(record.failed)} benchmark(s) failed to submit: {names}", file=sys.stderr)
sys.exit(1)


def _eval_run(args: argparse.Namespace, overrides: list[str]) -> None:
Expand Down Expand Up @@ -1113,6 +1139,11 @@ def _dataset_download(args: argparse.Namespace, overrides: list[str]) -> None:
"--dry-run", action="store_true", help="Print generated job scripts without submitting."
),
),
Flag(
register=lambda p: p.add_argument(
"--json", action="store_true", help="Emit the submission record as JSON."
),
),
),
),
"eval compare": Command(
Expand Down
9 changes: 6 additions & 3 deletions nemo_gym/decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,18 @@

import functools

import rich
from rich.console import Console


_stderr = Console(stderr=True)


def experimental(fn):
"""Decorator that prints an experimental warning before the function runs."""
"""Decorator that warns on stderr that a function is experimental, then runs it."""

@functools.wraps(fn)
def wrapper(*args, **kwargs):
rich.print(
_stderr.print(
f"[yellow]Warning:[/yellow] [bold]{fn.__name__}[/bold] is experimental and may change or be removed without notice."
)
return fn(*args, **kwargs)
Expand Down
11 changes: 5 additions & 6 deletions nemo_gym/global_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -1297,12 +1297,11 @@ def parse(self, parse_config: Optional[GlobalConfigDictParserConfig] = None) ->
)

with open_dict(global_config_dict):
# Populate head server defaults
if not global_config_dict.get(HEAD_SERVER_KEY_NAME):
global_config_dict[HEAD_SERVER_KEY_NAME] = {
"host": default_host,
"port": DEFAULT_HEAD_SERVER_PORT,
}
# Head server defaults, filled per key so a config may pin just one.
head_server = global_config_dict.get(HEAD_SERVER_KEY_NAME) or {}
head_server.setdefault("host", default_host)
head_server.setdefault("port", DEFAULT_HEAD_SERVER_PORT)
global_config_dict[HEAD_SERVER_KEY_NAME] = head_server

# Store final list of disallowed ports.
global_config_dict[DISALLOWED_PORTS_KEY_NAME] = disallowed_ports
Expand Down
9 changes: 9 additions & 0 deletions nemo_gym/orchestration/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,15 @@ class DriverConfig(_StrictModel):
# Name of a service in `services:` to use as the policy model. When set, injects
# policy_base_url/policy_model_name/policy_api_key into each benchmark's run config.
policy_model: str | None = None
# Which responses_api_models asset serves as the policy, passed as
# `--model-type`. Not every benchmark wants the same one: Gym permits exactly
# one entry under `policy_model.responses_api_models`, so composing
# openai_model against a benchmark that ships its own vllm_model policy (e.g.
# lmarena_v3) fails validation with "Dictionary should have at most 1 item
# 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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss

@wprazuch wprazuch Sep 14, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Then there are 2 questions:

  1. 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.
  2. 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

benchmarks: dict[str, BenchmarkRunConfig]
env: dict[str, str] = {}
# Pyxis-style bind mounts passed as --container-mounts.
Expand Down
40 changes: 39 additions & 1 deletion nemo_gym/orchestration/executors/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,48 @@
# limitations under the License.

from abc import ABC, abstractmethod
from collections.abc import Callable
from pathlib import Path

from nemo_gym.orchestration.api import SubmitConfig
from nemo_gym.orchestration.jobs import MANIFEST_NAME, SubmissionRecord


class BaseExecutor(ABC):
@abstractmethod
def run(self, config: SubmitConfig, *, dry_run: bool = False) -> None: ...
def run(self, config: SubmitConfig, *, dry_run: bool = False) -> SubmissionRecord | None:
"""Submit `config` and return the record describing it.

Returns None on a dry run, which renders the scripts and stops before
anything is submitted; every other path either returns a record or
raises.
"""

def persist(self, record: SubmissionRecord, write_manifest: Callable[[Path, str], None]) -> None:
"""Store the record, in the order that survives a partial failure.

Shared by every executor because the ordering and the failure handling
are policy rather than transport. The machine-local index goes first
precisely because it cannot fail the submit, so if the manifest write
does fail there is still a parseable record for the by-hand recovery the
error asks for.

Only the manifest's transport differs per executor, so it arrives as
`write_manifest` -- `Connection.write_text` already has this signature --
rather than this class owning a connection it cannot know how to open.
Call it while that transport is still open: reopening one here would pay
a second connection on every submit.

The jobs are queued by the time this runs, so a failure has to name them
or they are stranded with no record anywhere.
"""
record.write_local_index()
manifest = Path(record.run_dir) / MANIFEST_NAME
try:
write_manifest(manifest, record.dumps())
except Exception as error:
queued = ", ".join(f"{b.benchmark}={b.job_id}" for b in record.benchmarks if b.job_id)
raise RuntimeError(
f"Submitted jobs but could not write the manifest to {manifest}: {error}. "
f"Already queued: {queued or 'nothing'}. Record these by hand before collecting."
) from error
27 changes: 26 additions & 1 deletion nemo_gym/orchestration/executors/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ def copy(self, local: Path, remote: Path) -> None: ...
@abstractmethod
def run(self, commands: list[str]) -> str: ...

@abstractmethod
def write_text(self, remote: Path, content: str) -> None: ...

def close(self) -> None:
pass

Expand All @@ -47,7 +50,14 @@ def copy(self, local: Path, remote: Path) -> None:
shutil.copytree(local, remote)

def run(self, commands: list[str]) -> str:
return "\n".join(_checked(shlex.split(cmd)) for cmd in commands)
# Piped to bash, not shlex.split into argv: callers send compound bash
# (`out=$(...); rc=$?; ...`), and this has to mean the same thing here
# as it does over SSH.
return _checked(["bash", "-s"], input="\n".join(commands), context="local commands")

def write_text(self, remote: Path, content: str) -> None:
remote.parent.mkdir(parents=True, exist_ok=True)
remote.write_text(content, encoding="utf-8")


class SSHConnection(Connection):
Expand Down Expand Up @@ -128,6 +138,21 @@ def run(self, commands: list[str]) -> str:
context=f"ssh commands on {self._hostname}",
)

def write_text(self, remote: Path, content: str) -> None:
# A quoted heredoc delimiter: the payload reaches the file byte for byte,
# with no parameter or command substitution applied to it on the way.
# The heredoc supplies the newline before the delimiter, so a payload
# that already ends in one (jobs.dumps does) must shed it -- otherwise
# the remote manifest gains a blank line the local index does not have,
# and the two stores stop being byte-identical.
payload = content.removesuffix("\n")
script = f"cat > {shlex.quote(str(remote))} <<'GYM_EOF'\n{payload}\nGYM_EOF\n"
_checked(
["ssh", *self._ssh_opts(), self._hostname, "bash", "-s"],
input=script,
context=f"writing {remote} on {self._hostname}",
)

def close(self) -> None:
subprocess.run(
["ssh", *self._ssh_opts(), "-O", "exit", self._hostname],
Expand Down
49 changes: 43 additions & 6 deletions nemo_gym/orchestration/executors/script_templates.py
Comment thread
prokotg marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,25 @@ def render_ray_prelude() -> str:
return _RAY_PRELUDE


def escape_for_single_quoted_block(body: str) -> str:
"""Make `body` safe to embed inside a single-quoted `bash -c '...'` block.

POSIX shells do not nest single quotes: an inner quote ENDS the outer string
rather than nesting in it. Every value that had to be quoted -- a Hydra
override containing a space, or any of the JSON blobs vLLM flags take
(`--hf-overrides '{"architectures":[...]}'`) -- would otherwise break out of
the block and word-split. Both failure modes have been observed on real
submissions: Hydra rejecting `+multistage.stages=[{num_tasks:` on its own,
and `/usr/bin/env: Argument list too long` from a multi-node vLLM command
whose JSON flags reopened the quoting.

`'"'"'` is the standard end-quote / literal-quote / reopen-quote sequence.
It leaves `$VAR` and `$(( ))` untouched, which matters: the inner shell is
the one meant to expand them.
"""
return body.replace("'", "'\"'\"'")


def render_vllm_ray_symmetric_run(inner_cmd: str, total_nodes: int, resource_flags: str) -> str:
"""Render the Ray head/worker bootstrap that wraps a single vLLM instance's TP/PP command so
it spans multiple Slurm nodes.
Expand All @@ -106,7 +125,13 @@ def render_vllm_ray_symmetric_run(inner_cmd: str, total_nodes: int, resource_fla
pin fall back to manually starting head/worker Ray processes, keyed on Slurm's per-node task
rank ($SLURM_NODEID).
"""
return _VLLM_RAY_SYMMETRIC_RUN.format(total_nodes=total_nodes, resource_flags=resource_flags, inner_cmd=inner_cmd)
# Only the interpolated values are escaped; the template's own structure is
# what the quoting is meant to preserve.
return _VLLM_RAY_SYMMETRIC_RUN.format(
total_nodes=total_nodes,
resource_flags=escape_for_single_quoted_block(resource_flags),
inner_cmd=escape_for_single_quoted_block(inner_cmd),
)


def render_health_check(name: str, port: int, path: str, timeout: int) -> str:
Expand All @@ -125,13 +150,18 @@ def render_gym_cmd(subcommand: str, var_name: str, args: list[str]) -> str:
return f"{var_name}=(\n " + "\n ".join(entries) + "\n)"


def render_repo_checkout(repo: str, ref: str) -> str:
"""Render an &&-chained command that installs git if missing, then clones and checks out `ref`."""
def render_repo_checkout(repo: str, ref: str, dest: str | None = None) -> str:
"""Render an &&-chained command that installs git if missing, then clones and checks out `ref`.

`dest` is emitted verbatim so it may be a shell expression (the driver passes
`"$GYM_SRC/gym"` to clone outside the job directory); omit it to clone into a
directory named after the repo in the current one.
"""
repo_name = repo.rstrip("/").split("/")[-1].removesuffix(".git")
target = dest if dest is not None else shlex.quote(repo_name)
ensure_git = "command -v git >/dev/null 2>&1 || (apt-get update -qq && apt-get install -y -qq git)"
return (
f"({ensure_git})"
f" && git clone {shlex.quote(repo)} && cd {shlex.quote(repo_name)} && git checkout {shlex.quote(ref)}"
f"({ensure_git}) && git clone {shlex.quote(repo)} {target} && cd {target} && git checkout {shlex.quote(ref)}"
)


Expand All @@ -154,10 +184,16 @@ def render_driver_entrypoint(
preamble: list[str] = []

if repo and ref:
# Clone to /tmp rather than the job directory: a checkout plus its .venv
# inside every benchmark's rundir is slow to write on lustre and noise in
# the artifacts. `cd` into it because a benchmark's prepare_script and
# jsonl_fpath resolve against cwd; the driver's output path is absolute,
# so nothing depends on the clone being reachable afterwards.
preamble += [
"curl -LsSf https://astral.sh/uv/install.sh | sh",
'source "$HOME/.local/bin/env"',
render_repo_checkout(repo, ref),
'GYM_SRC="$(mktemp -d /tmp/gym-install-XXXXXX)"',
render_repo_checkout(repo, ref, dest='"$GYM_SRC/gym"'),
# A real venv, not --system: --system targets whatever interpreter happens to be on
# the container's PATH, sidestepping uv's own project-aware Python selection - `uv
# venv` instead reads requires-python from pyproject.toml and auto-downloads a
Expand All @@ -176,4 +212,5 @@ def render_driver_entrypoint(

preamble.append('exec "$@"')
body = "\n ".join(["set -euo pipefail", *preamble])
body = escape_for_single_quoted_block(body)
return f"bash -c '\n {body}\n' -- \"${{GYM_CMD[@]}}\""
Loading
Loading