Clean up asset initialization prim resolution#5887
Conversation
github-actions
Bot
added
enhancement
Include named predicates in expected match-count errors while omitting unhelpful lambda names.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Good refactoring PR that centralizes error handling logic into the query utilities instead of repeating it at each call site. The -124 net lines removed demonstrates effective code deduplication.
What I Like
- Cleaner call sites: The repetitive pattern of
resolve_matching_prims_from_source()→ check empty → raise error is now a single call withraise_if_no_matches=True(the default) - Consistent error messages: All sites now get the same error format from the centralized implementation
expected_num_matchesis well-designed: AcceptsNoneto disable (backwards compatible), validates non-negative input, and includes matched paths in error messages for debugging- Test coverage: New tests for both the happy path and error cases
Suggestions / Questions
1. Missing traverse_instance_prims=False in some refactored calls
Several of the original call sites explicitly passed traverse_instance_prims=False:
# Before (e.g., articulation.py):
root_prims = get_all_matching_child_prims(
walk_root, predicate=has_articulation_root_api, traverse_instance_prims=False
)
# After:
root_prims = get_all_matching_child_prims(walk_root, has_articulation_root_api, expected_num_matches=1)The refactored versions drop traverse_instance_prims=False. Since the default is True, this is a behavioral change. Was this intentional? If the original code explicitly disabled instance traversal for a reason, the refactored version may now traverse prims it shouldn't.
Affected files include:
isaaclab_physx/assets/articulation/articulation.pyisaaclab_physx/assets/rigid_object/rigid_object.pyisaaclab_physx/assets/rigid_object_collection/rigid_object_collection.pyisaaclab_physx/assets/deformable_object/deformable_object.pyisaaclab_newton/assets/articulation/articulation.pyisaaclab_newton/assets/rigid_object/rigid_object.pyisaaclab_newton/assets/rigid_object_collection/rigid_object_collection.pyisaaclab_newton/sensors/joint_wrench/joint_wrench_sensor.pyisaaclab_ovphysx/assets/articulation/articulation.pyisaaclab_ovphysx/assets/rigid_object/rigid_object.pyisaaclab_ovphysx/assets/rigid_object_collection/rigid_object_collection.py
2. Docstring simplification in resolve_matching_prims_from_source
The refactored docstring is much shorter, which is fine, but it lost some valuable context about:
- What "source instance" means (clone-plan source template vs env-0)
- How
env_regex_nsinteracts with clone plans - The fallback behavior when
path_exprdoesn't sit underenv_regex_ns
Consider preserving some of that detail for users unfamiliar with the clone-plan architecture.
3. Positional vs keyword argument style
Some calls now pass predicate positionally:
get_all_matching_child_prims(walk_root, has_articulation_root_api, expected_num_matches=1)While this works, using keyword arguments would be clearer given the function has 6 parameters:
get_all_matching_child_prims(walk_root, predicate=has_articulation_root_api, expected_num_matches=1)This is a style nit and not blocking.
4. Error message consistency
The new error from resolve_matching_prims_from_source:
No prim found at '{path_expr}'.
The new error from get_all_matching_child_prims:
Expected {expected_num_matches} prims under '{prim_path}', found {len(output_prims)}: {matched}.
The latter includes the matched paths which is great for debugging. Consider adding similar detail to the former when raise_if_no_matches triggers.
Minor
- Changelog entry looks good
- Test additions are appropriate
Overall this is a solid cleanup. The main concern is verifying the traverse_instance_prims behavioral change is intentional.
There was a problem hiding this comment.
Code Review Summary
Clean refactoring that consolidates repeated match-count validation into get_all_matching_child_prims (via expected_num_matches) and adds opt-out control for empty-result errors in resolve_matching_prims_from_source (via raise_if_no_matches). The pattern reduction across 12+ call sites is welcome.
However, there is one behavioral regression that needs attention before merging.
Findings
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🔴 Bug | Multiple assets/sensors | traverse_instance_prims=False silently dropped |
| 2 | 🟡 Suggestion | queries.py |
Grammar: "1 prims" → "1 prim" |
| 3 | 🟡 Suggestion | queries.py |
raise_if_no_matches — consider adding unit test |
| 4 | ℹ️ Note | queries.py |
Docstring simplification removes useful context for env_regex_ns |
Verdict
The refactoring goal is sound and the code reduction is significant. Finding #1 is a behavioral change that could cause issues in instanced USD scenes — please verify it's intentional or restore traverse_instance_prims=False at the affected call sites.
Update (commit 46cea13): Reviewed incremental changes since 4bebd6d.
The new commit refactors the error message formatting in queries.py (splits the predicate message into its own variable, introduces actual_num_matches local). This improves readability but does not address the previous findings:
- ❌
traverse_instance_prims=Falsestill dropped across all backend call sites (P1 concern) - ❌ Plural grammar ("Expected 1 prims") still present — the refactor did not add singular/plural handling
- ❌ No new tests for
raise_if_no_matches
No new issues introduced in this commit. The changelog .skip files are fine.
| f"Expected exactly one ArticulationRootAPI prim under '{walk_root}'" | ||
| f" (resolved from '{self.cfg.prim_path}'), found {len(root_prims)}: {matched}." | ||
| ) | ||
| root_prims = get_all_matching_child_prims(walk_root, has_articulation_root_api, expected_num_matches=1) |
There was a problem hiding this comment.
🔴 Bug: traverse_instance_prims=False was dropped
The old code explicitly passed traverse_instance_prims=False:
root_prims = get_all_matching_child_prims(
walk_root, predicate=has_articulation_root_api, traverse_instance_prims=False
)The new code omits it, defaulting to True:
root_prims = get_all_matching_child_prims(walk_root, has_articulation_root_api, expected_num_matches=1)This behavioral change affects 12 call sites across isaaclab_newton, isaaclab_ovphysx, and isaaclab_physx backends. In USD scenes with nested instance prims, traverse_instance_prims=True will descend into instances that the old code intentionally skipped, potentially finding duplicate API prims and triggering the new expected_num_matches validation.
If this change is intentional, please mention it in the PR description. Otherwise:
| root_prims = get_all_matching_child_prims(walk_root, has_articulation_root_api, expected_num_matches=1) | |
| root_prims = get_all_matching_child_prims(walk_root, has_articulation_root_api, expected_num_matches=1, traverse_instance_prims=False) |
| predicate_name = getattr(predicate, "__name__", None) | ||
| msg = "" if predicate_name is None or predicate_name == "<lambda>" else f" matching predicate '{predicate_name}'" | ||
| raise RuntimeError( | ||
| f"Expected {expected_num_matches} prims under '{prim_path}'{msg}," |
There was a problem hiding this comment.
🟡 Suggestion: Fix plural grammar for expected_num_matches=1
When expected_num_matches is 1, the error reads:
Expected 1 prims under '...'
Minor but easy to fix:
| f"Expected {expected_num_matches} prims under '{prim_path}'{msg}," | |
| f"Expected {expected_num_matches} {'prim' if expected_num_matches == 1 else 'prims'} under '{prim_path}'{msg}," |
|
|
||
| # test valid path | ||
| with pytest.raises(ValueError): | ||
| sim_utils.get_all_matching_child_prims("World/Room") |
There was a problem hiding this comment.
🟡 Suggestion: Add tests for resolve_matching_prims_from_source's new raise_if_no_matches parameter
The expected_num_matches parameter is well-tested here, but the new raise_if_no_matches parameter on resolve_matching_prims_from_source has no test coverage. Consider adding:
- Default behavior (
raise_if_no_matches=True) raisesRuntimeErrorfor non-existent path - Opt-out (
raise_if_no_matches=False) returns empty list for non-existent path
| predicate: Callable[[Usd.Prim], bool] | None = None, | ||
| env_regex_ns: str = "/World/envs/env_.*", | ||
| raise_if_no_matches: bool = True, | ||
| ) -> list[tuple[Usd.Prim, str]]: |
There was a problem hiding this comment.
ℹ️ Note: Docstring simplification removes implementation details for env_regex_ns
The old docstring explained how env_regex_ns interacts with clone-plan vs. legacy resolution, multi-wildcard fallback behavior, and when an explicit namespace is needed. The new docstring is terse:
Namespace pattern that marks one instance root when no clone plan applies.
Consider keeping a brief note about when callers should pass a custom env_regex_ns (e.g., layouts with more than one wildcard level) — it was useful guidance for non-standard scene topologies.
Greptile SummaryThis PR centralises repeated "no matches" and "wrong count" guards from a dozen asset/sensor initialisation methods into two new helper options:
Confidence Score: 3/5Mostly safe refactoring, but all physics-root-prim searches across three backends now traverse instance proxy prims that were previously excluded, which can cause spurious count-validation failures in scenes with USD-instanced assets. The All refactored Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Asset / Sensor _initialize_impl()"] --> B["resolve_matching_prims_from_source(prim_path)"]
B -- "raise_if_no_matches=True (default)" --> C{matches?}
C -- "no" --> D["RuntimeError: No prim found"]
C -- "yes" --> E["asset_prim, root_expr = matches[0]"]
E --> F["walk_root = asset_prim.GetPath().pathString"]
F --> G["get_all_matching_child_prims(walk_root, predicate, expected_num_matches=1)"]
G --> H{count == 1?}
H -- "no" --> I["RuntimeError: Expected N prims"]
H -- "yes" --> J["root_prims[0] → build root_prim_path_expr"]
J --> K["create physics view (ArticulationView / RigidBodyView / etc.)"]
subgraph "Opt-out paths (raise_if_no_matches=False)"
L["camera.py probe_path"] --> M["resolve... raise_if_no_matches=False"]
N["task_space_actions.py"] --> O["resolve... raise_if_no_matches=False → custom ValueError"]
P["frame_transformer.py"] --> Q["resolve... raise_if_no_matches=False → custom ValueError"]
end
Reviews (1): Last reviewed commit: "Improve prim match-count error message" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| f"Expected exactly one ArticulationRootAPI prim under '{walk_root}'" | ||
| f" (resolved from '{self.cfg.prim_path}'), found {len(root_prims)}: {matched}." | ||
| ) | ||
| root_prims = get_all_matching_child_prims(walk_root, has_articulation_root_api, expected_num_matches=1) |
There was a problem hiding this comment.
traverse_instance_prims behavior change — the original call sites across all three backends (physx, newton, ovphysx) and all asset types (Articulation, RigidObject, RigidObjectCollection, DeformableObject, JointWrenchSensor) explicitly set traverse_instance_prims=False. That argument is absent from every refactored call, so the traversal now defaults to True.
With True, instance proxy prims — synthetic read-only views that USD creates for every instance of a prototype — are included in the walk. If any prototype under walk_root has ArticulationRootAPI applied to it (or if the asset hierarchy contains instanced sub-trees), the traversal may now surface multiple matches, making the expected_num_matches=1 check fail with a RuntimeError in a scene that previously initialised correctly. The same applies to RigidBodyAPI, OmniPhysicsDeformableBodyAPI, etc. across the other refactored call sites in this PR. The PR description does not mention this as an intentional change.
| root_prims = get_all_matching_child_prims(walk_root, has_articulation_root_api, expected_num_matches=1) | |
| root_prims = get_all_matching_child_prims( | |
| walk_root, has_articulation_root_api, traverse_instance_prims=False, expected_num_matches=1 | |
| ) |
| if expected_num_matches is not None and len(output_prims) != expected_num_matches: | ||
| matched = [prim.GetPath().pathString for prim in output_prims] | ||
| predicate_name = getattr(predicate, "__name__", None) | ||
| msg = "" if predicate_name is None or predicate_name == "<lambda>" else f" matching predicate '{predicate_name}'" | ||
| raise RuntimeError( |
There was a problem hiding this comment.
expected_num_matches=1 the message reads "Expected 1 prims", which is ungrammatical. More importantly, the generic message no longer tells callers what kind of prim was expected (e.g. ArticulationRootAPI, RigidBodyAPI), making it significantly harder to diagnose the root cause without reading source code.
| if expected_num_matches is not None and len(output_prims) != expected_num_matches: | |
| matched = [prim.GetPath().pathString for prim in output_prims] | |
| predicate_name = getattr(predicate, "__name__", None) | |
| msg = "" if predicate_name is None or predicate_name == "<lambda>" else f" matching predicate '{predicate_name}'" | |
| raise RuntimeError( | |
| if expected_num_matches is not None and len(output_prims) != expected_num_matches: | |
| matched = [prim.GetPath().pathString for prim in output_prims] | |
| noun = "prim" if expected_num_matches == 1 else "prims" | |
| raise RuntimeError( | |
| f"Expected {expected_num_matches} {noun} under '{prim_path}', found {len(output_prims)}: {matched}." | |
| ) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
droping skip instanced is not a bug, the version before is the bug, not skipping is more accurate and correct, and the speed is justified because we are only walking within source instance @greptiles-app @isaaclab-review-bot |
|
That's a valid and well-reasoned point. Since Fair enough — the change is intentional and the reasoning is sound. Tip: You can customize Greptile's behavior for this repo with |
|
Thanks for the clarification, @ooctipus — that makes sense and I withdraw that finding. Looking at the code more carefully: The old Appreciate the context — the performance concern is moot since the traversal scope is already bounded. 👍 |
|
Thanks for the clarification @ooctipus! 👍 I understand now — the old I'll downgrade Finding #1 from 🔴 Bug to ℹ️ Note (intentional behavioral change). No action needed — just wanted to flag the difference in case it wasn't deliberate. The other findings (grammar fix, test coverage for |
2 participants
Description
Simplifies asset and sensor initialization code that resolves source prims and validates expected child prim matches.
This adds common query-helper options so call sites can avoid repeated
matcheschecks and manual match-count error handling, while preserving opt-out behavior for optional probes and custom error paths.Fixes # (issue)
Type of change
Screenshots
Not applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there