Refactor relation placement solver API

[xyao-nv]

Summary

Short description of the change (max 50 chars)

Detailed description

• Introduces ArenaRelationSolver.solve() as the high-level API: collect related objects, build PlacementProblem, solve object-only placement, select from the pool, then apply the result.
• Keeps ArenaEnvBuilder as the only owner of relation placement setup and removes the standalone preparation helper.
• Renames pooled placement to PlacementCandidatePool and leaves joint object/embodiment optimization as future TODO.

[isaaclab-review-bot]

🔍 Isaac Lab Review Bot — PR #707

Refactor relation placement solver API by @xyao-nv

---

📐 Architecture/Design

Strengths:

• ✅ Clean separation of concerns with ArenaRelationSolver → ObjectRelationSolver → PlacementCandidatePool hierarchy
• ✅ Well-defined dataclasses (PlacementProblem, ObjectRelationSolveResult, ArenaRelationSolveResult) provide clear API contracts
• ✅ Extensibility via ObjectRelationSolver hooks (validate_layout, validate_placement_candidate_pool) for future customization
• ✅ ArenaEnvBuilder is now significantly cleaner — from 106 deleted lines to 11 added, with logic delegated appropriately
• ✅ Renaming PooledObjectPlacer → PlacementCandidatePool better describes the component's purpose
• ✅ TODO comment (line 154) clearly notes future work for joint object/embodiment placement

Suggestions:

• 💡 Consider adding a docstring to ArenaRelationSolveResult.placement_event_cfg explaining when it's None (e.g., when all objects are anchors or resolve_on_reset=False)

---

🔇 Silent Failure Analysis

Good error handling found:

• ✅ Line 194-196: RuntimeError when placement candidate missing position for object
• ✅ Line 198-199: RuntimeError when object_cfg is missing
• ✅ Line 218-220: Consistent error handling in static pose path
• ✅ Line 233-238: Explicit validation of conflicting pose-reset events

Potential concerns:

• ⚠️ PlacementCandidatePool._solve_and_store (lines 83-95): When no candidates pass strict validation, a warning is printed and fallback candidates are used. While this matches pre-pool behavior, downstream code may not expect success=False results. Consider:

  • Adding a flag to PlacementCandidatePool to control this fallback behavior
  • Or at minimum, documenting this behavior in the class docstring

• ⚠️ candidate_validator exceptions: If the validator callback raises, exceptions propagate through sample_* methods. This is fine but should be documented in the candidate_validator parameter docstring.

---

🧪 Test Coverage

Added/Updated:

• ✅ New test_relation_placement.py with unit tests for solver API
• ✅ All test_placement_events.py tests updated for renamed classes

Coverage gaps to consider for follow-up:

• ❓ No test for resolve_on_reset=True (dynamic spawn pose) path — test_static_arena_relation_solver_reuses_object_only_placement only covers resolve_on_reset=False
• ❓ No test for anchor-only edge case (when anchor_objects_set == set(problem.objects), line 172)
• ❓ No test exercising ObjectRelationSolver validation hooks
• ❓ No test for error case when object_cfg is None

---

📝 Minor Notes

1. Type consistency: ObjectBase is used in new code but Object | ObjectReference was used in old code — good modernization to the base type
2. Import cleanup: Removed unused imports from arena_env_builder.py ✓
3. CI Status: Pre-commit check is still pending

---

Summary

This is a well-structured refactor that:

1. Introduces a clean, testable API for relation placement
2. Significantly simplifies ArenaEnvBuilder
3. Sets up extensibility for future embodiment placement work

The main suggestions are around documenting edge-case behaviors and adding test coverage for the resolve_on_reset=True path.

---

🤖 Automated review by Isaac Lab Review Bot • Feedback

---

Update (637d4b8): Good follow-up simplifications:

• ✅ Removed ObjectRelationSolveResult wrapper — ObjectRelationSolver.solve() now returns PlacementCandidatePool directly
• ✅ Added pool_size property to PlacementProblem — cleaner encapsulation
• ✅ Removed unused check_objects_valid method and _solve_object_only_placement indirection
• ✅ Added test assertions for pool_size and resolve_on_reset

API is now more streamlined. Previous concerns about documentation and test coverage for resolve_on_reset=True path still apply but are minor. LGTM.

---

Update (5587fc2): Minor readability improvement:

• ✅ Extracted _apply_object_placement_result() helper method with clear docstring
• ✅ Replaced awkward if/pass/elif/else chain with clean early returns
• No new issues. Ready to merge. ✅---

Update (a3ba693): This commit introduces significant new features beyond the original relation placement refactor:

🆕 New Features

1. merge_demos.py script — A comprehensive tool for merging HDF5 teleop demo files:

• ✅ Well-designed validation pipeline with _FileInfo, _ValidationReport dataclasses
• ✅ Excellent error handling: format_version/schema/env_args compatibility checks
• ✅ Atomic writes via .tmp + os.replace() pattern
• ✅ Thorough test coverage in test_merge_demos.py (470 lines of tests!)
• ✅ Documentation added in step_2_teleoperation.rst files with cross-references

2. record_demos.py enhancements:

• ✅ New --disable_full_sim_buffer_reset flag for performance tuning
• ✅ Guard against manual reset during post-success recording

3. G1AgilePolicy dynamic batch support:

• ✅ Removed num_envs == 1 restriction
• ✅ Smart ONNX batch dimension handling: detects static batch, creates cached dynamic-batch copy

4. StaticShelfSupport refactor in galileo_g1_static_pick_and_place_environment.py:

• ✅ Cleaner encapsulation as a class with initial_pose in constructor

⏳ Previous Concerns (Still Open)

• P2: No test for resolve_on_reset=True (dynamic-spawn) path — only static placement is tested
• P2: Validator hooks (validate_layout, validate_placement_candidate_pool) still lack docstrings explaining that callers should raise exceptions to reject (return value is ignored)

Summary

Major quality additions (especially merge_demos.py). Previous inline comments remain applicable but are minor compared to the overall value. Ready to merge. ✅

---

Update (2633285): PR rebased and restructured.

Structural Changes

• ✅ New module relation_solver_interface.py — Clean standalone function solve_and_apply_relation_placement() replacing class-based approach
• ✅ Improved naming — results → layouts throughout for clarity
• ✅ Helper extraction — _refill_pool_via_solve_if_required() in PooledObjectPlacer
• ✅ Added pool_size property — Exposes pool size for visibility

Test Coverage Improvements

• ✅ New test_relation_solver_interface.py with comprehensive tests:
  • Empty objects case
  • Anchor-only case (no reset event needed)
  • Static placement (resolve_on_reset=False)
  • Dynamic spawn pose with missing positions
  • Static initial poses with partial layouts
• ✅ Test naming updated to reflect PooledObjectPlacer rename

Previous Concerns Status

Concern	Status
resolve_on_reset=True path	✅ Now tested via _apply_dynamic_spawn_pose
Anchor-only edge case	✅ Added test
Validator hooks docstrings	⏳ Still open (minor)
object_cfg None error test	⏳ Still open (edge case)

Latest commit is formatting/lint only. Code is clean and well-structured. Ready for final maintainer review. ✅

---

Update (7976fd8): Test refactoring only:

• ✅ Moved imports from module-level to inside test functions/helpers in test_relation_solver_interface.py
• This is a common pattern to avoid import-time issues during test collection
• No functional changes; no new issues. ✅

[greptile-apps]

Greptile Summary

This PR refactors the relation placement pipeline by extracting ArenaEnvBuilder's inline placement logic into a dedicated relation_solver_interface module, and renames internal variables from results/candidates to layouts throughout PooledObjectPlacer and tests for clarity.

• solve_and_apply_relation_placement is introduced as the single public entry-point; it builds a PooledObjectPlacer, then delegates to _apply_dynamic_spawn_pose (resolve_on_reset=True) or _apply_static_initial_poses (resolve_on_reset=False).
• _validate_no_conflicting_pose_reset_events is now called unconditionally in _apply_relation_placement_result, whereas previously it was guarded by if resolve_on_reset: — environments using resolve_on_reset=False with a non-anchor object that has an event_cfg will now fail at build time.
• New tests in test_relation_solver_interface.py cover the empty-object, anchor-only, static-placement, and missing-position fallback paths.

Confidence Score: 4/5

Safe to merge with one known unresolved behavioural change: _validate_no_conflicting_pose_reset_events now fires for both static and dynamic placement paths, which can break existing resolve_on_reset=False setups that pair a non-anchor object with an explicit event_cfg.

The refactoring is clean and the new test coverage is solid. The only outstanding concern is that conflict validation was widened to cover resolve_on_reset=False environments without a migration note, meaning callers that previously relied on the silent pass-through will hit an unexpected RuntimeError at build time. This was flagged in a prior review cycle and has not yet been addressed.

isaaclab_arena/environments/relation_solver_interface.py — specifically _apply_relation_placement_result and _validate_no_conflicting_pose_reset_events.

Important Files Changed

Filename	Overview
isaaclab_arena/environments/arena_env_builder.py	Cleaned up: removed ~90 lines of inline placement logic, now delegates entirely to solve_and_apply_relation_placement. No issues found.
isaaclab_arena/environments/relation_solver_interface.py	New module extracting the placement orchestration. _validate_no_conflicting_pose_reset_events is called unconditionally regardless of resolve_on_reset, which is a breaking change from pre-refactor (previously guarded by if resolve_on_reset:). Also, PooledObjectPlacer is always constructed even when _apply_relation_placement_result will immediately return None for anchor-only inputs, wasting a full solver batch.
isaaclab_arena/relations/pooled_object_placer.py	Refactored: _refill_pool_via_solve_if_required extracted as a named method, variable renames from results to layouts, and new pool_size property added. No correctness issues.
isaaclab_arena/tests/test_relation_solver_interface.py	New test file covering empty-object, anchor-only, static-placement, and fallback-layout paths. _FakePlacementPool is a lightweight stub that satisfies sample_with_replacement.

Sequence Diagram

sequenceDiagram
    participant AEB as ArenaEnvBuilder
    participant RSI as solve_and_apply_relation_placement
    participant POP as PooledObjectPlacer
    participant ARPR as _apply_relation_placement_result
    participant ADS as _apply_dynamic_spawn_pose
    participant ASP as _apply_static_initial_poses

    AEB->>RSI: solve_and_apply_relation_placement(objects, num_envs, ...)
    RSI->>RSI: build ObjectPlacerParams
    RSI->>POP: PooledObjectPlacer(objects, placer_params, pool_size)
    POP->>POP: _solve_and_store(pool_size) [gradient solver]
    RSI->>ARPR: _apply_relation_placement_result(...)
    ARPR->>ARPR: get_anchor_objects → anchor_objects_set
    ARPR->>ARPR: _validate_no_conflicting_pose_reset_events (unconditional)
    alt all objects are anchors
        ARPR-->>RSI: return None
    else "resolve_on_reset=True"
        ARPR->>ADS: _apply_dynamic_spawn_pose(...)
        ADS->>POP: sample_with_replacement(1)
        ADS->>ADS: set object_cfg.init_state
        ADS-->>ARPR: EventTermCfg (reset event)
        ARPR-->>AEB: EventTermCfg stored as _placement_event_cfg
    else "resolve_on_reset=False"
        ARPR->>ASP: _apply_static_initial_poses(...)
        ASP->>POP: sample_with_replacement(num_envs)
        ASP->>ASP: obj.set_initial_pose(PosePerEnv)
        ARPR-->>AEB: None (no reset event needed)
    end

Loading

_{Reviews (8): Last reviewed commit: "fix test" | Re-trigger Greptile}

[alexmillane]

Hi @xyao-nv.

Thank you for abstracting all the object placement-related code out of the Env Builder. That simplifies a class that was becoming quite complicated. Amazing!

I have a few minor comments.

My main concern is that we're starting to get quite a deep hierarchy in the object placement-related code. We have: ArenaRelationSolver, ObjectRelationSolver, PlacementCandidatePool, ObjectPlacer. RelationSolver.

I think we can just eliminate ObjectRelationSolver, which is added in this MR - it appears to be an empty class that just adds a layer to the hierarchy.

At the top level, the ArenaRelationSolver seems to mainly(/entirely) add functions for converting the results of a placement into an effect on the arena environment (either a reset term or directly setting the initial pose). Could we eliminate this layer of the hierarchy by just moving this functionality to a few freestanding functions in a separate file?

[xyao-nv]

Updates based on last round of reviews:

• Keep original `PooledObjectPlacer : it should stay object-only: it owns ObjectPlacer, produces object layout candidates, and samples from that pool.
• Remove ArenaRelationSolver and remove PlacementCandidatePool.
• solve_and_apply_relation_placement(...) remains the orchestration boundary and arena_env_builder.py to call it directly.

[isaaclab-review-bot]

🔄 Update Review (8cff59c)

This update addresses the incremental changes from a3ba6931 → 8cff59c5.

Summary of Changes

This commit continues the API refactoring by completing the transition to a simpler, functional API:

Files Deleted (API Cleanup)

• arena_env_relation_solver.py — Removed the ArenaRelationSolver class entirely
• placement_candidate_pool.py — Removed in favor of renamed PooledObjectPlacer

Files Added

• relation_placement.py — New functional API entry point with solve_and_apply_relation_placement() function
• pooled_object_placer.py — Renamed from PlacementCandidatePool with updated terminology ("layouts" vs "candidates")

Files Modified

• arena_env_builder.py — Updated to use new solve_and_apply_relation_placement() function directly
• placement_events.py — Updated parameter name from placement_candidate_pool to placement_pool
• test_placement_events.py — Updated tests to use PooledObjectPlacer
• test_relation_placement.py — Updated tests for new functional API

Code Quality Assessment ✅

Positive observations:

1. Cleaner API surface — The new functional solve_and_apply_relation_placement() provides a simpler interface than the previous class-based ArenaRelationSolver
2. Consistent terminology — Rename from "candidates" to "layouts" is consistently applied throughout
3. Test coverage maintained — All existing tests updated to match new API
4. Backwards-compatible behavior — The logic remains intact, only the API structure changed

Minor observation:

• The parameter rename in placement_events.py (placement_candidate_pool → placement_pool) is a breaking change for any external code calling solve_and_place_objects directly with keyword arguments. This is acceptable given the refactor scope.

Verdict

No new concerns. The changes complete the API refactoring cleanly. ✅

[alexmillane]

Thanks for the clean up! Looks great. I had a few more small comments.

@zhx06 One of the comments if for you :)

[isaaclab-review-bot]

🔄 Update Review (commit 7976fd8)

Changes since 2633285:

• test_relation_solver_interface.py: Refactored imports from module-level to local function scope

Assessment: ✅ Good cleanup. Moving imports inside test helper functions improves test isolation and avoids potential import-time side effects. No functional concerns.

---

Auto-review by isaaclab-review-bot