Add standalone physics settle check over pooled layouts#769
Conversation
![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: Add physics settle validation check to PooledObjectSolver
Good feature direction — gating layouts on physical settling is the right post-validation strategy for agentic env gen. The PlacementValidationChecklist refactor cleanly generalizes validation, and the failure cache with layout-signature invalidation is well-thought-out. A few issues need attention before merging:
🔴 Critical: CLI argument --enable_physics_settle_check is always True
isaaclab_arena/cli/isaaclab_arena_cli.py
arena_group.add_argument(
"--enable_physics_settle_check",
action="store_true",
default=True,
...
)action="store_true" combined with default=True means the flag is always True — there's no way for the user to disable it. store_true only sets the value to True when the flag is present; since the default is already True, the flag is a no-op.
Fix: Use BooleanOptionalAction (Python 3.9+) to get both --enable_physics_settle_check and --no-enable_physics_settle_check:
arena_group.add_argument(
"--enable_physics_settle_check",
action=argparse.BooleanOptionalAction,
default=True,
help=...,
)🔴 Critical: Wrong TYPE_CHECKING import in placement_events.py
isaaclab_arena/relations/placement_events.py:19
if TYPE_CHECKING:
from isaaclab_arena.assets.object_base import ObjectBase
from isaaclab_arena.relations.placement_validation import PlacementResultPlacementResult lives in isaaclab_arena.relations.placement_result, not placement_validation (which only defines PlacementValidationChecklist). This won't crash at runtime (guarded by TYPE_CHECKING), but type checkers and IDE navigation will break on the result: PlacementResult annotation in write_layout_to_sim.
Fix:
from isaaclab_arena.relations.placement_result import PlacementResult🟡 Warning: PhysicsSettleFailureCache._load can crash on corrupted JSON
isaaclab_arena/relations/physics_settle_failure_cache.py:99-108
def _load(self) -> set[int]:
path = self._path()
if path is None or not os.path.exists(path):
return set()
with open(path) as cache_file:
data = json.load(cache_file)
...A corrupted or truncated cache file (e.g. from a crash mid-write, disk full, or manual editing) will raise json.JSONDecodeError and crash pool construction with a confusing traceback. Since this is an optimization cache (not critical state), the safe behavior is to log a warning and return an empty set.
Suggested fix:
def _load(self) -> set[int]:
path = self._path()
if path is None or not os.path.exists(path):
return set()
try:
with open(path) as cache_file:
data = json.load(cache_file)
except (json.JSONDecodeError, OSError):
return set()
...🟡 Warning: verify_in_sim_and_reselect may append duplicate entries to newly_failed
isaaclab_arena/relations/pooled_object_placer.py:600-620
In the retry loop, when an env fails to settle on attempt 0, the layout is appended to newly_failed. On attempt 1, a replacement is drawn and written, but if the same original layout object (already marked with physics_settled: False via add_checklist_item) is somehow still referenced, the "physics_settled" in checklist.checklist_items guard will use the cached False verdict and append it again. More practically: the current[env_id] is updated to the replacement after the first failure, but the original layout is already in newly_failed — so the replacement (if it also fails) gets appended too, which is correct. However, the if "physics_settled" in checklist.checklist_items branch re-reads the old verdict without re-stepping, which means a replacement layout that was previously cached as failed will short-circuit correctly. This logic is sound but fragile — a brief comment clarifying that current[env_id] is always the replacement at this point would help future readers.
🟡 Warning: _last_applied is never cleared between resets
isaaclab_arena/relations/pooled_object_placer.py:563-565
note_applied accumulates entries in _last_applied keyed by env_id. On subsequent resets, solve_and_place_objects calls note_applied again which overwrites the entry — so it's fine for the settle pass. But if some envs are not reset (partial reset), _last_applied still holds stale entries from prior resets. When verify_in_sim_and_reselect is called, it processes all entries in _last_applied, potentially re-stepping and re-checking envs that weren't just reset.
Suggestion: Clear _last_applied at the start of verify_in_sim_and_reselect (after copying current = dict(self._last_applied)) or only record applied layouts for the current reset batch.
🔵 Improvement: EnvLayoutPool.available is O(n) on every call
isaaclab_arena/relations/pooled_object_placer.py:37-40
@property
def available(self) -> int:
return sum(
1 for layout in self.layouts[self.cursor :] if not layout.validation_checklist.physics_settled_failed
)This scans all remaining layouts on every access. It's called from _available_per_env() which is called frequently during solve loops. For typical pool sizes (~100 layouts) this is fine, but if pools grow large or the check is called in tight loops, consider maintaining a counter decremented when a layout is marked as a settle failure.
🧪 Test Coverage
The PR includes solid test coverage:
test_physics_settle.py: End-to-end sim tests for the settle primitives (at-rest, colliding, parallel per-env) — good.test_physics_settle_failure_cache.py: Persistence round-trip, invalidation (seed mismatch, signature mismatch), and signature determinism/sensitivity — thorough.- Existing tests updated to use the new
PlacementValidationChecklistAPI — no regressions.
Gap: No unit test for verify_in_sim_and_reselect with the retry loop (e.g. mock that demonstrates re-selection when first layout fails and next one settles). The sim tests exercise the primitives, but the orchestration logic is only tested end-to-end via the sim. Consider a focused mock test for the retry/re-selection flow.
CI Status
Pre-commit is currently failing — likely a lint/formatting issue that needs a fix-up commit before merge.
Verdict: Minor fixes needed. The critical CLI bug and the wrong import should be fixed before merge. The cache robustness and _last_applied clearing are recommended improvements.
Update (e6d8178): Significant refactor addressing several issues from the initial review and restructuring the physics settle check architecture:
✅ Issues Addressed
-
CLI default fixed —
--enable_physics_settle_checknow defaults toFalsewithaction="store_true", which is the correct pattern (flag absent → disabled, flag present → enabled). This resolves the "always True" bug from the initial review. -
PhysicsSettleFailureCacheremoved entirely — The on-disk persistence cache (physics_settle_failure_cache.py) and its test file have been deleted. The approach was replaced by a simpler design: failed layouts are marked in-memory only via thePlacementValidationChecklist. This eliminates the corrupted-JSON crash risk and the layout-signature complexity flagged in the initial review. -
_last_appliedis now a public property —note_applied()was removed; callers write directly topool.last_applied[env_id] = result. The stale-partial-reset concern remains (the dict is never cleared), but the logic is now inplacement_events.pywhere the caller has full control. -
verify_in_sim_and_reselectmoved out ofPooledObjectPlacer— It's now a free function inplacement_events.py, receiving the pool, env, and a newPhysicsSettleParamsdataclass. This is a cleaner separation of concerns: the pool owns layout storage/sampling; the event module owns sim-stepping logic. -
newly_failedtracking removed — Since the cache is gone, there's no need to accumulate and persist failures. The logic now simply marks the checklist and moves on, removing the duplicate-append fragility.
🆕 New Architecture
PhysicsSettleParams(new dataclass): Holdsnum_steps,lin_vel_thresh,ang_vel_thresh,max_retries. Decoupled fromObjectPlacerParamswhich previously held these fields.PlacementCheckenum (newStrEnum): Replaces raw string keys ("no_overlap","on_relation","physics_settled") with typed constants. Good for IDE support and typo prevention.run_placement_physics_settle_checkreturnsVecEnvObs | None— After the settle pass steps physics and potentially swaps layouts, the reset-time observation is stale. The function now recomputes and returns a fresh observation (including RTX sensor re-renders). The caller inpolicy_runner.pyadopts it. This is a correctness fix for camera-based policies.draw_replacementis now public — renamed from_draw_replacementsince it's called from outside the class.
🟡 Remaining Concerns
-
_last_appliedstale-entry issue persists — The dict is still never cleared between resets. In partial-reset scenarios,verify_in_sim_and_reselectprocesses all entries including envs not just reset. The caller (policy_runner.py) invokes it once afterenv.reset(), so for full resets this is fine, but partial resets could re-verify stale layouts unnecessarily. -
PlacementResultimport — TheTYPE_CHECKINGimport inplacement_events.pystill importsPlacementResultfromplacement_validation(line in the diff showsfrom isaaclab_arena.relations.placement_validation import PlacementResult). This was flagged in the initial review and appears unchanged. (Needs verification — the diff shows the import block was restructured but the target module may still be wrong.) -
objects_settledlogic flip — The condition was changed fromnot (lin > thresh or ang > thresh)tolin <= thresh and ang <= thresh. These are logically equivalent (De Morgan's law), so this is a readability improvement, not a bug fix. 👍 -
New test
test_settle_check_reselects_unstable_layout— Excellent addition! This directly addresses the test coverage gap identified in the initial review. It builds a real sim env, seeds an unstable layout (cracker box dropped from height), and verifies the settle pass re-selects a stable resting layout. This is the orchestration-level test that was missing.
🧪 Test Coverage Update
- ✅
test_physics_settle_failure_cache.pyremoved (no longer needed since cache is deleted) - ✅
test_settle_check_reselects_unstable_layoutadded — covers the full retry/re-selection flow with live physics - ✅
test_placement_events.pyupdated — mock pools now exposelast_applied: dict = {}instead ofnote_applied()method
Updated Verdict: Good progress. The refactor simplifies the architecture significantly (removing the cache, separating concerns, fixing the CLI default). The observation-refresh after settle is a meaningful correctness improvement. The TYPE_CHECKING import issue from the initial review should still be verified/fixed. Overall, closer to merge-ready.
Update (9078efe): Minor cleanup commit — no new issues.
Changes
-
policy_runner.py— Addedfrom __future__ import annotationsand removed string-quoted type annotations ("PolicyBase"→PolicyBase). This is a standard Python 3.10+ forward-reference pattern backported via the future import. Clean. -
test_relation_solver_interface.py— Introduced_fallback_layout()test helper that constructsPlacementResultwith the newPlacementValidationChecklistAPI (usingPlacementCheck.NO_OVERLAP: Falseto indicate failure). Replaced inlinePlacementResult(success=False, ...)calls throughout the test file. This aligns the tests with the API changes from the previous commit — good housekeeping.
Assessment
Both changes are mechanical/cleanup in nature. No new logic, no new issues. Tests properly updated to match the new validation checklist interface. ✅
|
Thanks for putting this together! I read through it and wanted to share some thoughts, mostly around keeping the placement/solver layer sim-agnostic. Main design point to keep the solver/placement code sim-free We deliberately kept the solver and placement code (ObjectPlacer, PooledObjectPlacer, ...) free of any SimulationApp dependency. Both to keep its dependency surface lean and because that path is fast, whereas anything touching sim is comparatively expensive. I'd love to preserve that boundary here. Conceptually Idea: move the settle/reselection loop into On the cross-run failure cache
|
| placement_seed: int | None = None, | ||
| resolve_on_reset: bool | None = None, | ||
| random_yaw_init: bool = False, | ||
| physics_settle_cache_key: str | None = None, |
There was a problem hiding this comment.
We may want to add more validation checks at the end, which would likely introduce more arguments to the function. I’m wondering whether we can design a clean and extensible interface so that users can easily understand the options, or at least define a well-structured argument list.
There was a problem hiding this comment.
let's scope out a design tgt and imple it in future MRs
There was a problem hiding this comment.
Sounds good — scoping a design target first makes sense here. Happy to review the follow-up when it lands. 👍
…_cache) Strip the persistent disk cache from this branch: PhysicsSettleFailureCache, compute_layout_signature, the physics_settle_cache_key plumbing, and the _enqueue wrappers that pre-marked cached failures. The in-sim settle check and live re-selection (verify_in_sim_and_reselect) remain. Full cache implementation is preserved on branch xyao/dev/physcis_failure_cache. Signed-off-by: Xinjie Yao <xyao@nvidia.com>
xyao-nv
requested review from
alexmillane,
peterd-NV,
qianl-nv and
viiik-inside
as code owners
xyao-nv
changed the title
|
@isaaclab-review-bot |
Greptile SummaryThis PR adds a standalone physics-settle validation sweep over pooled placement layouts, refactors the placement validation model from a single
Confidence Score: 5/5The change is safe to merge; the new pool validation sweep is a pure addition and all existing placement paths are correctly updated to the new checklist model. The core data-model refactor (bool → PlacementValidationChecklist) is applied consistently across all callers, tests, and helpers. The new physics-settle infrastructure is well-isolated and only mutates pool layouts in-place by design. Two minor latent issues were found: a bare dict subscript in
Important Files Changed
Sequence DiagramsequenceDiagram
participant Script as run_placement_pool_validation.py
participant VPL as validate_pool_layouts()
participant WLE as _write_layouts_per_episode()
participant PS as physics_settle
participant GB as _grade_settled_batch()
participant CL as PlacementValidationChecklist
Script->>VPL: validate_pool_layouts(env, settle_params)
VPL->>VPL: get_placement_pool(env) → pool
VPL->>VPL: pool.layouts_per_env() → layouts_per_env[env_id]
loop episode_index in range(max_episodes)
VPL->>WLE: _write_layouts_per_episode(env, layouts_per_env, episode_index)
loop env_id with layout at episode_index
WLE->>WLE: write_layout_to_sim(env, env_id, layout)
Note over WLE: sets pose + zero velocity
end
WLE-->>VPL: batch [(env_id, PlacementResult)]
VPL->>PS: step_physics(env, num_steps) [ALL envs]
PS->>PS: sim.step() + scene.update() × num_steps
VPL->>GB: _grade_settled_batch(env, batch)
GB->>PS: objects_settled_per_episode(env, env_ids)
PS-->>GB: [bool per env_id]
loop (env_id, layout), settled
GB->>CL: add_checklist_item(PHYSICS_SETTLED, settled)
end
GB-->>VPL: [(env_id, checklist)]
end
VPL-->>Script: [(env_id, episode_idx, checklist)]
Script->>Script: log_validation_results(results)
Reviews (4): Last reviewed commit: "remove old" | Re-trigger Greptile |
|
👋 @xyao-nv — I see 5 new commits since my last review ( Summary of ChangesMajor refactor that lifts the physics-settle logic out of What Looks Good ✅
Minor Observations / Questions ❓
Overall this is a clean refactor. The interface is simpler, responsibilities are better isolated, and the new test ( |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def pass_validation_checklist(self, checklist_items: list[str] | None = None) -> bool: | ||
| """Check whether the gating checks pass. Defaults to the required checks only. | ||
| Args: | ||
| checklist_items: checks that must pass. If None, only required checks gate (optional checks are ignored). | ||
| Returns: | ||
| True if all the considered checks pass, False otherwise | ||
| """ | ||
| if checklist_items is None: | ||
| checklist_items = self._required() | ||
| return all(self.checklist_items[item] for item in checklist_items) |
There was a problem hiding this comment.
KeyError
The parameter checklist_items in pass_validation_checklist has the same name as the instance attribute self.checklist_items. Inside the function, the local checklist_items is used as the set of keys to evaluate, while self.checklist_items is the dict of results. The lookup self.checklist_items[item] is correct today, but any future edit that accidentally drops the self. prefix would silently attempt to subscript the list[str] parameter and either produce a wrong result or raise a TypeError. Renaming the parameter (e.g., items_to_check) removes the ambiguity entirely.
| def pass_validation_checklist(self, checklist_items: list[str] | None = None) -> bool: | |
| """Check whether the gating checks pass. Defaults to the required checks only. | |
| Args: | |
| checklist_items: checks that must pass. If None, only required checks gate (optional checks are ignored). | |
| Returns: | |
| True if all the considered checks pass, False otherwise | |
| """ | |
| if checklist_items is None: | |
| checklist_items = self._required() | |
| return all(self.checklist_items[item] for item in checklist_items) | |
| def pass_validation_checklist(self, items_to_check: list[str] | None = None) -> bool: | |
| """Check whether the gating checks pass. Defaults to the required checks only. | |
| Args: | |
| items_to_check: checks that must pass. If None, only required checks gate (optional checks are ignored). | |
| Returns: | |
| True if all the considered checks pass, False otherwise | |
| """ | |
| if items_to_check is None: | |
| items_to_check = self._required() | |
| return all(self.checklist_items[item] for item in items_to_check) |
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!
xyao-nv
changed the title
There was a problem hiding this comment.
Review: Add standalone physics settle check over pooled layouts
Nice work structuring the physics settle validation as a separate, composable layer on top of the existing analytical checks. The PlacementValidationChecklist abstraction is clean, and separating the sim-side primitives (physics_settle.py) from the orchestration logic (placement_pool_validation.py) is a good design decision. The test coverage for the new settle primitives is thorough.
A few issues I'd like to flag:
🐛 Bug: Incorrect TYPE_CHECKING import in placement_events.py
if TYPE_CHECKING:
from isaaclab_arena.assets.object_base import ObjectBase
from isaaclab_arena.relations.placement_validation import PlacementResult # ← wrong modulePlacementResult lives in isaaclab_arena.relations.placement_result, not placement_validation. This will cause type-checker failures (mypy/pyright) when analyzing this file. Should be:
from isaaclab_arena.relations.placement_result import PlacementResult⚠️ enable_physics_settle_check is accepted but unused
The rollout_policy() function in policy_runner.py accepts enable_physics_settle_check: bool = False (line 68), and main() passes the CLI arg through (line 255), but the parameter is never referenced in the function body. The CLI arg and plumbing exist, but no settle check is actually invoked during rollout.
Is this intentional scaffolding for a follow-up PR? If so, consider adding a # TODO or raising NotImplementedError when the flag is True, so users don't assume it's functional.
📝 Misleading docstring on draw_replacement
The docstring states:
"Draw the next pooled layout to retry in one env, skipping layouts known to fail to settle."
But the implementation simply draws the next layout from the queue (sample_for_envs or _sample_reusable_without_replacement) without any filtering of previously-failed layouts. The description implies a skip-list mechanism that isn't present. Consider updating the docstring to match what it actually does (draws the next available layout), or noting that the skip behavior is planned for the follow-up cache PR.
🛡️ Potential KeyError in pass_validation_checklist
In PlacementValidationChecklist.pass_validation_checklist():
def pass_validation_checklist(self, checklist_items: list[str] | None = None) -> bool:
if checklist_items is None:
checklist_items = self._required()
return all(self.checklist_items[item] for item in checklist_items)If required_items contains a check name that hasn't yet been added to checklist_items (e.g., a check that's registered as required but the validation hasn't run yet), this raises a KeyError. Consider using .get(item, False) with a clear semantic ("missing checks are treated as failing"), or validating upfront that all required items are present and raising a more informative error.
💡 Minor: _grade_settled_batch passes gym-wrapped env to objects_settled_per_episode
In placement_pool_validation.py, _grade_settled_batch passes the gym-wrapped env to physics_settle.objects_settled_per_episode, which then calls env.unwrapped.scene internally. This works fine, but it's inconsistent with _write_layouts_per_episode which passes env.unwrapped directly to write_layout_to_sim. Consider making the unwrap point consistent across helpers.
Overall this is solid foundational work. The split between the offline validator script (run_placement_pool_validation.py) and the rollout-time check is well-motivated. Looking forward to seeing the runtime settle-and-reselect loop land in a follow-up!
Update (2cfa1b8): New commits address most previous concerns. Summary:
✅ enable_physics_settle_check unused parameter — Resolved. The CLI arg and rollout_policy parameter have been removed entirely. Clean approach.
✅ Misleading draw_replacement docstring — Resolved. The method (and last_applied property) have been removed entirely, simplifying the PooledObjectPlacer API.
✅ Typo: "recoder" → "recorder" — Fixed in physics_settle.py.
✅ Removed unused last_applied tracking — The last_applied dict and draw_replacement method have been completely removed from PooledObjectPlacer, along with the tracking in placement_events.py. This is a cleaner approach than trying to maintain state for a runtime settle-and-reselect feature that isn't implemented yet.
✅ Tests updated — Mock pools in test_placement_events.py no longer expose last_applied, and test_physics_settle.py now reads from layouts_per_env() instead of last_applied.
⬜ Wrong TYPE_CHECKING import (PlacementResult from placement_validation instead of placement_result) — Still present at line 20 of placement_events.py. This bug was flagged in the initial review and remains unaddressed.
⬜ pass_validation_checklist potential KeyError — Not addressed in this diff (not blocking).
Verdict: Good cleanup. The new commit removes scaffolding for the runtime settle-and-reselect feature that wasn't being used, keeping only the offline pool validator path. One TYPE_CHECKING import bug remains from the initial review (wrong module for PlacementResult). Fix that and this is merge-ready.
| or ``None`` when ``placement_pool`` is omitted and the env has no pooled layouts. | ||
| """ | ||
| # No-ops when no layouts are stored in the pool | ||
| if placement_pool is None: |
There was a problem hiding this comment.
Can we simplify the if condition here? We have two if placement_pool is None
3 participants
Summary
When a solver returns a pooled layouts, there are 2 types of validations. One is analytical, fast, does not require SimApp, e.g. check no overlapping & on relation (existing). The other involves stepping physics, slow but more closely reflects how end user consumes, e.g. physics stability.
In an ideal world, placement_enev shall only use valid layout everytime thru
env.reset()Detailed description
Why
Agentic Env Gen requires a post-validation step to rule out envs with unstable physics, and re-trigger generation if nothing valid. It has to be automatic.
How
python isaaclab_arena/scripts/run_placement_pool_validation.py (--viz kit --render) --num_envs 10 pick_and_place_maple_tableThe standalone script checks if physics settled by advancing physics only, over the pool of layouts returned by the solver, and report results.
What
PlacementValidationChecklistdata class to contain each individual validation check results;physics_settle.pyto execute physics stepping without rendering, and check linear_vel, and ang_vel if below thresholds.placement_pool_validation.pyorchestras how many episodes over how many envs needed for the fixed pool of layoutsrun_placement_pool_validation.pyto execute the checkNext
Add integration with Pool filling and lookup table.
Ideal pipeline:
Each time pool is filled (after solver, before placement event) -> [Optional] run physics check for all env's pool unvalidated layouts & log it down -> Select (valid) one to load in placement events