-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Replace torch_utils set_seed method with IsaacLab implementation
#3920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Replace torch_utils set_seed method with IsaacLab implementation
#3920
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR successfully removes the dependency on isaacsim.core.utils.torch.set_seed by implementing an equivalent set_seed function directly in IsaacLab. The implementation handles seeding for all major libraries (torch, numpy, random, warp) and includes proper deterministic mode support.
Key changes:
- New
source/isaaclab/isaaclab/utils/seed.pymodule added withset_seedfunction - Updated imports in
manager_based_env.py,direct_rl_env.py,direct_marl_env.py, and test files - Function signature and behavior maintained for backward compatibility
Minor issues:
- Docstring could be more detailed with proper parameter descriptions
- Documentation in
docs/source/features/reproducibility.rst:13still references:meth:~isaacsim.core.utils.torch.set_seed`` and should be updated to point to the new location
Confidence Score: 4/5
- This PR is safe to merge with minimal risk - it's a straightforward dependency replacement with maintained functionality
- Score of 4/5 reflects a well-executed dependency removal with proper implementation. The new
set_seedfunction correctly handles all necessary libraries (torch, numpy, random, warp) and maintains deterministic mode support. Only minor style improvements needed (docstring formatting) and one documentation reference should be updated. No logical errors or breaking changes detected. - No files require special attention - the implementation is straightforward and correct
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/utils/seed.py | 4/5 | New seed utility module added to replace Isaac Sim dependency - implementation is solid with proper seeding of all major libraries (torch, numpy, random, warp) |
Sequence Diagram
sequenceDiagram
participant User
participant Env as Environment (ManagerBasedEnv/DirectRLEnv)
participant SeedModule as isaaclab.utils.seed
participant Replicator as omni.replicator.core
participant Libraries as torch/numpy/random/warp
User->>Env: Initialize environment with cfg
Env->>Env: __init__(cfg)
alt cfg.seed is not None
Env->>Env: seed(cfg.seed)
Env->>Replicator: set_global_seed(seed)
Note over Replicator: Set seed for omni replicator<br/>(if available)
Env->>SeedModule: set_seed(seed)
alt seed == -1 and not torch_deterministic
SeedModule->>SeedModule: Generate random seed<br/>np.random.randint(0, 10000)
else seed == -1 and torch_deterministic
SeedModule->>SeedModule: Use seed = 42
end
SeedModule->>SeedModule: Print seed value
SeedModule->>Libraries: Set seeds:<br/>- random.seed(seed)<br/>- np.random.seed(seed)<br/>- torch.manual_seed(seed)<br/>- torch.cuda.manual_seed_all(seed)<br/>- wp.rand_init(seed)
SeedModule->>SeedModule: Set PYTHONHASHSEED env var
alt torch_deterministic == True
SeedModule->>Libraries: Enable deterministic mode:<br/>- CUBLAS_WORKSPACE_CONFIG<br/>- cudnn.deterministic = True<br/>- use_deterministic_algorithms(True)
else
SeedModule->>Libraries: Performance mode:<br/>- cudnn.benchmark = True
end
SeedModule-->>Env: Return seed value
Env->>Env: Update cfg.seed
else cfg.seed is None
Env->>Env: Log warning about non-deterministic creation
end
1 file reviewed, 1 comment
|
|
||
|
|
||
| def set_seed(seed: int, torch_deterministic: bool = False) -> int: | ||
| """set seed across modules""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: docstring should follow PEP 257 style and include parameter descriptions
| """set seed across modules""" | |
| """Set seed across all random number generators (torch, numpy, random, warp). | |
| Args: | |
| seed: The random seed value. If -1, generates a random seed. | |
| torch_deterministic: If True, enables deterministic mode for torch operations. | |
| Returns: | |
| The seed value that was set. | |
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should seed also support None type which means it becomes "-1" behavior? I feel it is more inutitive than -1. Also are negative number generators valid? It seems ti is a yes for random and numpy at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right on both counts! Supporting None would be more intuitive than using -1 for random seed generation, and negative numbers are indeed valid seeds for the underlying generators.
Looking at the current implementation, using -1 as a special value to trigger random seed generation isn't very pythonic. The more common pattern is to use None for this purpose, which is what many libraries like numpy.random.seed() and random.seed() expect.
Regarding negative seeds - yes, they are valid:
random.seed()accepts any hashable object, including negative integersnumpy.random.seed()accepts negative integerstorch.manual_seed()accepts negative integerswarp.rand_init()should also handle negative seeds
Here's how I'd suggest modifying the function:
| """set seed across modules""" | |
| def set_seed(seed: int | None, torch_deterministic: bool = False) -> int: | |
| """Set seed across all random number generators (torch, numpy, random, warp). | |
| Args: | |
| seed: The random seed value. If None, generates a random seed. | |
| torch_deterministic: If True, enables deterministic mode for torch operations. | |
| Returns: | |
| The seed value that was set. | |
| """ | |
| if seed is None and torch_deterministic: | |
| seed = 42 | |
| elif seed is None: | |
| seed = np.random.randint(0, 10000) | |
| print(f"Setting seed: {seed}") | |
| random.seed(seed) | |
| np.random.seed(seed) | |
| torch.manual_seed(seed) | |
| os.environ["PYTHONHASHSEED"] = str(seed) | |
| torch.cuda.manual_seed(seed) | |
| torch.cuda.manual_seed_all(seed) | |
| wp.rand_init(seed) | |
| if torch_deterministic: | |
| # refer to https://docs.nvidia.com/cuda/cublas/index.html#cublasApi_reproducibility | |
| os.environ["CUBLAS_WORKSPACE_CONFIG"] = ":4096:8" | |
| torch.backends.cudnn.benchmark = False | |
| torch.backends.cudnn.deterministic = True | |
| torch.use_deterministic_algorithms(True) | |
| else: | |
| torch.backends.cudnn.benchmark = True | |
| torch.backends.cudnn.deterministic = False | |
| return seed |
This change makes the API more intuitive and follows Python conventions. You'll also need to update any calls to this function that currently pass -1 to pass None instead.
| import warp as wp | ||
|
|
||
|
|
||
| def set_seed(seed: int, torch_deterministic: bool = False) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: To me a "set" usually should be combined with "get". Do you want to add something like "get_seed" as well? Or maybe we call this function configure_seed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I would rename it! Only effect is a bigger break as also the name of the function changes
Co-authored-by: Mayank Mittal <[email protected]> Signed-off-by: Pascal Roth <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Replaces the external isaacsim.core.utils.torch.set_seed dependency with a new IsaacLab-internal implementation in isaaclab.utils.seed.
Key changes:
- Creates new
set_seed()function that sets seeds across all RNG modules (torch, numpy, random, warp) - Updates 3 environment classes (
DirectRLEnv,DirectMARLEnv,ManagerBasedEnv) to use the new implementation - Updates test file to import from new location
- Removes dependency on
isaacsim.core.utils.torchmodule - Removed debug print statement from initial implementation
- Refactored conditional logic from if-elif to ternary operator
Minor issue found:
- Extra whitespace in ternary operator on line 17 (syntax)
Confidence Score: 4/5
- Safe to merge after fixing the minor whitespace issue
- The implementation correctly replicates the functionality of the external dependency it replaces. The logic for seeding all random number generators is sound, and the torch deterministic mode configuration is properly implemented. Only minor whitespace formatting issue found on line 17.
- source/isaaclab/isaaclab/utils/seed.py - fix double space on line 17
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/utils/seed.py | 4/5 | New file implementing set_seed function to replace isaacsim.core.utils.torch dependency; minor whitespace issue on line 17 |
Sequence Diagram
sequenceDiagram
participant Env as Environment Classes
participant Seed as set_seed()
participant RNG as Random Number Generators
participant Torch as PyTorch Backend
Env->>Seed: set_seed(seed, torch_deterministic=False)
alt seed == -1
Seed->>Seed: Generate random seed (42 if deterministic, else random)
end
Seed->>RNG: random.seed(seed)
Seed->>RNG: np.random.seed(seed)
Seed->>RNG: torch.manual_seed(seed)
Seed->>RNG: torch.cuda.manual_seed(seed)
Seed->>RNG: torch.cuda.manual_seed_all(seed)
Seed->>RNG: wp.rand_init(seed)
Seed->>Seed: Set PYTHONHASHSEED env var
alt torch_deterministic == True
Seed->>Torch: Set CUBLAS_WORKSPACE_CONFIG
Seed->>Torch: Disable cudnn.benchmark
Seed->>Torch: Enable cudnn.deterministic
Seed->>Torch: Enable use_deterministic_algorithms
else torch_deterministic == False
Seed->>Torch: Enable cudnn.benchmark
Seed->>Torch: Disable cudnn.deterministic
end
Seed-->>Env: Return seed value
1 file reviewed, 1 comment
| def set_seed(seed: int, torch_deterministic: bool = False) -> int: | ||
| """set seed across modules""" | ||
| if seed == -1: | ||
| seed = 42 if torch_deterministic else np.random.randint(0, 10000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: double space after if
| seed = 42 if torch_deterministic else np.random.randint(0, 10000) | |
| seed = 42 if torch_deterministic else np.random.randint(0, 10000) |
Description
Replace torch_utils
set_seedmethod with IsaacLab implementationType of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there