Replaces nucleus_utils with reimplementation as part of IsaacLab
#3921
+88
−18
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
enhancement
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR removes the external dependency on isaacsim.storage.native / isaacsim.core.utils.nucleus by implementing a new isaaclab.sim.utils.nucleus module internally. The implementation provides the same get_assets_root_path() API used by test scripts to locate Isaac Sim assets on Nucleus servers.
Major changes:
- Created new
source/isaaclab/isaaclab/sim/utils/nucleus.pywithcheck_server()andget_assets_root_path()functions - Moved
source/isaaclab/isaaclab/sim/utils.pytosource/isaaclab/isaaclab/sim/utils/utils.pyto create a proper utils submodule - Updated 4 test scripts to use
import isaaclab.sim.utils.nucleus as nucleus_utilsinstead of the old try/except import pattern
Issues found:
- Critical bug: The
timeoutparameter is retrieved from carb settings but never passed tocheck_server()calls on lines 70 and 72, making the timeout configuration ineffective
Confidence Score: 2/5
- This PR has a critical logic bug that breaks timeout functionality
- Score reflects a critical bug where the timeout parameter is retrieved from settings but never used in check_server calls (lines 70, 72), making timeout configuration non-functional. The dependency removal itself is clean and well-structured, but the incomplete timeout implementation is a blocker
source/isaaclab/isaaclab/sim/utils/nucleus.pyrequires immediate attention to fix timeout parameter usage
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/sim/utils/nucleus.py | 2/5 | new nucleus utilities module added with timeout parameter bug - retrieved but never used in check_server calls |
| source/isaaclab/isaaclab/sim/utils/init.py | 5/5 | module initialization file created - exports all utils functions |
| source/isaaclab/isaaclab/sim/utils/utils.py | 5/5 | moved from utils.py to utils/utils.py - import path updated for schemas |
Sequence Diagram
sequenceDiagram
participant Test as Test Scripts
participant NucleusUtils as isaaclab.sim.utils.nucleus
participant Carb as carb.settings
participant OmniClient as omni.client
participant NucleusServer as Nucleus Server
Test->>NucleusUtils: get_assets_root_path(skip_check=False)
NucleusUtils->>Carb: get(DEFAULT_ASSET_ROOT_TIMEOUT_SETTING)
Carb-->>NucleusUtils: timeout value or None
NucleusUtils->>Carb: get(DEFAULT_ASSET_ROOT_PATH_SETTING)
Carb-->>NucleusUtils: default_asset_root path
alt skip_check is True
NucleusUtils-->>Test: return default_asset_root
else skip_check is False
NucleusUtils->>NucleusUtils: check_server(default_asset_root, "/Isaac", timeout)
NucleusUtils->>OmniClient: set_hang_detection_time_ms(20000)
NucleusUtils->>OmniClient: stat(server + path)
OmniClient->>NucleusServer: check path exists
NucleusServer-->>OmniClient: Result.OK or error
OmniClient-->>NucleusUtils: result
alt Isaac path found
NucleusUtils->>NucleusUtils: check_server(default_asset_root, "/NVIDIA", timeout)
NucleusUtils->>OmniClient: set_hang_detection_time_ms(20000)
NucleusUtils->>OmniClient: stat(server + path)
OmniClient->>NucleusServer: check path exists
NucleusServer-->>OmniClient: Result.OK or error
OmniClient-->>NucleusUtils: result
alt NVIDIA path found
NucleusUtils-->>Test: return default_asset_root
else NVIDIA path not found
NucleusUtils-->>Test: raise RuntimeError
end
else Isaac path not found
NucleusUtils-->>Test: raise RuntimeError
end
end
7 files reviewed, 3 comments
| return default_asset_root | ||
|
|
||
| # check path | ||
| result = check_server(default_asset_root, "/Isaac", timeout) |
Contributor
There was a problem hiding this comment.
logic: timeout parameter retrieved on line 57 is never passed to check_server calls
Suggested change
| result = check_server(default_asset_root, "/Isaac", timeout) | |
| result = check_server(default_asset_root, "/Isaac", timeout) |
| # check path | ||
| result = check_server(default_asset_root, "/Isaac", timeout) | ||
| if result: | ||
| result = check_server(default_asset_root, "/NVIDIA", timeout) |
Contributor
There was a problem hiding this comment.
logic: timeout parameter is not passed to this check_server call either
Suggested change
| result = check_server(default_asset_root, "/NVIDIA", timeout) | |
| result = check_server(default_asset_root, "/NVIDIA", timeout) |
| import omni.client | ||
| from omni.client import Result | ||
|
|
||
| # import logger |
Contributor
There was a problem hiding this comment.
style: remove commented-out code
Suggested change
| # import logger |
Mayankm96
reviewed
Nov 3, 2025
| from omni.client import Result | ||
|
|
||
| # import logger | ||
| logger = logging.getLogger(__name__) |
Collaborator
There was a problem hiding this comment.
seems like this will be using the refactored #3912 logging style
ooctipus
approved these changes
Nov 5, 2025
Collaborator
ooctipus
left a comment
ooctipus
left a comment
There was a problem hiding this comment.
Looks good to me and tests are also passing, remember to remove the commended out import logger greptile suggested!
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR removes the external dependency on isaacsim.storage.native or isaacsim.core.utils.nucleus by implementing a minimal nucleus utilities module within IsaacLab at isaaclab.sim.utils.nucleus. The implementation provides the get_assets_root_path() function used by test scripts.
Key Changes:
- New
nucleus.pymodule withcheck_server()andget_assets_root_path()functions - Updated all test imports to use
isaaclab.sim.utils.nucleus - Removed fallback try/except import logic from test files
Critical Issues Found:
- The
timeoutparameter retrieved on nucleus.py:57 is never passed tocheck_server()calls on lines 70 and 72 - The
timeoutparameter incheck_server()function is defined but never used - only hardcoded 20000ms is set - Commented-out code on line 12:
# import logger
Confidence Score: 2/5
- This PR has critical logical bugs in the timeout handling that need to be fixed before merging
- The timeout parameter is retrieved from settings but never actually used, making the implementation incomplete. The bugs are in the core new functionality being added.
- Pay close attention to source/isaaclab/isaaclab/sim/utils/nucleus.py - it has critical timeout handling bugs
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/sim/utils/nucleus.py | 2/5 | New nucleus utils implementation with timeout parameter bugs (lines 70, 72) and unused timeout in check_server |
Sequence Diagram
sequenceDiagram
participant Test as Test Scripts
participant Nucleus as isaaclab.sim.utils.nucleus
participant Carb as carb.settings
participant OmniClient as omni.client
Test->>Nucleus: get_assets_root_path()
Nucleus->>Carb: get(DEFAULT_ASSET_ROOT_TIMEOUT_SETTING)
Carb-->>Nucleus: timeout value (or None)
Nucleus->>Carb: get(DEFAULT_ASSET_ROOT_PATH_SETTING)
Carb-->>Nucleus: default_asset_root
alt skip_check is True
Nucleus-->>Test: return default_asset_root
else skip_check is False
Nucleus->>Nucleus: check_server(default_asset_root, "/Isaac", timeout)
Note over Nucleus,OmniClient: BUG: timeout not passed to check_server
Nucleus->>OmniClient: set_hang_detection_time_ms(20000)
Nucleus->>OmniClient: stat(server + path)
OmniClient-->>Nucleus: result
alt Isaac path found
Nucleus->>Nucleus: check_server(default_asset_root, "/NVIDIA", timeout)
Note over Nucleus,OmniClient: BUG: timeout not passed here either
Nucleus->>OmniClient: set_hang_detection_time_ms(20000)
Nucleus->>OmniClient: stat(server + path)
OmniClient-->>Nucleus: result
alt NVIDIA path found
Nucleus-->>Test: return default_asset_root
else NVIDIA path not found
Nucleus-->>Test: raise RuntimeError
end
else Isaac path not found
Nucleus-->>Test: raise RuntimeError
end
end
7 files reviewed, 1 comment
| """ | ||
| logger.info(f"Checking path: {server}{path}") | ||
| # Increase hang detection timeout | ||
| omni.client.set_hang_detection_time_ms(20000) |
Contributor
There was a problem hiding this comment.
logic: timeout parameter accepted by function is never used - only hardcoded 20000ms is set. Should use timeout * 1000 to convert seconds to milliseconds
Suggested change
| omni.client.set_hang_detection_time_ms(20000) | |
| omni.client.set_hang_detection_time_ms(int(timeout * 1000)) |
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Replaces external nucleus_utils dependency with internal reimplementation in isaaclab.sim.utils.nucleus. This removes the dependency on isaacsim.storage.native and isaacsim.core.utils.nucleus modules.
Key changes:
- New
nucleus.pymodule providesget_assets_root_path()andcheck_server()functions - All test files updated to import from new location:
import isaaclab.sim.utils.nucleus as nucleus_utils - Functionality mirrors original implementation for checking Nucleus server paths
Critical issues found:
- The
timeoutparameter is retrieved from settings but never passed tocheck_server()calls (lines 69, 71) - The
check_server()function acceptstimeoutparameter but uses hardcoded 20000ms instead (line 31) - These bugs mean configured timeout values are completely ignored, potentially causing unexpected hang behavior
Confidence Score: 2/5
- This PR has critical timeout handling bugs that must be fixed before merging
- Score reflects three interconnected logic bugs where the timeout parameter is retrieved but never used, causing the function to ignore user-configured timeout settings. While the dependency replacement approach is sound, the implementation has runtime-affecting bugs that need immediate correction.
- source/isaaclab/isaaclab/sim/utils/nucleus.py requires immediate attention - all three timeout-related bugs must be fixed
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/sim/utils/nucleus.py | 2/5 | New nucleus utils reimplementation with critical timeout handling bugs - timeout parameter retrieved but never passed to check_server calls and hardcoded value used instead |
Sequence Diagram
sequenceDiagram
participant Client as Test/Application
participant NucleusUtils as nucleus_utils
participant CarbSettings as carb.settings
participant OmniClient as omni.client
Client->>NucleusUtils: get_assets_root_path(skip_check=False)
NucleusUtils->>CarbSettings: get(DEFAULT_ASSET_ROOT_TIMEOUT_SETTING)
CarbSettings-->>NucleusUtils: timeout value (or None)
Note over NucleusUtils: Default to 10.0 if not set
NucleusUtils->>CarbSettings: get(DEFAULT_ASSET_ROOT_PATH_SETTING)
CarbSettings-->>NucleusUtils: default_asset_root
alt skip_check is False
NucleusUtils->>NucleusUtils: check_server(default_asset_root, "/Isaac", timeout)
Note over NucleusUtils,OmniClient: BUG: timeout not passed to check_server
NucleusUtils->>OmniClient: set_hang_detection_time_ms(20000)
Note over OmniClient: BUG: hardcoded 20000ms instead of timeout*1000
NucleusUtils->>OmniClient: stat(server + path)
OmniClient-->>NucleusUtils: result
alt result == OK
NucleusUtils->>NucleusUtils: check_server(default_asset_root, "/NVIDIA", timeout)
Note over NucleusUtils,OmniClient: BUG: timeout not passed here either
NucleusUtils->>OmniClient: set_hang_detection_time_ms(20000)
NucleusUtils->>OmniClient: stat(server + path)
OmniClient-->>NucleusUtils: result
alt result == OK
NucleusUtils-->>Client: default_asset_root
else
NucleusUtils-->>Client: RuntimeError
end
else
NucleusUtils-->>Client: RuntimeError
end
else skip_check is True
NucleusUtils-->>Client: default_asset_root
end
1 file reviewed, no comments
ooctipus
force-pushed
the
feature/replace-nucleus-utils
branch
from
f237a94 to
6cfe857
Compare
Contributor
Greptile OverviewGreptile SummaryThis PR replaces the external Isaac Sim Major Changes:
Critical Issues:
Positive Aspects:
Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Test as Test Files
participant NucleusUtils as isaaclab.sim.utils.nucleus
participant CarbSettings as carb.settings
participant OmniClient as omni.client
participant NucleusServer as Nucleus Server
Test->>NucleusUtils: get_assets_root_path()
NucleusUtils->>CarbSettings: get timeout setting
CarbSettings-->>NucleusUtils: timeout value
NucleusUtils->>CarbSettings: get default asset root path
CarbSettings-->>NucleusUtils: asset root path
alt skip_check is False
NucleusUtils->>NucleusUtils: check_server(path, "/Isaac", timeout)
NucleusUtils->>OmniClient: set_hang_detection_time_ms(20000)
Note over NucleusUtils,OmniClient: BUG: timeout param ignored, hardcoded 20000
NucleusUtils->>OmniClient: stat(server + path)
OmniClient->>NucleusServer: check path exists
NucleusServer-->>OmniClient: Result
OmniClient-->>NucleusUtils: result status
alt /Isaac check passes
NucleusUtils->>NucleusUtils: check_server(path, "/NVIDIA", timeout)
Note over NucleusUtils,OmniClient: BUG: timeout param not passed to check_server
NucleusUtils->>OmniClient: stat(server + path)
OmniClient->>NucleusServer: check path exists
NucleusServer-->>OmniClient: Result
OmniClient-->>NucleusUtils: result status
end
end
NucleusUtils-->>Test: asset root path or RuntimeError
|
ooctipus
force-pushed
the
feature/replace-nucleus-utils
branch
from
6cfe857 to
c664781
Compare
Collaborator
|
pytest source/isaaclab_rl/test/test_rl_games_wrapper.py did not TIMEOUT locally |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Replaces import of
with own utils implemented inside the sim utils folder.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there