Add benchmarking script for relation solver#855
Conversation
Signed-off-by: zhx06 <zihaox@nvidia.com>
| bounding_box: AxisAlignedBoundingBox, | ||
| initial_pose: Pose | None = None, | ||
| relations: list[RelationBase] = [], | ||
| collision_mesh: object | None = None, |
There was a problem hiding this comment.
🟡 This adds a collision_mesh field (and get_collision_mesh() below) to the public DummyObject, but nothing in the repo actually reads it — the only consumer is the mesh benchmark path, which itself depends on an isaaclab_arena.relations.collision_mode module and RelationSolverParams(collision_mode=..., num_spheres=...) that don't exist on this branch. Could we hold this off until the mesh / collision_mode feature actually lands, rather than adding public asset surface for an unmerged feature?
| verbose=False, | ||
| profile=False, | ||
| save_position_history=False, | ||
| collision_mode=CollisionMode.MESH, |
There was a problem hiding this comment.
🟡 The whole mesh collision path targets a collision_mode module and RelationSolverParams.collision_mode / num_spheres fields that don't exist on this branch (there is no relations/collision_mode.py, and RelationSolverParams has neither field). mesh_collision_available() is therefore always False, so this branch is dead today — and if it ever ran, RelationSolverParams(collision_mode=..., num_spheres=...) would raise TypeError. The file-wide # pyright: reportArgumentType=false (line 6) is partly masking this (the DummyObject-as-ObjectBase calls are a legit duck-typing case, but these kwargs are genuinely invalid).
Suggest dropping the mesh mode from this PR — _build_mesh_clutter_scene, num_spheres, this branch, DummyObject.collision_mesh, and the --collision-mode mesh / --compare-modes / --num-spheres CLI — and adding it back alongside the collision_mode feature (or landing collision_mode first if this is meant to depend on it). Is that the intent?
| results.append( | ||
| BenchmarkMeasurement( | ||
| scenario_name=solver_row.scenario_name, | ||
| collision_mode=solver_row.collision_mode, | ||
| num_objects=solver_row.num_objects, | ||
| num_envs=solver_row.num_envs, | ||
| num_optimizable=solver_row.num_optimizable, | ||
| device=solver_row.device, | ||
| solve_ms=solver_row.solve_ms, | ||
| place_ms=placer_row.place_ms, | ||
| iters=solver_row.iters, | ||
| overlap_pairs=solver_row.overlap_pairs, | ||
| ms_per_iter=solver_row.ms_per_iter, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🔵 Every field here is copied verbatim from solver_row except place_ms. Since replace is already imported, this block can collapse to one line that also won't silently drift if BenchmarkMeasurement gains a field:
| results.append( | |
| BenchmarkMeasurement( | |
| scenario_name=solver_row.scenario_name, | |
| collision_mode=solver_row.collision_mode, | |
| num_objects=solver_row.num_objects, | |
| num_envs=solver_row.num_envs, | |
| num_optimizable=solver_row.num_optimizable, | |
| device=solver_row.device, | |
| solve_ms=solver_row.solve_ms, | |
| place_ms=placer_row.place_ms, | |
| iters=solver_row.iters, | |
| overlap_pairs=solver_row.overlap_pairs, | |
| ms_per_iter=solver_row.ms_per_iter, | |
| ) | |
| ) | |
| results.append(replace(solver_row, place_ms=placer_row.place_ms)) |
Greptile SummaryThis PR introduces a sim-free benchmarking suite for
Confidence Score: 3/5Safe to merge for the bbox-only path; the mesh path is guarded. However, The helper
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["CLI: relation_solver_benchmark.py\n(argparse)"]
BASE["_base_scenarios()\npresets / object sweep / env sweep"]
MODES["_collision_modes()\nbbox | mesh"]
APPLY["_apply_run_settings()\noverride iters, seed, warmup, repeat"]
RUN["run_benchmarks(scenarios)"]
SOLVER_BM["run_solver_benchmark(scenario)"]
PLACER_BM["run_placer_benchmark(scenario)"]
BUILD_SCENE["build_clutter_scene()"]
BUILD_INPUTS["build_solve_inputs()\n_initial_positions_for_env()"]
SOLVER["RelationSolver.solve()"]
PLACER["ObjectPlacer.place()"]
MERGE["Merged BenchmarkMeasurement"]
OUTPUT["format_results_table() / write_results_json()"]
CLI --> BASE
CLI --> MODES
BASE --> APPLY
MODES --> APPLY
APPLY --> RUN
RUN --> SOLVER_BM
RUN --> PLACER_BM
SOLVER_BM --> BUILD_SCENE
SOLVER_BM --> BUILD_INPUTS
PLACER_BM --> BUILD_SCENE
BUILD_INPUTS --> SOLVER
PLACER_BM --> PLACER
SOLVER_BM --> MERGE
PLACER_BM --> MERGE
MERGE --> OUTPUT
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
CLI["CLI: relation_solver_benchmark.py\n(argparse)"]
BASE["_base_scenarios()\npresets / object sweep / env sweep"]
MODES["_collision_modes()\nbbox | mesh"]
APPLY["_apply_run_settings()\noverride iters, seed, warmup, repeat"]
RUN["run_benchmarks(scenarios)"]
SOLVER_BM["run_solver_benchmark(scenario)"]
PLACER_BM["run_placer_benchmark(scenario)"]
BUILD_SCENE["build_clutter_scene()"]
BUILD_INPUTS["build_solve_inputs()\n_initial_positions_for_env()"]
SOLVER["RelationSolver.solve()"]
PLACER["ObjectPlacer.place()"]
MERGE["Merged BenchmarkMeasurement"]
OUTPUT["format_results_table() / write_results_json()"]
CLI --> BASE
CLI --> MODES
BASE --> APPLY
MODES --> APPLY
APPLY --> RUN
RUN --> SOLVER_BM
RUN --> PLACER_BM
SOLVER_BM --> BUILD_SCENE
SOLVER_BM --> BUILD_INPUTS
PLACER_BM --> BUILD_SCENE
BUILD_INPUTS --> SOLVER
PLACER_BM --> PLACER
SOLVER_BM --> MERGE
PLACER_BM --> MERGE
MERGE --> OUTPUT
Reviews (1): Last reviewed commit: "add benchmarking script for relation sol..." | Re-trigger Greptile |
| ms_per_iter=ms_per_iter, | ||
| ) |
There was a problem hiding this comment.
Unguarded
next() raises StopIteration for non-On relations
next(r for r in obj.get_relations() if isinstance(r, On)) will raise StopIteration (Python ≥ 3.7 wraps it as a RuntimeError inside a generator) for any object whose only relations are NextTo, AtPosition, or other types. build_solve_inputs is public and accepts an arbitrary list[DummyObject], but the internal helper silently requires every non-anchor object to carry exactly one On relation — a constraint that is nowhere documented. A caller who passes a mixed scene gets an opaque error rather than a clear failure.
Additionally, the non-anchor loop at line 390 iterates objects in definition order. If a child with On(other_box) appears before other_box in the list, the positions[parent] look-up on line 400 will raise a KeyError. Any non-flat stacking order (box stacked on box) hits this today. Consider guarding with next(..., None) and raising a clear ValueError, and sorting or topologically ordering non-anchor objects before iterating.
| initial_positions: list[dict[DummyObject, tuple[float, float, float]]] = [] | ||
| generator = torch.Generator().manual_seed(seed) | ||
| for env_idx in range(num_envs): | ||
| generator.manual_seed(seed + env_idx) |
There was a problem hiding this comment.
The first
generator.manual_seed(seed) on this line is dead code: the very first loop iteration immediately overwrites it with generator.manual_seed(seed + 0), so the initial seeding has no effect. Only the per-env re-seeding inside the loop matters.
| initial_positions: list[dict[DummyObject, tuple[float, float, float]]] = [] | |
| generator = torch.Generator().manual_seed(seed) | |
| for env_idx in range(num_envs): | |
| generator.manual_seed(seed + env_idx) | |
| initial_positions: list[dict[DummyObject, tuple[float, float, float]]] = [] | |
| generator = torch.Generator() | |
| for env_idx in range(num_envs): | |
| generator.manual_seed(seed + env_idx) |
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!
🤖 Isaac Lab-Arena Review BotSummaryThis PR adds a sim-free wall-clock benchmark harness for Design, Boundaries & Scope
Findings
Test CoverageGood coverage of the VerdictMinor fixes needed — resolve the speculative mesh path (remove or land |
Add relation solver benchmarks
Detailed description