refactor(docker): read the actor venv list from a Python leaf module by terrykong · Pull Request #4002 · NVIDIA-NeMo/RL

terrykong · 2026-09-04T21:53:00Z

Draft, for discussion.

This branch is stacked on #3947, so the diff below includes all three of
@tdene's commits. The hardlink work, the three-layer prefetch and the TRT-LLM handling
are entirely his and the credit is his. My contribution is the seven commits on top.

@tdene originally had this list in a separate manifest file and said he preferred it
there. I asked for the move into pyproject.toml, and I now think he was closer to
right than I was — this is a third option rather than a vindication of either.

What this changes about #3947

#3947 has to make the actor → uv extras mapping readable from the dependency layer,
because that is where the worker venvs must be built (hardlinking them anywhere else
copies every wheel up into the final layer). It solves that by moving the mapping into a
[tool.nemo_rl.actor_environments] table in pyproject.toml, which the Dockerfile then
parses with an inline tomllib snippet duplicated in two RUN blocks.

This branch keeps the layering, the hardlinks and the performance exactly as they are, and
changes only where the list lives and who reads it.

The approach

A dependency-free leaf module, nemo_rl/distributed/actor_environments.py, holds the
mapping. It has two readers:

Runtime — ray_actor_environment_registry.py imports the dict and builds the
py_executable strings.
Build — the Dockerfile runs it as a script.

Running it as a script rather than importing it is the part that makes this work: Python
only executes nemo_rl/__init__.py when you import the package, and that file does real
work at import (fingerprint check, sys.path surgery, transformers patching) that cannot
run in the dependency layer.

# nemo_rl/distributed/actor_environments.py — stdlib only, no nemo_rl imports
ACTOR_ENVIRONMENTS: dict[str, list[str] | None] = {
    "nemo_rl.models.generation.vllm.vllm_worker.VllmGenerationWorker": ["vllm"],
    "nemo_rl.models.policy.workers.megatron_policy_worker.MegatronPolicyWorker": ["mcore"],
    "nemo_rl.environments.math_environment.MathEnvironment": None,   # driver interpreter
    ...
}

The honest case for it

Not "fewer lines" — the Dockerfile is 638 lines here against 640 in #3947, a difference of
two. What this actually buys:

	#3947	this branch
places the list is parsed	3 (two shell heredocs + Python)	1
list lives in	`pyproject.toml` (67 lines of app wiring)	a Python module, type-checked by pyrefly
registry at import	reads and parses `pyproject.toml`	no file I/O
a parse failure in the Dockerfile	silently prefetches nothing, build goes green	fails the build
tests	0	62

The set -e item is the one real correctness fix. In #3947 the loop is
done < <(list_actor_venvs), and a process substitution's exit status is invisible to
set -e — so a broken parse would prefetch nothing and still produce a green build. This
branch writes the list once with a plain redirect plus test -s, which turns that into a
build failure. Verified by repro.

Behavior changes, declared

Previously this section also claimed seven actors newly honour
NEMO_RL_PY_EXECUTABLES_SYSTEM=1. That went stale when this branch rebased onto #4020,
which added PY_EXECUTABLES._resolve_system_overrides() on main — every constant honours
the flag there now, so this branch preserves that rather than changing it. Checked by
rebuilding both actor → py_executable mappings on main and here, with the flag set and
unset: 28 actors, no differing value.

SKIP_*_BUILD now filters on declared extras, not on a substring of the actor name.
SKIP_VLLM_BUILD=1 previously missed AsyncTrajectoryCollector, ReplayBuffer and
SyncRolloutActor — all of which need the vLLM extra but have no "vllm" in their name.
Both prefetch passes now read the same list, so they cannot disagree.
The container fingerprint now hashes the actor table. Venvs are reused rather than
rebuilt and nothing prunes them, so a changed extras list has to invalidate the
fingerprint the way a dependency change does. While the table lived in pyproject.toml
this was covered by accident; moving it to a .py file dropped the coverage, so it is
restored explicitly. Containers built in the ~2-day window around this branch's base will
report a one-time mismatch on the new key;
python tools/generate_fingerprint.py > /opt/nemo_rl_container_fingerprint clears it.

Tests

tests/unit/distributed/test_actor_environments.py — 10 test functions, 62 cases, no GPU:

every extra exists in [project.optional-dependencies], and the registry rejects a
typo'd one at import
every actor FQN resolves to a module defining that class (parsed, not imported)
the generated py_executable matches the PY_EXECUTABLES.* constant
the script's output matches the runtime registry — including the stage and extra-flag
columns the Dockerfile actually consumes, not just the actor name
SKIP_VLLM_BUILD drops actors by extra, not by name
a mistyped stage or skip extra exits non-zero instead of printing an empty list
the container fingerprint's entry equals this file's md5
actor_environments.py imports only the standard library — the one real risk of a Python
module over a TOML table is that someone adds from nemo_rl... import ... later and
breaks the build in its most expensive layer

Every one of these was checked by mutation: the defect it names was introduced, the test
was confirmed to fail, and the mutation reverted.

What this trades away

A structural guarantee for a checked one. A TOML table cannot import anything; this
module only stays import-free as long as the test above keeps passing. That is still the
right trade — it buys type-checking, real unit tests, and one reader instead of three — but
it is a trade and not a free win.

Open questions

Should this fold into perf(docker): hardlink worker venvs into layers #3947 rather than landing separately, given perf(docker): hardlink worker venvs into layers #3947 is unblocking
pipelines and has no submitted reviews yet?

copy-pr-bot · 2026-09-04T21:53:05Z

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.

terrykong

This is a self-review of a draft PR. It is stacked on #3947 by @tdene and adds four commits on top of his three; only those four are in scope here.

Those four move the actor -> extras table out of pyproject.toml and into a stdlib-only Python leaf module, nemo_rl/distributed/actor_environments.py, which both the registry and the Dockerfile read. They also add the table to the container fingerprint and add 7 tests.

The PR description is wrong in four ways. Flagging it as a finding because the PR's stated justification is contradicted by its own diff:

It says "the two commits on top". There are four — c987117, 91adf33, f03ddf0, 762d50b — and only one is linked.
The size table says docker/Dockerfile is "-45 / +24 vs #3947". The real number is +58 / -52, and the file goes from 640 to 646 lines. It got larger. That inverts the headline claim.
It says "6 tests". There are 7.
It numbers three phases, but the Dockerfile comments this PR adds define only "phase 1" and "phase 2". There is no phase 3 to grep for.

"Fewer lines" is not one of this PR's merits, so make the honest case instead: one owner for the actor list, no tomllib I/O at import, the set -e fix, fingerprint coverage of the actor table, and 7 tests where there were none.

The set -e fix deserves to be called out. At the base, done < <(list_actor_venvs) swallowed the generator's exit status, so a parse failure would prefetch nothing and the build would still go green. This PR writes the list with a plain redirect plus test -s, which turns that into a build failure. Confirmed by repro: a failing command inside process substitution leaves rc=0 and the script keeps going; the same failure with a plain redirect aborts.

Fingerprint key (91adf33), not a finding: containers built from the roughly two-day window around this branch's base — 6 consecutive main commits, measured — will report a one-time mismatch on the new key. python tools/generate_fingerprint.py > /opt/nemo_rl_container_fingerprint clears it. That window is already closed, and it goes away after merge.

On #3947. @tdene moved the actor list into pyproject.toml at this author's request, said publicly that he preferred a separate file, and asked for a decision on 2026-09-02. #3947 still has zero submitted reviews. This PR reverses that decision, so the description should say so and credit him. There is also a stale pending review by this author on #3947 arguing to keep the pyproject table; it now contradicts this PR and should be submitted or deleted before this one is discussed.

CI has not run. The required CI quality check needs /ok to test from a vetter. All eight lightweight GitHub-hosted checks pass.

Reviewed by a team of six agents; test-agent did not report, so its mutation-testing results are not reflected here.

Generated by Claude Code

Review of #4002 found three issues; this fixes all three. 1. The release-stage prefetch still filtered on a substring of the actor name while phase 1 filtered on declared extras, so under SKIP_VLLM_BUILD=1 phase 1 skipped 7 actors and phase 2 skipped 4. The three in the gap -- AsyncTrajectoryCollector, ReplayBuffer and SyncRolloutActor -- declare ['vllm'] but have no 'vllm' in their name, so they were built in the release layer: a cold download of the vLLM wheels plus an overlayfs copy-up of the shared ones. docs/docker.md documents that flag as the way to cut build time and image size, so it was working against its purpose. Pass the FQNs phase 1 already wrote instead, which also drops the now-dead NEGATIVE_FILTERS block. 2. Restore the import-time check that an actor's extras exist in [project.optional-dependencies]. Moving the table to a Python module dropped it, leaving the check only in a unit test; a typo'd extra would have built a valid-looking 'uv run --extra mcore2' and failed at venv creation -- and during an image build not even then, since prefetch_venvs.py catches the per-actor error and exits 0. It lives in the registry rather than the leaf module because the leaf module must stay dependency-free. 3. main() accepted any string as a stage and any string as a skip extra, so a typo printed nothing and exited 0. Reject both. Verified: reference counts unchanged (18 venvs, 17 deps + 1 trtllm, 11 under SKIP_VLLM_BUILD); the restored check rejects an injected 'mcore2' typo; 59 tests pass; ruff and ruff-format clean. Signed-off-by: Terry Kong <terryk@nvidia.com>

terrykong · 2026-09-04T23:19:23Z

/ok to test 4d60381

terrykong · 2026-09-04T23:39:44Z

/ok to test 5697770

Review of #4002 found three issues; this fixes all three. 1. The release-stage prefetch still filtered on a substring of the actor name while phase 1 filtered on declared extras, so under SKIP_VLLM_BUILD=1 phase 1 skipped 7 actors and phase 2 skipped 4. The three in the gap -- AsyncTrajectoryCollector, ReplayBuffer and SyncRolloutActor -- declare ['vllm'] but have no 'vllm' in their name, so they were built in the release layer: a cold download of the vLLM wheels plus an overlayfs copy-up of the shared ones. docs/docker.md documents that flag as the way to cut build time and image size, so it was working against its purpose. Pass the FQNs phase 1 already wrote instead, which also drops the now-dead NEGATIVE_FILTERS block. 2. Restore the import-time check that an actor's extras exist in [project.optional-dependencies]. Moving the table to a Python module dropped it, leaving the check only in a unit test; a typo'd extra would have built a valid-looking 'uv run --extra mcore2' and failed at venv creation -- and during an image build not even then, since prefetch_venvs.py catches the per-actor error and exits 0. It lives in the registry rather than the leaf module because the leaf module must stay dependency-free. 3. main() accepted any string as a stage and any string as a skip extra, so a typo printed nothing and exited 0. Reject both. Verified: reference counts unchanged (18 venvs, 17 deps + 1 trtllm, 11 under SKIP_VLLM_BUILD); the restored check rejects an injected 'mcore2' typo; 59 tests pass; ruff and ruff-format clean. Signed-off-by: Terry Kong <terryk@nvidia.com>

terrykong · 2026-09-05T00:19:16Z

/ok to test 5dbda0b

terrykong · 2026-09-05T05:47:49Z

CI triage on `5dbda0b47`

Three jobs failed. I re-ran all three. One was flaky and is now green; the other two
reproduce byte-for-byte and neither comes from the commits in this PR.

job	re-run	where it comes from
Build GB200/GCP container	pass	flaky, cleared
`L0_Unit_Tests_Megatron_Policy_1`	same failure	main, `da39e1785` (#2957)
`L0_Unit_Tests_Vllm_1`	same failure	this PR's base, #3947

1. GB200 container build — flaky, now green

ERROR: failed to build: failed to solve: DeadlineExceeded: failed to compute cache key:
failed to copy: httpReadSeeker: failed open: no active session for 4kmry4me3uu7qtr4gcpfhmhnn:
context deadline exceeded

A BuildKit registry-session timeout while pulling a cache blob. The H100 build passed in
the same run on the same Dockerfile. It passed on re-run. Nothing to do.

2. `L0_Unit_Tests_Megatron_Policy_1` — broken on main

test_forwards_model_owned_packing_flags[model-owned-cp-slicing] fails with
NotImplementedError: train_microbatch does not support multimodal models.

Two recently merged PRs disagree:

feat(megatron): support MTP in split training #3881 (merged 18:59) made train_microbatch pass the packing/CP flags to its
microbatch iterator, and added
the test
asserting it does.
feat(vlm): route VLM GRPO through TQ trainer when data_plane.enabled #2957 (merged 22:58) added
a guard
rejecting model_slices_context_parallel_inputs, so that test now raises.

The guard's stated reason does not hold. It says the iterator is "built without
attach_media_token_validity_mask, delegate_pack_to_model,
delegate_mtp_loss_mask_to_model or model_slices_context_parallel_inputs" — but the
same function passes three of those four about a hundred lines below the guard, at
lines 1566-1568.
Only attach_media_token_validity_mask is really missing. It reads like the guard was
written against the code as it stood before #3881 landed four hours earlier.

model_slices_context_parallel_inputs is also not a multimodal signal on its own —
_model_slices_context_parallel_inputs()
reads a model capability about slicing CP after embedding.

So either the guard should narrow to media_placeholder_token_id is not None, which is
what its message claims to check, or #3881's test case should change. That is a call for
the #2957 and #3881 authors, so I have not touched it here.

This is red on main right now, so it blocks anything rebased onto da39e1785, not just
this PR. Rebasing does not help: main has moved two commits past it (#3903, #3914) and
neither touches this code.

3. `L0_Unit_Tests_Vllm_1` — comes from #3947, not from my commits

test_processed_logprobs_matches_manual_computation fails at import:

ImportError: cannot import name 'NamespaceTool' from 'openai.types.responses'
(/opt/nemo_rl_venv/lib/python3.13/site-packages/openai/types/responses/__init__.py)

vLLM 0.25.1's tool parser wants a newer openai than the locked 2.6.1.

This is not from the four commits on top of #3947. #3947 fails the same test with the same
error at a6e758fdd, dated 2026-09-02, before any of them existed — and uv.lock and
pyproject.toml here are byte-identical to main.

One hypothesis for @tdene, not proven. Main ends the TRT-LLM layer with a
restore sync:

uv sync --link-mode symlink --locked --all-groups --no-install-project

#3947 removes it, on the grounds that the TRT-LLM build now runs in a throwaway venv and
no longer pollutes the main one. That part is right. But this was also the last step that
forced /opt/nemo_rl_venv back to exactly the locked set, and uv run is inexact by
default, so dropping it lets earlier steps leave packages behind at versions the lock did
not pick. That would explain why the failure shows up on full rebuilds (#3947 and this PR,
both ~1h15m builds) and not on cache-reusing ones (#4009, 21m).

Worth checking before anything else: print openai.__version__ from /opt/nemo_rl_venv
in a built image and compare it against uv.lock.

Review of #4002 found three issues; this fixes all three. 1. The release-stage prefetch still filtered on a substring of the actor name while phase 1 filtered on declared extras, so under SKIP_VLLM_BUILD=1 phase 1 skipped 7 actors and phase 2 skipped 4. The three in the gap -- AsyncTrajectoryCollector, ReplayBuffer and SyncRolloutActor -- declare ['vllm'] but have no 'vllm' in their name, so they were built in the release layer: a cold download of the vLLM wheels plus an overlayfs copy-up of the shared ones. docs/docker.md documents that flag as the way to cut build time and image size, so it was working against its purpose. Pass the FQNs phase 1 already wrote instead, which also drops the now-dead NEGATIVE_FILTERS block. 2. Restore the import-time check that an actor's extras exist in [project.optional-dependencies]. Moving the table to a Python module dropped it, leaving the check only in a unit test; a typo'd extra would have built a valid-looking 'uv run --extra mcore2' and failed at venv creation -- and during an image build not even then, since prefetch_venvs.py catches the per-actor error and exits 0. It lives in the registry rather than the leaf module because the leaf module must stay dependency-free. 3. main() accepted any string as a stage and any string as a skip extra, so a typo printed nothing and exited 0. Reject both. Verified: reference counts unchanged (18 venvs, 17 deps + 1 trtllm, 11 under SKIP_VLLM_BUILD); the restored check rejects an injected 'mcore2' typo; 59 tests pass; ruff and ruff-format clean. Signed-off-by: Terry Kong <terryk@nvidia.com>

terrykong · 2026-09-07T01:38:47Z

/ok to test 35a2a29

terrykong · 2026-09-07T05:04:53Z

Correction: both failures are gone after rebasing onto `c0165c8c2`

Rebased to 35a2a299b. All 23 L0 unit test jobs pass, and both container builds pass.
Everything I flagged in the comment above is resolved, and one part of that comment was wrong.

`L0_Unit_Tests_Megatron_Policy_1` — fixed on main, as expected

c0165c8c2 narrowed the guard to media_placeholder_token_id is not None and dropped the
model_slices_context_parallel_inputs clause, which is what the diagnosis above predicted.

`L0_Unit_Tests_Vllm_1` — passes now, and my explanation for it was wrong

@tdene, please ignore the restore-sync theory in my previous comment — it does not hold.
I said dropping the uv sync --link-mode symlink --locked --all-groups step at the end of the
TRT-LLM layer probably let a stale openai survive in /opt/nemo_rl_venv. That was a guess and
it is not what was happening.

What rules it out: the Dockerfile in this PR is unchanged, that sync is still deleted, and the
H100 image was rebuilt from scratch again (1h13m, same as the failing run). The only thing that
changed is the base. So the Dockerfile change cannot have been the cause.

What actually changed is the lock. Between da39e1785 and c0165c8c2, uv.lock dropped its
third vllm entry — it used to carry 0.24.0 alongside two 0.25.1 entries, and now carries
only the two 0.25.1 ones — and the openai dependency markers were rewritten. openai itself
is still 2.6.1 in both. So this was a resolution problem in the lock, fixed on main by
#3837 / #4022, not anything about how the image is built.

I also predicted this job would still fail after the rebase. It passed. Recording that here
because the earlier comment stated the theory with more confidence than the evidence supported.

Review of #4002 found three issues; this fixes all three. 1. The release-stage prefetch still filtered on a substring of the actor name while phase 1 filtered on declared extras, so under SKIP_VLLM_BUILD=1 phase 1 skipped 7 actors and phase 2 skipped 4. The three in the gap -- AsyncTrajectoryCollector, ReplayBuffer and SyncRolloutActor -- declare ['vllm'] but have no 'vllm' in their name, so they were built in the release layer: a cold download of the vLLM wheels plus an overlayfs copy-up of the shared ones. docs/docker.md documents that flag as the way to cut build time and image size, so it was working against its purpose. Pass the FQNs phase 1 already wrote instead, which also drops the now-dead NEGATIVE_FILTERS block. 2. Restore the import-time check that an actor's extras exist in [project.optional-dependencies]. Moving the table to a Python module dropped it, leaving the check only in a unit test; a typo'd extra would have built a valid-looking 'uv run --extra mcore2' and failed at venv creation -- and during an image build not even then, since prefetch_venvs.py catches the per-actor error and exits 0. It lives in the registry rather than the leaf module because the leaf module must stay dependency-free. 3. main() accepted any string as a stage and any string as a skip extra, so a typo printed nothing and exited 0. Reject both. Verified: reference counts unchanged (18 venvs, 17 deps + 1 trtllm, 11 under SKIP_VLLM_BUILD); the restored check rejects an injected 'mcore2' typo; 59 tests pass; ruff and ruff-format clean. Signed-off-by: Terry Kong <terryk@nvidia.com>

terrykong · 2026-09-07T23:48:27Z

/ok to test b142cb2

terrykong · 2026-09-08T00:49:51Z

/ok to test b142cb2

Review of #4002 found three issues; this fixes all three. 1. The release-stage prefetch still filtered on a substring of the actor name while phase 1 filtered on declared extras, so under SKIP_VLLM_BUILD=1 phase 1 skipped 7 actors and phase 2 skipped 4. The three in the gap -- AsyncTrajectoryCollector, ReplayBuffer and SyncRolloutActor -- declare ['vllm'] but have no 'vllm' in their name, so they were built in the release layer: a cold download of the vLLM wheels plus an overlayfs copy-up of the shared ones. docs/docker.md documents that flag as the way to cut build time and image size, so it was working against its purpose. Pass the FQNs phase 1 already wrote instead, which also drops the now-dead NEGATIVE_FILTERS block. 2. Restore the import-time check that an actor's extras exist in [project.optional-dependencies]. Moving the table to a Python module dropped it, leaving the check only in a unit test; a typo'd extra would have built a valid-looking 'uv run --extra mcore2' and failed at venv creation -- and during an image build not even then, since prefetch_venvs.py catches the per-actor error and exits 0. It lives in the registry rather than the leaf module because the leaf module must stay dependency-free. 3. main() accepted any string as a stage and any string as a skip extra, so a typo printed nothing and exited 0. Reject both. Verified: reference counts unchanged (18 venvs, 17 deps + 1 trtllm, 11 under SKIP_VLLM_BUILD); the restored check rejects an injected 'mcore2' typo; 59 tests pass; ruff and ruff-format clean. Signed-off-by: Terry Kong <terryk@nvidia.com>

terrykong · 2026-09-08T03:59:37Z

/ok to test 01c1ade

Replaces the symlink scheme with hardlinks. Increased size of image by ~3%. Reduces number of symlinks from ~1.5M to ~3k. Improves SQSH conversion time from ~3 hours to ~25 minutes. Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>

Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>

Keeps tdene's three-layer hardlink prefetch exactly as-is, but moves the actor -> uv extras mapping out of pyproject.toml and back into Python, so there is one list with one owner. - nemo_rl/distributed/actor_environments.py holds ACTOR_ENVIRONMENTS. It is stdlib-only and dependency-free, so docker/Dockerfile can run it as a script from the dependency layer, where the source tree does not exist yet. Running it as a script (not importing) also keeps nemo_rl/__init__.py from executing there. - ray_actor_environment_registry.py imports that dict instead of parsing pyproject.toml at import time. No file I/O, no tomllib. - The Dockerfile's two byte-identical tomllib heredocs are gone. The list is written once to /opt/actor_venvs.tsv with a plain redirect, so set -e catches a parse failure instead of silently prefetching nothing. - SKIP_*_BUILD now filters on the declared extras rather than a substring of the actor name, so SKIP_VLLM_BUILD also skips AsyncTrajectoryCollector, ReplayBuffer and SyncRolloutActor. - Phase 2 sets TRTLLM_REQUIRE_CACHED_WHEEL=1, matching the release stage, so a cache miss fails fast instead of starting a source build. - Adds tests: extras are declared, actor modules exist, generated py_executables match PY_EXECUTABLES, script output matches the registry, and actor_environments.py imports only stdlib. Signed-off-by: Terry Kong <terryk@nvidia.com>

Venvs under NEMO_RL_VENV_DIR are reused rather than rebuilt, and after #3947 nothing prunes them -- the base sync passes --inexact and 'uv run' is inexact by default. So changing an actor's extras leaves the old extra's packages installed in a venv that is silently reused. _check_container_fingerprint() is what catches that and points the user at NRL_FORCE_REBUILD_VENVS=true, but generate_fingerprint.py hashes only pyproject.toml, uv.lock and the submodule SHAs. While the table lived in pyproject.toml that was covered by accident; moving it to a .py file dropped the coverage. Hash the table explicitly so the check still fires. Verified: swapping MegatronValueWorker from ['mcore'] to ['automodel'] changes the fingerprint hash. Adds a test so the coupling cannot break silently. Signed-off-by: Terry Kong <terryk@nvidia.com>

It was declared three times with the same value: base, hermetic, and release. hermetic and release both derive from base, so the later two were dead. Worse, the hermetic one came *after* the worker-venv prefetch that reads it, so changing it there would have looked effective while the prefetch kept using base's value. Keep the base declaration, note that the later stages inherit it. Signed-off-by: Terry Kong <terryk@nvidia.com>

The dependency stage builds the actor venvs by hand rather than calling prefetch_venvs.py, and nothing said why. The reason is that prefetch_venvs.py installs the nemo_rl project, and the source tree is not in that layer yet -- it arrives in the release stage on purpose, so a source edit does not invalidate the hour-long dependency build. So phase 1 does the expensive cacheable half (third-party packages, hardlinked, --no-install-project) and phase 2 does the cheap half (editable install plus the wrapper scripts). Also corrects the NeMo Gym note: UV_LINK_MODE=symlink applies to the outer uv run, but the gym server venvs are hardlinked -- prefetch_venvs.py pins UV_LINK_MODE=hardlink in the Gym actor's runtime_env (#3785), because Gym's setup scripts use 'uv pip install --no-cache', which uv refuses to combine with symlink installs. Signed-off-by: Terry Kong <terryk@nvidia.com>

Review of #4002 found three issues; this fixes all three. 1. The release-stage prefetch still filtered on a substring of the actor name while phase 1 filtered on declared extras, so under SKIP_VLLM_BUILD=1 phase 1 skipped 7 actors and phase 2 skipped 4. The three in the gap -- AsyncTrajectoryCollector, ReplayBuffer and SyncRolloutActor -- declare ['vllm'] but have no 'vllm' in their name, so they were built in the release layer: a cold download of the vLLM wheels plus an overlayfs copy-up of the shared ones. docs/docker.md documents that flag as the way to cut build time and image size, so it was working against its purpose. Pass the FQNs phase 1 already wrote instead, which also drops the now-dead NEGATIVE_FILTERS block. 2. Restore the import-time check that an actor's extras exist in [project.optional-dependencies]. Moving the table to a Python module dropped it, leaving the check only in a unit test; a typo'd extra would have built a valid-looking 'uv run --extra mcore2' and failed at venv creation -- and during an image build not even then, since prefetch_venvs.py catches the per-actor error and exits 0. It lives in the registry rather than the leaf module because the leaf module must stay dependency-free. 3. main() accepted any string as a stage and any string as a skip extra, so a typo printed nothing and exited 0. Reject both. Verified: reference counts unchanged (18 venvs, 17 deps + 1 trtllm, 11 under SKIP_VLLM_BUILD); the restored check rejects an injected 'mcore2' typo; 59 tests pass; ruff and ruff-format clean. Signed-off-by: Terry Kong <terryk@nvidia.com>

Mutation testing found three gaps in the tests added by this PR. Each mutant below passed the whole suite before this change and fails after it. - main() emitting only the first extra passed all 59 tests. Both script tests parsed only column 1 (the actor name), but the Dockerfile consumes column 3 as the flags for 'uv sync $extras'. Dropping extras would have shipped the modelopt venvs without vllm/automodel/mcore. - _build_stage always returning "deps" passed all 59 tests. The deps layer branches on column 2 to leave the TRT-LLM venv base-only until its wheel exists, so this would make it try --extra trtllm too early. - The fingerprint test asserted only that the key existed and was non-empty, so pointing the entry at uv.lock passed while its docstring claimed the hash tracks the actor table. Adds test_script_emits_the_stage_and_extra_flags_each_actor_needs and strengthens test_fingerprint_covers_the_actor_table to compare against the file's md5. 60 tests pass; each of the three mutants now fails. Signed-off-by: Terry Kong <terryk@nvidia.com>

Mutation testing showed all three guards added in 4d60381 were untested -- deleting any of them left the whole suite green: - main() rejecting an unknown stage - main() rejecting an unknown skip extra - _reject_undeclared_extras() running at registry import The third needed care. A test that calls _reject_undeclared_extras() directly still passes when the call site is deleted, which is exactly the defect worth catching, so the test imports the registry module fresh with a typo'd extra patched into the table and asserts the import raises. Each of the three deletions now fails the suite. 62 tests pass. Signed-off-by: Terry Kong <terryk@nvidia.com>

#3917 added nemo_rl.data.energon.sft_worker.SFTMegatronPolicyWorker to ray_actor_environment_registry.py with the mcore extra. This branch builds that registry from ACTOR_ENVIRONMENTS instead of a literal dict, so the new entry did not carry over on rebase and the actor would have raised from get_actor_python_env at launch. Signed-off-by: Terry Kong <terryk@nvidia.com>

#4009 pointed VLLM_EXECUTABLE at PY_EXECUTABLES.VLLM_GYM so token capture (token_capture.enabled) can import nemo_gym inside the worker. Worker venvs are cached by actor class name, so a venv prebuilt with plain --extra vllm is reused as-is and the import fails; that is the L1_Functional_Tests_SingleController failure on this branch. This branch builds the registry from ACTOR_ENVIRONMENTS rather than the literal dict on main, so the change did not carry over on rebase. Add a guard test: a PY_EXECUTABLES constant that names extras but is wired to no actor is the signature of exactly this miss. Signed-off-by: Terry Kong <terryk@nvidia.com>

#4020 moved the flag into PY_EXECUTABLES._resolve_system_overrides and says callers should not check it themselves. That rewrite only touches the class attributes, so uv_py_executable -- which builds a fresh string -- was not covered, and this branch had to keep its own USE_SYSTEM_EXECUTABLE check. Put the check in uv_py_executable instead and drop the one in the registry, so the flag is handled in one place. Verified: with the flag set, all 28 actors resolve to the driver interpreter. Signed-off-by: Terry Kong <terryk@nvidia.com>

terrykong · 2026-09-08T05:05:52Z

/ok to test 4c839ea

The example still showed ["vllm"], but the vLLM workers carry ["vllm", "nemo_gym"] since #4009 moved them onto PY_EXECUTABLES.VLLM_GYM. Use the real value and say why, since a two-extra entry is otherwise puzzling. Signed-off-by: Terry Kong <terryk@nvidia.com>

yuki-97

Scope: the move of the actor -> uv extras table into nemo_rl/distributed/actor_environments.py, its two readers (the runtime registry and docker/Dockerfile), the venv prefetch split across the dependency and TRT-LLM layers, the container fingerprint change, and the new test module.

Checked:

Removals worklist -- every named thing the diff deletes, grepped repo-wide at head and confirmed at its definition site.
Dead surface -- reachability walked from each new symbol back to a shipped caller, in both directions.
Skip semantics -- SKIP_*_BUILD traced end to end through SKIP_EXTRAS, the TSV and all three consumers. The extras axis drops 7 actors where the old name-substring axis dropped 4; the three it adds are the ones the two passes used to disagree on.
Doc surfaces -- every prose claim about the registry, the prefetch and the fingerprint checked against the code it describes.
Byte-equivalence -- both actor -> py_executable mappings reconstructed on main and at head, flag set and unset: 28 actors, no differing value.

Remaining comments are on dead surface left behind by the move, and on prose that contradicts the code. Nothing blocking. The new test module was reviewed as well but carries no comments from me -- the existing threads there already cover it.

@tdene

Review feedback from @tdene. test_actor_extras_are_declared asserted that every extra is declared in pyproject.toml, but this module imports the registry, whose import runs _reject_undeclared_extras(). An undeclared extra therefore raises during collection and the assertion is never reached -- verified by mutation: pointing an actor at a bogus extra produces a collection ERROR, not a failure. Keep the type check, which IS reachable: a tuple of a real extra passes the import-time check (it only does set arithmetic) and fails here. Renamed to say what it now does, and dropped the DECLARED_EXTRAS/tomllib read it no longer needs. Signed-off-by: Terry Kong <terryk@nvidia.com>

@tdene

Review feedback from @tdene. test_script_emits_the_stage_and_extra_flags_each_actor_needs asserts `rows == expected`, where both dicts are keyed by actor FQN, so dict equality already requires the same actor list -- which was all test_script_output_matches_the_registry checked. Drop the weaker test and say in the docstring that the surviving one covers both. Signed-off-by: Terry Kong <terryk@nvidia.com>

@tdene

Review feedback from @tdene. prefetch_venvs collected failures into `failed`, printed them in the summary, and then dropped them -- the function returned None and __main__ never checked anything, so the process exited 0. This runs in the release stage of the image build, so a venv that failed to build shipped a green image with that venv missing, and the actor died the first time someone launched it. Return `failed` and exit 1 from __main__. The per-actor loop still continues after a failure, so one broken venv does not hide the rest. Pre-existing on main; not introduced by this PR. Signed-off-by: Terry Kong <terryk@nvidia.com>

@yuki-97

Review feedback from @yuki-97. This PR removed the release stage's NEGATIVE_FILTERS block -- exclusion now happens upstream on the extras axis, in the dependency layer -- which left --negative-filters with no caller anywhere in the repo and no test coverage. Its FQN-substring matching is also the rule this PR moved away from, since it cannot see that AsyncTrajectoryCollector needs the vLLM extra. Verified with git grep: after this, no reference to negative_filters remains. Signed-off-by: Terry Kong <terryk@nvidia.com>

@yuki-97

Review feedback from @yuki-97. SKIP_VLLM_BUILD / SKIP_SGLANG_BUILD / SKIP_TRTLLM_BUILD were re-declared in the release stage only to feed the NEGATIVE_FILTERS block this PR removed. Nothing after `FROM hermetic AS release` reads them now. The flags themselves are unaffected: the hermetic stage still declares all three and consumes them for the backend syncs and the SKIP_EXTRAS list. Signed-off-by: Terry Kong <terryk@nvidia.com>

@yuki-97

Review feedback from @yuki-97. The docstring said only this file plus pyproject.toml and uv.lock exist in that layer, then explained that running as a script avoids executing nemo_rl/__init__.py -- which only makes sense because __init__.py IS copied there. docker/Dockerfile also copies package_info.py, two tools/ scripts, research/ and 3rdparty/. Keep the reason the DO NOT IMPORT rule rests on -- the rest of the package is absent -- without the false claim about the layer's contents. Signed-off-by: Terry Kong <terryk@nvidia.com>

@yuki-97

Review feedback from @yuki-97. This PR made the fingerprint hash a third file, but the version-check page still named only pyproject.toml and uv.lock, and the sample mismatch warning had no entry for it. Someone who edits an actor's extras would see an unexplained key in the warning. The key name is copied from generate_fingerprint()'s actual output. Signed-off-by: Terry Kong <terryk@nvidia.com>

terrykong · 2026-09-08T21:55:36Z

/ok to test 6a4e496

github-actions Bot added the Documentation Improvements or additions to documentation label Sep 4, 2026

terrykong commented Sep 4, 2026

View reviewed changes

Comment thread docker/Dockerfile Outdated

Comment thread nemo_rl/distributed/ray_actor_environment_registry.py

Comment thread nemo_rl/distributed/actor_environments.py

terrykong added the CI:L1 Run doctests, unit tests, and functional tests label Sep 4, 2026

terrykong force-pushed the terryk/pr-3947-leaf-module branch from 5697770 to 5dbda0b Compare September 5, 2026 00:18

terrykong marked this pull request as ready for review September 5, 2026 06:34

terrykong requested review from a team as code owners September 5, 2026 06:34

yuki-97 mentioned this pull request Sep 5, 2026

fix: honor NEMO_RL_PY_EXECUTABLES_SYSTEM for all actor environments #4020

Merged

4 tasks

terrykong force-pushed the terryk/pr-3947-leaf-module branch from 5dbda0b to 35a2a29 Compare September 7, 2026 01:38

terrykong force-pushed the terryk/pr-3947-leaf-module branch from 35a2a29 to b142cb2 Compare September 7, 2026 23:48

terrykong force-pushed the terryk/pr-3947-leaf-module branch from b142cb2 to 01c1ade Compare September 8, 2026 03:59

tdene added 2 commits September 7, 2026 21:48

Fix TRTLLM layer

206a7f2

Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>

tdene and others added 11 commits September 7, 2026 21:56

Move manifest to pyproject.toml

d09d3eb

Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>

terrykong force-pushed the terryk/pr-3947-leaf-module branch from 01c1ade to 4c839ea Compare September 8, 2026 05:05

tdene reviewed Sep 8, 2026

View reviewed changes

Comment thread nemo_rl/distributed/ray_actor_environment_registry.py

Comment thread tests/unit/distributed/test_actor_environments.py Outdated

Comment thread tests/unit/distributed/test_actor_environments.py

yuki-97 reviewed Sep 8, 2026

View reviewed changes

terrykong added 7 commits September 8, 2026 11:39

yuki-97 approved these changes Sep 9, 2026

View reviewed changes

yuki-97 enabled auto-merge (squash) September 9, 2026 00:16

yuki-97 merged commit 7b1510a into main Sep 9, 2026
104 checks passed

yuki-97 deleted the terryk/pr-3947-leaf-module branch September 9, 2026 02:43

Uh oh!

Conversation

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

Uh oh!

What this changes about #3947

The approach

The honest case for it

Behavior changes, declared

Tests

What this trades away

Open questions

Uh oh!

copy-pr-bot Bot commented Sep 4, 2026

Uh oh!

terrykong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

terrykong commented Sep 4, 2026

Uh oh!

terrykong commented Sep 4, 2026

Uh oh!

terrykong commented Sep 5, 2026

Uh oh!

terrykong commented Sep 5, 2026

CI triage on 5dbda0b47

1. GB200 container build — flaky, now green

2. L0_Unit_Tests_Megatron_Policy_1 — broken on main

3. L0_Unit_Tests_Vllm_1 — comes from #3947, not from my commits

Uh oh!

terrykong commented Sep 7, 2026

Uh oh!

terrykong commented Sep 7, 2026

Correction: both failures are gone after rebasing onto c0165c8c2

L0_Unit_Tests_Megatron_Policy_1 — fixed on main, as expected

L0_Unit_Tests_Vllm_1 — passes now, and my explanation for it was wrong

Uh oh!

terrykong commented Sep 7, 2026

Uh oh!

terrykong commented Sep 8, 2026

Uh oh!

terrykong commented Sep 8, 2026

Uh oh!

terrykong commented Sep 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuki-97 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

terrykong commented Sep 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

terrykong commented Sep 4, 2026 •

edited

Loading

CI triage on `5dbda0b47`

2. `L0_Unit_Tests_Megatron_Policy_1` — broken on main

3. `L0_Unit_Tests_Vllm_1` — comes from #3947, not from my commits

Correction: both failures are gone after rebasing onto `c0165c8c2`

`L0_Unit_Tests_Megatron_Policy_1` — fixed on main, as expected

`L0_Unit_Tests_Vllm_1` — passes now, and my explanation for it was wrong