Skip to content

Add benchmarking script for relation solver#855

Open
zhx06 wants to merge 1 commit into
mainfrom
zxiao/feature/relation_solver_benchmarking
Open

Add benchmarking script for relation solver#855
zhx06 wants to merge 1 commit into
mainfrom
zxiao/feature/relation_solver_benchmarking

Conversation

@zhx06

@zhx06 zhx06 commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Add relation solver benchmarks

Detailed description

  • Adds a sim-free benchmark script for relation solver

Signed-off-by: zhx06 <zihaox@nvidia.com>
bounding_box: AxisAlignedBoundingBox,
initial_pose: Pose | None = None,
relations: list[RelationBase] = [],
collision_mesh: object | None = None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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?

Comment on lines +448 to +462
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,
)
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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:

Suggested change
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-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces a sim-free benchmarking suite for RelationSolver and ObjectPlacer, measuring wall-clock time against configurable dummy clutter scenes without requiring Isaac Sim. It also adds a last_no_overlap_pair_count property to both RelationSolver and ObjectPlacer, and extends DummyObject with an optional collision_mesh field needed by the mesh collision path.

  • New benchmark library (isaaclab_arena/relations/relation_solver_benchmark.py): builds bbox and mesh dummy scenes, times solve() and place() over configurable warmup/timed runs, and reports median latency in a formatted table or JSON file.
  • CLI entry-point (isaaclab_arena_examples/relations/relation_solver_benchmark.py): exposes suite selection (presets, objects, envs), collision mode, iteration cap, seeding, and output path as command-line arguments.
  • Tests (isaaclab_arena/tests/test_relation_solver_benchmark.py): covers scene construction, sweep helpers, solver timing, table formatting, and JSON round-trip.

Confidence Score: 3/5

Safe to merge for the bbox-only path; the mesh path is guarded. However, _initial_positions_for_env will crash with an opaque error for any scene that uses non-On relations or stacks objects in a non-linear order.

The helper _initial_positions_for_env uses an unguarded next() that raises StopIteration (surfaced as RuntimeError inside a generator) for any object without an On relation, and relies on objects being in topological order for stacked (box-on-box) scenes — neither constraint is documented or enforced. The benchmarks and tests today only exercise flat On(table) scenes so they pass, but any future caller extending to NextTo, AtPosition, or stacked objects would hit a cryptic failure. The rest of the PR — the new properties, DummyObject extension, CLI, and tests — is straightforward and correct.

isaaclab_arena/relations/relation_solver_benchmark.py — specifically _initial_positions_for_env and build_solve_inputs.

Important Files Changed

Filename Overview
isaaclab_arena/relations/relation_solver_benchmark.py New benchmark library: builds dummy clutter scenes, times RelationSolver.solve() and ObjectPlacer.place(), writes JSON results. Contains a P1 issue in _initial_positions_for_env — unguarded next() call crashes on non-On relations and unordered parent references cause KeyError for stacked objects.
isaaclab_arena_examples/relations/relation_solver_benchmark.py CLI entry-point that wires argparse arguments to run_benchmarks; straightforward and clean.
isaaclab_arena/tests/test_relation_solver_benchmark.py Unit tests for the benchmark library; covers scene building, sweep helpers, timing, JSON round-trip, and mode expansion. All tests only exercise the bbox path, so the StopIteration / ordering issues in _initial_positions_for_env are not caught.
isaaclab_arena/relations/relation_solver.py Adds last_no_overlap_pair_count property exposing the already-tracked _last_no_overlap_pair_count field — minimal and correct.
isaaclab_arena/relations/object_placer.py Adds last_no_overlap_pair_count property that delegates to the internal solver — clean passthrough.
isaaclab_arena/assets/dummy_object.py Adds optional collision_mesh parameter and get_collision_mesh() accessor to support the mesh benchmark path; no issues.

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
Loading
%%{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
Loading

Reviews (1): Last reviewed commit: "add benchmarking script for relation sol..." | Re-trigger Greptile

Comment on lines +395 to +396
ms_per_iter=ms_per_iter,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Comment on lines +334 to +337
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Suggested change
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!

@arena-review-bot

Copy link
Copy Markdown
Contributor

🤖 Isaac Lab-Arena Review Bot

Summary

This PR adds a sim-free wall-clock benchmark harness for RelationSolver / ObjectPlacer, plus a thin CLI in isaaclab_arena_examples/ and a good unit-test suite. The bbox path is solid, deterministic (seeded), and well tested. The main concern is that a large mesh collision path is wired up against a collision_mode feature that doesn't exist on this branch yet — it's dead as merged and pulls new public API onto DummyObject for that unmerged feature.

Design, Boundaries & Scope

  • Speculative mesh path (main issue). make_solver_params's mesh branch, _build_mesh_clutter_scene, num_spheres, the --collision-mode mesh/--compare-modes/--num-spheres CLI, and the new DummyObject.collision_mesh / get_collision_mesh() all depend on isaaclab_arena.relations.collision_mode and RelationSolverParams.collision_mode/num_spheres, none of which exist here. mesh_collision_available() is always False, so the path never runs, and the file-wide # pyright: reportArgumentType=false partly masks that the mesh kwargs are genuinely invalid. Suggest stripping the mesh mode from this PR and reintroducing it with the collision_mode feature (or landing that first if this is meant to depend on it). See inline.
  • Harness placement (minor question). The 504-line harness lives in the shipped core package isaaclab_arena/relations/. It's sim-agnostic so it doesn't break the layering, but a benchmark harness reads more like dev tooling — would isaaclab_arena/tests/ or a dedicated benchmarks/ location fit better? The tests import it, so I understand the current choice; just flagging for consideration.

Findings

  • 🟡 isaaclab_arena/assets/dummy_object.py:22 — new public collision_mesh field with no consumer except the speculative mesh benchmark path.
  • 🟡 isaaclab_arena/relations/relation_solver_benchmark.py:237 — mesh branch built against a non-existent collision_mode module and RelationSolverParams fields; dead today, TypeError if reached.
  • 🔵 isaaclab_arena/relations/relation_solver_benchmark.py:448 — the 15-field BenchmarkMeasurement reconstruction can collapse to replace(solver_row, place_ms=placer_row.place_ms).

Test Coverage

Good coverage of the bbox path (scene construction, batch shapes, timing sanity, JSON round-trip, sweeps, table formatting) and the mesh case is correctly importorskip/gated. These are pure sim-free unit tests, so the inner/outer run_simulation_app_function pattern isn't needed here. No gaps on the code that actually ships.

Verdict

Minor fixes needed — resolve the speculative mesh path (remove or land collision_mode first); the rest are small.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant