Refactor Relation Solver#681
Conversation
zhx06
requested review from
alexmillane,
cvolkcvolk,
peterd-NV,
qianl-nv,
viiik-inside and
xyao-nv
as code owners
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR cleanly extracts relation placement logic from ArenaEnvBuilder into dedicated solver abstractions. The architecture enables future robot/IK-aware validation while preserving existing behavior.
✅ CI Issues Fixed
Pre-commit was failing with two issues:
Black formatting —relation_placement.pyneeds reformattingFlake8 R501 — Lines 115 and 162 have explicitreturn None
✅ Fixed in cb6d966 — The unnecessary return None statements have been removed and pre-commit should now pass.
✅ Strengths
-
Good separation of concerns — The three-tier solver hierarchy (
ObjectRelationSolver→RobotRelationSolver→ArenaRelationSolver) provides clear extension points for future IK validation. -
Explicit error handling — Missing positions in layouts now raise
RuntimeErrorinstead of silently failing (lines 380-381, 395-396). -
Excellent test coverage — 477 lines of comprehensive tests covering normal flows, edge cases, and error conditions. The fake pooled object placers and mock solvers are well-designed.
-
Correct
__deepcopy__handling —ValidatedPlacementPoolpreserves object identity while copying solver state, which is essential forEventTermCfgdeep-copy semantics. -
Clean API surface — The
prepare_relation_placement()entry point provides a simple interface while allowing power users to inject custom solvers.
💭 Questions / Minor Items
-
One-time warning visibility (line 163-165):
The_warned_base_validation_skipflag ensures the "skipping IK validation" warning prints only once perRobotRelationSolverinstance. In long-running sessions with many resets, this could be logged early and missed. Consider whetherlogging.warningwith rate-limiting would be more appropriate for production visibility. -
Embodiment reconciliation flow (lines 287-321):
When neitherembodimentnorrobot_solveris supplied at construction, butembodimentis provided later viaprepare(), the internalrobot_solvergets updated correctly. However, the flow through_reconcile_robot_solver_embodimentis subtle—a brief docstring note about the intended lifecycle would help future maintainers. -
Suggested test addition: Consider adding a test that calls
RobotRelationSolver.validate_layouttwice with embodiment to verify the warning is suppressed on subsequent calls. This would exercise the_warned_base_validation_skipflag.
Verdict
Clean refactor with solid test coverage. The architecture is extensible for the planned IK validation. Ready to merge ✅
Update (808c1bf): ✅ Good fix — placement_events.py now uses obj.name as dictionary keys instead of object instances, which correctly handles deep-copied objects from EventTermCfg. The new test test_solve_and_place_objects_handles_copied_event_objects verifies this edge case. LGTM.
Update (a64cf76): Major simplification — the RobotRelationSolver class has been removed entirely, streamlining the architecture to focus on object-only relation solving. This resolves all three items from my "Questions / Minor Items" section (they're now N/A). The simplified PlacementSolution dataclass and ArenaRelationSolver are cleaner and easier to maintain. Tests updated accordingly. No new issues. Still LGTM ✅
Update (a5cd187): Import reorganization — moved ManagerBasedEnv and EventTermCfg imports into TYPE_CHECKING blocks with lazy runtime import where needed. This is a good practice to avoid potential circular import issues. No functional changes. Previous inline comments about validation scope and check_objects_valid remain open for author consideration.
Greptile SummaryThis PR extracts relation-placement logic from
Confidence Score: 4/5Safe to merge for greenfield callers; existing callers using resolve_on_reset=False with non-anchor objects that have event_cfg set will now hit a RuntimeError that was previously silent. The refactor is well-structured and the new test suite is thorough. The main concern is that _validate_no_explicit_non_anchor_pose_events is now called unconditionally in prepare(), where the old code only invoked it under the resolve_on_reset=True branch — any caller that relied on the silent pass-through for resolve_on_reset=False will break at runtime without a config change. isaaclab_arena/relations/relation_placement.py — specifically the unconditional _validate_no_explicit_non_anchor_pose_events call in prepare() and the double-pass of embodiment through both the ArenaRelationSolver constructor and prepare(). Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class ObjectRelationSolver {
+num_envs: int
+placement_seed: int|None
+resolve_on_reset: bool|None
+solve(objects) ObjectRelationSolveResult
+validate_object_placement_pool(pool) None
+validate_layout(layout) None
+check_objects_valid(layout) bool
}
class ValidatedPlacementPool {
-_placement_pool: PooledObjectPlacer
-_object_solver: ObjectRelationSolver
+sample_without_replacement(count) list
+sample_with_replacement(count) list
+remaining: int
+pool_size: int
+__deepcopy__(memo) ValidatedPlacementPool
}
class ArenaRelationSolver {
+num_envs: int
+embodiment: EmbodimentBase|None
+object_solver: ObjectRelationSolver
+objects: list[ObjectBase]
+prepare(objects, embodiment) PlacementSolution|None
+solve_relations() ObjectRelationSolveResult
+validate_layout(layout) None
}
class PlacementSolution {
+placement_event_cfg: EventTermCfg|None
}
class ArenaEnvBuilder {
+_solve_relations()
}
ArenaRelationSolver --> ObjectRelationSolver : owns
ArenaRelationSolver --> ValidatedPlacementPool : creates
ArenaRelationSolver --> PlacementSolution : returns
ValidatedPlacementPool --> ObjectRelationSolver : delegates validate_layout
ArenaEnvBuilder ..> ArenaRelationSolver : via prepare_relation_placement()
Reviews (6): Last reviewed commit: "fix simapp loading" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| relation_result = self.solve_relations() | ||
| anchor_objects_set = set(get_anchor_objects(self.objects)) | ||
| _validate_no_explicit_non_anchor_pose_events(self.objects, anchor_objects_set) |
There was a problem hiding this comment.
resolve_on_reset=False callers
_validate_no_explicit_non_anchor_pose_events is now called unconditionally inside prepare(), but in the old code it was only called inside the if placer_params.resolve_on_reset: branch. Any existing caller using resolve_on_reset=False with a non-anchor object that has an event_cfg will now receive a RuntimeError where the old code silently allowed it. The PR states "Preserve current behavior," but this is a breaking behavioral change that contradicts that goal. Is expanding the validation scope to cover resolve_on_reset=False intentional? If so, the PR description should note this as an explicit behavior tightening rather than pure refactoring.
| placement_solver = solver or ArenaRelationSolver( | ||
| num_envs=num_envs, | ||
| placement_seed=placement_seed, | ||
| resolve_on_reset=resolve_on_reset, | ||
| embodiment=embodiment, | ||
| ) | ||
| return placement_solver.prepare(objects, embodiment=embodiment) |
There was a problem hiding this comment.
prepare_relation_placement passes embodiment to the ArenaRelationSolver constructor and then passes the same value again via prepare(embodiment=embodiment). The second call is redundant when solver is None: the constructor already wires the embodiment into self.robot_solver and self.embodiment, so _set_embodiment in prepare() is a no-op for an internally-created solver. Consider passing embodiment=None to prepare() in that path (or documenting why the double-pass is intentional).
| placement_solver = solver or ArenaRelationSolver( | |
| num_envs=num_envs, | |
| placement_seed=placement_seed, | |
| resolve_on_reset=resolve_on_reset, | |
| embodiment=embodiment, | |
| ) | |
| return placement_solver.prepare(objects, embodiment=embodiment) | |
| if solver is None: | |
| placement_solver = ArenaRelationSolver( | |
| num_envs=num_envs, | |
| placement_seed=placement_seed, | |
| resolve_on_reset=resolve_on_reset, | |
| embodiment=embodiment, | |
| ) | |
| return placement_solver.prepare(objects) | |
| return placement_solver.prepare(objects, embodiment=embodiment) |
| def _reconcile_robot_solver_embodiment(self) -> None: | ||
| if not self._robot_solver_supplied and self.robot_solver.embodiment is None: | ||
| self.robot_solver.embodiment = self.embodiment | ||
| return | ||
| if self.robot_solver.embodiment is not self.embodiment: | ||
| raise ValueError("embodiment must match robot_solver.embodiment when both are provided.") |
There was a problem hiding this comment.
_reconcile_robot_solver_embodiment raises a confusing ValueError when an ArenaRelationSolver that was created without an explicit robot_solver is reused across multiple prepare() calls with different non-None embodiments. The first prepare(embodiment=emb_a) call passes the robot_solver.embodiment is None guard and sets the default solver's embodiment to emb_a. A subsequent prepare(embodiment=emb_b) call finds robot_solver.embodiment is emb_a (not None), skips the first branch entirely, and then the second check robot_solver.embodiment is not self.embodiment fires, raising ValueError: "embodiment must match robot_solver.embodiment when both are provided" — a message that references a robot_solver the user never explicitly provided. Removing the is None guard makes the default solver always mirror self.embodiment.
| def _reconcile_robot_solver_embodiment(self) -> None: | |
| if not self._robot_solver_supplied and self.robot_solver.embodiment is None: | |
| self.robot_solver.embodiment = self.embodiment | |
| return | |
| if self.robot_solver.embodiment is not self.embodiment: | |
| raise ValueError("embodiment must match robot_solver.embodiment when both are provided.") | |
| def _reconcile_robot_solver_embodiment(self) -> None: | |
| if not self._robot_solver_supplied: | |
| self.robot_solver.embodiment = self.embodiment | |
| return | |
| if self.robot_solver.embodiment is not self.embodiment: | |
| raise ValueError("embodiment must match robot_solver.embodiment when both are provided.") |
| return layout.success | ||
|
|
||
|
|
||
| class RobotRelationSolver: |
There was a problem hiding this comment.
Let's take out this class from this MR. This MR is meant to be refactoring only. We can start the class design in a doc with high level class diagram first.
| def validate_layout(self, layout: PlacementResult) -> None: | ||
| """Validate one object-relation placement result.""" | ||
|
|
||
| def check_objects_valid(self, layout: PlacementResult) -> bool: | ||
| return layout.success |
There was a problem hiding this comment.
check_objects_valid is defined but never called anywhere in the codebase. ValidatedPlacementPool._validate_layouts exclusively calls validate_layout on the solver. A subclass author who overrides check_objects_valid expecting it to run during sampling will get no error and no effect — a silent no-op that is hard to diagnose. Either wire it into validate_layout's default implementation, or remove it to keep the extension API unambiguous.
| def validate_layout(self, layout: PlacementResult) -> None: | |
| """Validate one object-relation placement result.""" | |
| def check_objects_valid(self, layout: PlacementResult) -> bool: | |
| return layout.success | |
| def validate_layout(self, layout: PlacementResult) -> None: | |
| """Validate one object-relation placement result.""" | |
| if not self.check_objects_valid(layout): | |
| raise RuntimeError("Placement layout failed object validation.") | |
| def check_objects_valid(self, layout: PlacementResult) -> bool: | |
| return layout.success |
| credentials: | ||
| username: $oauthtoken | ||
| password: ${{ env.NGC_API_KEY }} | ||
| password: ${{ secrets.ARENA_NGC_API_KEY }} |
|
Close it as #707 addressed the same request |
2 participants
Summary
Refactor Relation Solver
Detailed description