Unify env random seeding using ManagerBasedRLEnv seed

[peterd-NV]

Summary

Arena was previously setting the seed using a custom set_seed function in Arena. The seeding was inconsistent and not always being set (for example it was not used for batched eval). This gave non-deterministic results.

This PR removes the inconsistent env seeding and uses Isaac Lab's built in env random seeding mechanism. Any time a user provides a seed via CLI, it is passed to the arena environment builder which sets it in the env_cfg. Isaac Lab then handles setting all global seeds.

Note: Isaac Lab's seed mechanism sets all of the seeds that Arena was setting plus additional seeds for replicator and warp. It is more comprehensive than what Arena had.

[greptile-apps]

Greptile Summary

Replaces Arena's custom set_seed helper with Isaac Lab's built-in ManagerBasedRLEnv seeding by writing env_cfg.seed in ArenaEnvBuilder.compose_manager_cfg. This gives broader coverage (replicator, warp) and ensures the seed is applied consistently across all launch paths, including batched eval, which previously received no seeding.

• arena_env_builder.py: env_cfg.seed is assigned after env_cfg_callback so the CLI value is authoritative; --seed defaults to 42 at the CLI layer so the field is never accidentally None.
• policy_runner.py: The per-rank seed offset (seed += local_rank) is now applied to args_cli.seed before env construction, so the adjusted seed flows through env_cfg.seed correctly.
• utils/random.py: set_seed and its gymnasium/numpy imports are removed cleanly; get_random_rotation and get_rngs are unchanged.

Confidence Score: 5/5

Safe to merge — the refactor correctly routes seeding through Isaac Lab's existing mechanism with no regressions in the non-distributed or distributed paths.

All three call paths (single-process, distributed, batched eval) now receive the seed via env_cfg.seed before env construction. The per-rank offset is applied at the right moment, the callback can still influence env_cfg before the seed is pinned, and the removed set_seed function had no remaining callers.

No files require special attention.

Important Files Changed

Filename	Overview
isaaclab_arena/environments/arena_env_builder.py	Adds env_cfg.seed assignment after the callback (correct placement); seed is unconditionally set from CLI, defaulting to 42. Missing clarifying comment about why it follows the callback.
isaaclab_arena/evaluation/policy_runner.py	Moves per-rank seed offset to before env construction so the adjusted seed propagates through env_cfg.seed; removes now-redundant set_seed call.
isaaclab_arena/utils/random.py	Removes set_seed and its gymnasium/numpy imports; remaining get_random_rotation and get_rngs helpers are untouched and unaffected.

Sequence Diagram

sequenceDiagram
    participant CLI as CLI (--seed)
    participant PR as policy_runner.py
    participant AB as ArenaEnvBuilder
    participant IL as Isaac Lab (ManagerBasedRLEnv)

    CLI->>PR: args_cli.seed (default 42)
    Note over PR: Distributed? seed += local_rank
    PR->>AB: make_registered_and_return_cfg(args_cli)
    AB->>AB: env_cfg_callback(env_cfg)
    Note over AB: After callback:<br/>env_cfg.seed = args.seed
    AB->>IL: gym.make(env_name, env_cfg)
    Note over IL: Sets torch / cuda / numpy /<br/>random / replicator / warp seeds
    IL-->>PR: env

Loading

_{Reviews (2): Last reviewed commit: "fix seed setting location in env builder" | Re-trigger Greptile}

[isaaclab-review-bot]

Review Summary

Clean refactoring that unifies environment seeding through Isaac Lab's built-in ManagerBasedRLEnv.seed mechanism, eliminating the inconsistent custom set_seed utility. The approach is sound — centralizing seed propagation in compose_manager_cfg ensures all code paths through ArenaEnvBuilder (including batched eval) get consistent seeding.

---

Findings

1. 🟡 Seed mutation of args_cli is a side effect that persists beyond its intended scope

File: isaaclab_arena/evaluation/policy_runner.py (line ~172)

if args_cli.seed is not None:
    args_cli.seed += local_rank

This mutates args_cli.seed in-place before passing it to get_arena_builder_from_cli. Since args_cli is a namespace object shared throughout main(), any subsequent code that reads args_cli.seed will see the modified value (base_seed + local_rank) rather than the original user-supplied seed. While nothing currently reads it afterwards, this is fragile — future code (e.g., logging the user's requested seed, or a second env build) would silently get the wrong value.

Suggestion: Use a local variable instead:

env_seed = args_cli.seed
if env_seed is not None and is_distributed(args_cli):
    env_seed += local_rank
args_cli.seed = env_seed  # or pass env_seed explicitly

Or at minimum, add a comment noting the intentional mutation.

2. 🟡 env_cfg.seed is set before the env_cfg_callback, which could silently override it

File: isaaclab_arena/environments/arena_env_builder.py (line ~295-296)

# Set seed for Isaac Lab env.
env_cfg.seed = getattr(self.args, "seed", None)

# Apply the environment configuration callback if it is set
if self.arena_env.env_cfg_callback is not None:
    env_cfg = self.arena_env.env_cfg_callback(env_cfg)

The seed is set before the callback runs. If any env_cfg_callback also sets env_cfg.seed (e.g., for environment-specific reproducibility requirements), it will silently override the CLI-provided seed without warning. Conversely, if the intent is that CLI --seed should always be authoritative, consider moving the seed assignment after the callback (similar to how --presets is applied after the callback with the comment "the user's CLI choice is the final authority").

3. 🟢 Minor: The --seed CLI default is 42, so getattr(self.args, "seed", None) will never return None

File: isaaclab_arena/environments/arena_env_builder.py (line ~296)

The CLI defines --seed with default=42, meaning args.seed will always be an integer (never None) when invoked through standard CLI paths. The getattr(..., None) fallback only activates if self.args is a manually constructed namespace without a seed attribute. This isn't a bug, but the code reads as if None is a normal case. A brief comment clarifying the intent (e.g., "None when seed is explicitly disabled or args built programmatically") would improve readability.

4. 🟢 Dead code removal is incomplete — set_seed function body removed but not the function itself

File: isaaclab_arena/utils/random.py

The diff removes the set_seed function entirely, which is correct since no other module imports it (confirmed: only policy_runner.py imported it). The cleanup is complete. ✅ (No action needed — this is confirmation that the removal is safe.)

5. 🟡 No test coverage for the new seeding path

The PR removes the set_seed call but doesn't add tests verifying that env_cfg.seed is correctly propagated through compose_manager_cfg. Given that the PR description mentions this fixes non-deterministic results in batched eval, a regression test (even a simple unit test asserting compose_manager_cfg().seed == args.seed) would protect against future refactors accidentally breaking seed propagation.

---

Overall Assessment

This is a well-motivated simplification. Isaac Lab's native seeding is indeed more comprehensive (covers replicator + warp in addition to torch/numpy/random). The main concerns are:

1. The in-place mutation pattern for distributed seeds (minor robustness issue)
2. Ordering relative to env_cfg_callback (design decision worth documenting)
3. Lack of test coverage for the new path

None of these are blocking. Nice cleanup! 👍

---

Update (commit f940f58): ✅ Finding #2 addressed — seed assignment moved after env_cfg_callback, ensuring CLI --seed is always authoritative (matching --presets pattern). No new issues in this commit. Remaining findings (#1 args mutation, #3 minor readability, #5 test coverage) are non-blocking and unchanged.

[xyao-nv]

Thanks for fixing!

[xyao-nv]

How about for the seed you modify here, we call it runtime_seed? To differentiate it from placement_seed.

[alexmillane]

Looks good to me!!! Thanks for doing this!

We still have this variable placement_seed around. I wonder if we can also simplify by getting rid of that? But that's a question for another time.

[alexmillane]

We have a default seed of 42, so I think we can skip the getattr no?

claude likes to chuck these everywhere, but in my opinion that hurt the readablity.