Skip to content

fix(runtime): honor system Python for all actor environments - #3982

Closed
jepio wants to merge 1 commit into
mainfrom
fix/system-python-actor-environments
Closed

fix(runtime): honor system Python for all actor environments#3982
jepio wants to merge 1 commit into
mainfrom
fix/system-python-actor-environments

Conversation

@jepio

@jepio jepio commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

What does this PR do ?

Makes NEMO_RL_PY_EXECUTABLES_SYSTEM=1 apply to the FSDP, Automodel, NeMo Gym, trajectory collector, replay buffer, and synchronous rollout actor environments. This prevents those actors from creating specialized uv environments when the runtime uses system Python.

The default mappings remain unchanged when the flag is disabled. ModelOpt retains its existing independent handling of the system-Python flag.

Issues

N/A

Usage

Set NEMO_RL_PY_EXECUTABLES_SYSTEM=1 when all actor dependencies are available in the system Python environment.

Before your PR is "Ready for review"

Pre checks:

  • Read and followed the contributor guidelines.
  • Added focused tests for both flag states.
  • Ran the focused unit tests locally in Lima.
  • No documentation change is needed.

Additional Information

Validation in Lima:

  • Focused unit tests: 2 passed.
  • End-of-file, trailing-whitespace, Ruff lint, Ruff format, and Pyrefly checks passed for the changed Python files.

@jepio
jepio requested review from a team as code owners September 3, 2026 16:24
@copy-pr-bot

copy-pr-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

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.

@jepio jepio added the CI:L0 Run doctests and unit tests label Sep 3, 2026
@jepio

jepio commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test b869b51

@jepio jepio added CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) and removed CI:L0 Run doctests and unit tests labels Sep 3, 2026
Signed-off-by: Jeremi Piotrowski <jpiotrowski@nvidia.com>
@jepio
jepio force-pushed the fix/system-python-actor-environments branch from b869b51 to 799ff4f Compare September 3, 2026 16:37
@jepio

jepio commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 799ff4f

@jepio jepio left a comment

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.

Agent-team review of PR #3982. Clean, well-scoped bug fix — 3 actors (AsyncTrajectoryCollector, ReplayBuffer, SyncRolloutActor) were hardcoded to raw PY_EXECUTABLES.VLLM instead of the flag-aware VLLM_EXECUTABLE, so they silently ignored NEMO_RL_PY_EXECUTABLES_SYSTEM=1. No bugs found; registry completeness and ModelOpt interaction independently verified. Two low-severity, non-blocking suggestions below (doc + test coverage).

Generated by Claude Code

PY_EXECUTABLES.SYSTEM if USE_SYSTEM_EXECUTABLE else PY_EXECUTABLES.TRTLLM
)

FSDP_EXECUTABLE = (

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.

nemo_rl/distributed/ray_actor_environment_registry.py:33

1 action item.

NEMO_RL_PY_EXECUTABLES_SYSTEM isn't documented anywhere in the repo outside this PR's description (only referenced in the two registry .py files). This PR expands its effect from generation-only backends to the full training path (FSDP/Automodel/NeMo Gym) plus the async rollout actors, so it's worth a sentence in docs/design-docs/uv.md for future readers.

AI-1

Add a sentence to docs/design-docs/uv.md (around line 43, end of the "Worker Configuration" paragraph) documenting the flag, e.g.:

Setting the NEMO_RL_PY_EXECUTABLES_SYSTEM=1 environment variable overrides most registry entries to PY_EXECUTABLES.SYSTEM, skipping per-actor uv venv creation when all dependencies are already available in the system Python environment.

for actor in DEFAULT_EXECUTABLES
} == dict.fromkeys(DEFAULT_EXECUTABLES, registry.PY_EXECUTABLES.SYSTEM)
finally:
_restore_actor_environment_registry(original_registry)

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.

tests/unit/distributed/test_ray_actor_environment_registry.py:66

1 action item, optional.

Pre-existing coverage gap (not introduced by this PR): no test guards that the always-PY_EXECUTABLES.SYSTEM actors (DynamoVllmWorker, MathEnvironment, etc.) stay pinned to SYSTEM regardless of the flag. Verified locally in lima (4 passed, 1 warning in 33.90s).

AI-1

Add a parametrized regression test (also needs import pytest added near the top, after import importlib):

Suggested change
_restore_actor_environment_registry(original_registry)
finally:
_restore_actor_environment_registry(original_registry)
ALWAYS_SYSTEM_EXECUTABLES = [
"nemo_rl.models.generation.dynamo.dynamo_worker.DynamoVllmWorker",
"nemo_rl.environments.math_environment.MathEnvironment",
"nemo_rl.environments.math_environment.MathMultiRewardEnvironment",
"nemo_rl.environments.vlm_environment.VLMEnvironment",
"nemo_rl.environments.code_environment.CodeEnvironment",
"nemo_rl.environments.reward_model_environment.RewardModelEnvironment",
"nemo_rl.environments.code_jaccard_environment.CodeJaccardEnvironment",
"nemo_rl.environments.games.sliding_puzzle.SlidingPuzzleEnv",
"nemo_rl.environments.tools.retriever.RAGEnvironment",
]
@pytest.mark.parametrize("system_flag", ["0", "1"])
def test_always_system_actor_environments_are_unaffected_by_flag(
monkeypatch, system_flag
):
original_registry = registry.ACTOR_ENVIRONMENT_REGISTRY
try:
with monkeypatch.context() as context:
context.setenv("NEMO_RL_PY_EXECUTABLES_SYSTEM", system_flag)
importlib.reload(registry)
assert {
actor: registry.get_actor_python_env(actor)
for actor in ALWAYS_SYSTEM_EXECUTABLES
} == dict.fromkeys(
ALWAYS_SYSTEM_EXECUTABLES, registry.PY_EXECUTABLES.SYSTEM
)
finally:
_restore_actor_environment_registry(original_registry)

@yuki-97

yuki-97 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

closing in favor of #4020.

@yuki-97 yuki-97 closed this Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants