Repository navigation
Conversation
heyong4725
left a comment
heyong4725
left a comment
There was a problem hiding this comment.
Review findings on head 739acf7:
-
[P1] The default unit gate fails without the optional Nexus wheel (
tests/unit/test_rollout_engine_attestation.py:63-83,src/aisle/harness/rollout.py:390-395). The two new tests callrun_gates(..., sim_engine="nexus")but stub only the env-hash subprocess.resolve_sim_identity()refuses before that stub whennexus3dis not installed. Nexus/Rapier are absent frompyproject.tomlanduv.lockand are installed separately, so a normaluv sync --extra simenvironment reaches this failure. I reproduced50 passed, 2 failedacross freeze/docs/attestation tests; both failures are these tests. Mock the optional engine availability or inject the resolver in this unit fixture, then run the required unit gate in a clean locked environment. -
[P1] A shortened retail episode gets an unsafe wall clamp (
src/aisle/harness/rollout.py:150-160).resolve_budgets("S1", "oracle")returns(600, 2100), but--episode-timeout-s 180changes it to(180, 570)by applying the desk A7 multiplier to the retail tier. This file documents retail RTF around 0.07; 180 simulated seconds can need roughly 2,570 wall seconds, so a healthy Genesis S1 episode will be killed and recorded aswall_clamp. Keep the retail wall estimate tier/engine-specific or require an explicit measured wall override, rather than silently shrinking it to 570 seconds. Please also reject zero/negative episode timeout overrides. -
[P2]
fleetcan report a different engine from its bridge graph (src/aisle/harness/cli.py:925-939). Omitting--sim-enginecallsnormalize_engine(None)and reports/injectsgenesis, whilerun_fleet()preserves a bridge node's graph-declaredAISLE_SIM_ENGINE=nexus|rapier. Unlikerollout, this path never resolves the graph declaration or rejects a conflicting flag. Depending on Dora's env precedence, the child either runs the graph engine while the report says Genesis, or runs Genesis despite the graph declaration. Apply the sameengine_check/conflict rule used byrolloutbefore stamping and launching the fleet graph. -
[P2] RTF double-counts render time (
src/aisle/nodes/dora_genesis.py:1620-1634,src/aisle/harness/rollout.py:289-309). The bridge measurestick_sfrom beforescene.step()until afterrender_due()and all publishes;render_due()already contributes to that interval.summarize_timing()then addsrender_walltotick_wallagain before computing RTF. The new timing unit test supplies separate synthetic tick/render costs, so it misses this live-path overlap. Use the full tick total alone as the denominator, or measure a tick interval that excludes rendering.
Local review checks: Ruff format/check, trace check, docs inventory, and 166 targeted unit tests passed. The separate freeze/docs/attestation group had the two failures above. I could not run Nexus/Rapier sim tests because their optional wheels are not installed here. The PR is draft and currently has no CI checks. The PR description should also list affected requirement IDs as required by CON-11.
heyong4725
left a comment
heyong4725
left a comment
There was a problem hiding this comment.
Thanks for this. The engine seam is careful work: Genesis dispatch is unchanged when no engine is set, the sim imports stay lazy, the engine is attested separately via sim_engine_hash so existing Genesis env hashes don't move, src/aisle/sim joins the hot-swap frozen roots, and tools/engine_sources.py requires full 40-character commit pins. The frozen set (scenes/verifier/reset/expert graphs) is untouched.
Blocking issues before this can merge (details in the inline comments):
- CRITICAL: ADR-55 and ADR-56 are overwritten. On main they are open PROPOSED decisions for other work (ADR-55: blinded task-selection pilots, #346 / SPEC 490; ADR-56: replicated block-randomized H4 trial, #347 / SPEC 500), and freeze registrations and the README still cite them. The engine ADRs need new numbers (ADR-67, ADR-68), with the original files restored and every
ADR-55/ADR-56engine reference in code and docs renamed. - HIGH: the unit tier will fail in CI.
ci.ymlrunsuv sync(no sim extra) thenpytest -m unit. The new installed-engine refusal inresolve_sim_identityfires for Genesis too, so 8 tests fail withsimulation engine 'genesis' is not installed in this environment: 7 intests/unit/test_idea_gate.pyandtests/unit/test_run_attestation.py::test_settle_records_actual_episode_count. The same files pass on main. The unit tests need to stub the availability check. - HIGH: two concerns in one PR (CON-9).
--episode-timeout-s/--build-grace-sis an independent harness feature with no ADR, and it forces a second round of freeze successors (BND v16, CSE v28, pilot v14). Please move it and those successors to a separate PR. - HIGH (needs a maintainer decision): Nexus and Rapier are built from unmerged dimforge branches pinned in
engine-runtime.json, outsidepyproject.toml/uv.lock. Runs on those engines can therefore never beorigin/main-env-attested. That fits "development-only engines" but should be stated explicitly, together with the spec-change PR the ADR says is still owed for SPEC 020/030.
Also inline: the freeze README history error, the fleet engine check gap, the ambient-lighting DR record mismatch, the Nexus camera leak, the Rapier wake-up gap, and a few low-severity items.
Gates run locally on the PR head: ruff format --check and ruff check pass, and tools/trace_check.py is ok. On the touched unit test files, 256 pass and 8 fail (item 2).
| @@ -1,61 +1,168 @@ | |||
| # ADR-55 — Select two non-oracle tasks by blinded unscored pilots | |||
| # ADR-55: a second physics engine (Nexus) behind the scene contract | |||
There was a problem hiding this comment.
CRITICAL: this replaces the existing ADR-55 ("Select two non-oracle tasks by blinded unscored pilots", PROPOSED, #346) rather than adding a new record. That ADR is still open and is cited by the bnd-task-band-calibration-v* registrations and the README status table. Please restore the original file and give this decision a new number (e.g. ADR-67), then rename the ADR-55 references in code, docs, freeze README and engine-runtime.json. Also, ACCEPTED while "a spec-change PR is still owed" should probably be PROPOSED until that spec change lands.
There was a problem hiding this comment.
Fixed in cf2f246: main's ADR-55 restored; ours is now ADR-67 with every reference renamed; status set to PROPOSED until the spec-change PR lands
| @@ -1,60 +1,81 @@ | |||
| # ADR-56 — Use a replicated block-randomized session trial for H4 | |||
| # ADR-56 — A CPU engine: rapier physics behind the Nexus renderer | |||
There was a problem hiding this comment.
CRITICAL: same issue as ADR-55. The existing ADR-56 ("Use a replicated block-randomized session trial for H4", PROPOSED, #347 / SPEC 500) is the record the cse-causal-study-v* registrations stand on. Please restore it and number this one fresh (e.g. ADR-68).
There was a problem hiding this comment.
Also fixed in cf2f246: main's ADR-56 restored; ours is now ADR-68, also PROPOSED
| sim_engine = normalize_engine(sim_engine) | ||
| except ValueError as exc: | ||
| return {"ok": False, "gate": "sim_engine", "detail": str(exc)} | ||
| if not engine_available(sim_engine): |
There was a problem hiding this comment.
HIGH: breaks the CI unit tier. CI's unit job runs uv sync with no sim extra, so engine_available("genesis") is False and this refuses. 8 unit tests that pass on main now fail: 7 in test_idea_gate.py and test_run_attestation.py::test_settle_records_actual_episode_count. Suggest stubbing engine_available in those tests (or injecting it), rather than requiring the sim extra for -m unit.
There was a problem hiding this comment.
Fixed in 9456595: those tests stub the availability check. I reproduced the 8 failures first by hiding the engine packages. Added a test for the refusal itself.
| ) | ||
| _add_sim_engine_flag(roll) | ||
| roll.add_argument( | ||
| "--episode-timeout-s", |
There was a problem hiding this comment.
HIGH (scope): --episode-timeout-s / --build-grace-s is a separate harness feature from the engine backends, has no ADR, and forces a second round of freeze successors (BND v16, CSE v28, pilot v14). Could this move to its own PR with its own registrations? Also note the override is attested in the run report but nothing cross-checks it against a frozen declaration's budget.
There was a problem hiding this comment.
This has been removed from this branch as requested (will be on a different branch/PR).
| flt-bank-calibration-v6 supersedes v5 with the artifacts of the v2 round that actually ran; | ||
| bnd v6 supersedes v5 after the semantic-gateway manifest gained its sensor-arm inputs. | ||
| bnd v6 supersedes v5 after the semantic-gateway manifest gained its sensor-arm inputs; | ||
| bnd v11 supersedes v10 after the perception CLI gained the ADR-55 `--sim-engine` |
There was a problem hiding this comment.
MEDIUM: this rewrites history. On main, bnd v11 superseded v10 for the regenerated BND-5 perception audit (#580), not for an ADR-55 --sim-engine flag. Looks like a rebase leftover; please drop this line (the v15/v16 paragraphs below are the real entries).
| Marker `sim`: imports nexus3d (and genesis for the Franka asset), runs | ||
| headless. Skipped when the nexus3d wheel is not installed. | ||
|
|
||
| Scene budget: the shared Nexus viewer never releases a sensor camera, so |
There was a problem hiding this comment.
MEDIUM: the Nexus viewer never releases sensor cameras, so the process panics inside wgpu after roughly 11 scene builds. Fixture scoping avoids it here, but any in-process multi-seed or calibration loop will crash with a native panic rather than a Python error. A teardown/remove_sensor_camera path, or at minimum a clear RuntimeError before the budget is exhausted, would help.
There was a problem hiding this comment.
nexus b1b30499 adds remove_sensor_camera, and 48b1c70 frees a superseded scene's cameras. Reproduced the panic on build 11; 30 builds now run. The rapier test skip that worked around it is gone.
| rows = [np.asarray(art.multibody().generalized_velocity()) for art in self.arts] | ||
| return _squeeze_envs(np.stack(rows), self.scene.n_envs) | ||
|
|
||
| def _write_qpos(self, env: int, qpos) -> None: |
There was a problem hiding this comment.
MEDIUM (moderate confidence): _write_qpos (and _write_root at L413) teleport the multibody but never call wake_up() on the link/root bodies, while RapierEntity._write (L168) does. After an episode where the arm settles and rapier puts it to sleep, a TC-6 reset may not integrate at the new pose for a tick. The current tests keep the arm driven, which would mask this.
There was a problem hiding this comment.
Fixed in 48b1c70: both wake every link; the test fails without the fix
|
|
||
| # -- control --------------------------------------------------------- | ||
|
|
||
| def set_dofs_kp(self, kp, dofs_idx_local=None, envs_idx=None) -> None: |
There was a problem hiding this comment.
LOW: if kp has neither 1 nor len(dofs) entries, this silently broadcasts values[0] to every DOF instead of raising. A malformed gains profile would apply wrong gains without error. Same in set_dofs_kv.
There was a problem hiding this comment.
Fixed in 48b1c70: refused with ValueError, on Nexus too.
|
|
||
| def render_due(due: list[str]) -> dict[str, np.ndarray]: | ||
| return render_frames(handle.cams, due, work) | ||
| render_t0 = clock() |
There was a problem hiding this comment.
LOW: with AISLE_DEBUG_VIEW set, the debug camera render lands inside the render_t0 window, so render_ms_mean in sim_timing.jsonl includes it and cross-engine timing is no longer comparable. Suggest recording before the debug render. (Also note sim_timing.jsonl is now written on every Genesis run too.)
There was a problem hiding this comment.
Fixed in 665a46b: render is timed before the debug camera draws. The sidecar on Genesis runs is intentional, since the three-engine comparison needs it.
| samples = [] | ||
| for step in range(steps): | ||
| t_ns = step * 10_000_000 | ||
| if t_ns >= jt[0]: |
There was a problem hiding this comment.
LOW: an empty trace makes jt[0] / gt[0] raise an unhandled IndexError, which escapes the JSON-error handling (CON-8: JSON to stdout, exit nonzero).
There was a problem hiding this comment.
Fixed in 45a7143: returns a JSON error with a nonzero exit; test added.
|
@sebcrozet thanks again for this PR. Could you address the findings in the review above? The blocking ones are:
The inline MEDIUM items (the fleet engine check, the ambient DR record, the Nexus camera leak and the Rapier wake-up) would be good to fix in this PR too. The LOW items can wait for a follow-up. |
…out of the engine change
…teleported arms, refuse malformed gains
…IK and counter APIs
…thout an engine installed
heyong4725
left a comment
heyong4725
left a comment
There was a problem hiding this comment.
Re-review of head 45b76f6:
The four findings from my prior review are addressed: the optional-engine unit tests now stub availability, the unrelated episode/build overrides were removed, fleet uses the rollout engine gate, and RTF no longer double-counts render time. I also verified the ADR renumbering and the other inline fixes from the later review.
Remaining findings:
-
[P1] Development-engine runs do not require their only source-provenance receipt (
tools/env_hash.py:311-329,src/aisle/harness/rollout.py:755-772)._engine_build()turns an absent/malformed Nexus or Rapier receipt intoNone, andrun_gates(..., env_baseline="local")accepts the resultingsim_engine_buildwithout checking itsbuild. Therefore any importablenexus3d/rapier3dwheel can run and produce a manifest whose engine hash contains"build": null. This contradicts ADR-67's development-only scope, which says the build receipt is the out-of-lock engine's only provenance. I reproduced it directly withpython tools/env_hash.py --sim-engine nexus: it succeeds and emits{"build": null, ...}. Refuse a non-Genesis engine when its required receipt is absent/malformed (and for Rapier, when either solver or renderer receipt is absent), ideally verifying it against the pinned commits before launch. -
[P1] The CSE successor manifests claim the wrong source commit (
analysis/freeze/cse-causal-study-v27/freeze-manifest.json:79,539and pilot v13 equivalents). Both manifests now bind the currentrollout.pyhash568f98..., but retaingit_head=6173684.... At that commit,rollout.pyhashes to142af0...; the RTF fix that produced568f98...is the later commit45b76f6. The ordinary freeze check passes because it hashes the working tree and copiesgit_headwithout checking that commit's blobs, but the recorded source revision cannot reproduce the manifest's own artifact hash. Rebuild the manifests from a committed source snapshot and pointgit_headat that snapshot (then commit the manifests separately), or add a successor if repository policy treats these registrations as immutable. -
[P2] ADR-67 still describes pre-implementation gaps as current facts (
docs/decisions/ADR-67.md:167-175). It says rollout records neithersim_engine_hashnor the receipt, althoughrun_gates()and the manifest now recordsim_engine_build; it also says Nexus robot velocities report zero, althoughNexusRobot.get_dofs_velocity()now readsrobot_qvel. This makes the decision record contradict the implementation and obscures the real receipt failure in finding 1. Update the consequences/known-gaps section to the current state.
Verification on this head: Ruff format/check, trace check, docs inventory, claim-evidence, all four freeze checks, 282 targeted unit tests, and the two pinned upstream commit lookups pass. One typed-validation fixture hit its existing 60-second preflight budget on this machine (validation wall budget exhausted before launch); that is an environment-speed limitation rather than a failure attributable to this diff. Nexus/Rapier sim tests remain unrun here because the optional out-of-lock wheels are not installed. The PR is still draft and has no GitHub CI checks.
|
I fixed the three re-review findings in commits
This fork has maintainer edits disabled, so the authenticated maintainer cannot push those commits to Validation: 90 focused unit/freeze/docs tests pass; Ruff format and lint pass; traceability passes; both affected freeze checks report zero drift. The full unit run reached 780 passes before the existing macOS worker sandbox test returned |
|
Superseded by #595, which contains this PR's full history plus the review fixes. Continuing CI and merge work there because this fork has maintainer edits disabled. |
2 participants
This add the support for both Nexus and Rapier as physics simulation backend, using their python bindings. Preliminary performance numbers are shown below.
Rapier is much faster here since the scene is fairly small and doesn’t leverage batch simulation, making GPU engines less efficient than CPU.
Nexus’s renderer seems to be slower than the one in Genesis, resulting in a total time that is slower in small scenes. Nexus is generally measurably faster in the larger scenes (S1, S3) since it physics simulation performance scales better than Genesis as object count in the scene increases.
Scenario S2 wasn’t run since it is missing the execution graph.
Scenario S3 was run from a seed that is accepted by the system, but that particular seed 28 ends up failing the same way with all three engines (misalignment).
This currently relies on the github versions of nexus/parry/kiss3d since the change we need have not been published yet.
Run of a MacbookPro M4 Max.
EDIT: the review comments requested the PR description to be modified. I’m not exactly what the requirement IDs stand for, so I asked Claude to list them for me. Below is is response.
Spec concern
SPEC 020 (scene realization) / SPEC 030 (bridge): optional Nexus and rapier engines behind the unchanged scene contract. Development-only (ADR-67 "Scope"); Genesis remains the default and the only engine behind the measured record.
Requirement IDs implemented or affected
--sim-engineselection, engine gate and conflict refusal, run manifest attestation of engine and buildsrc/aisle/simadded to the hot-swap frozen roots--sim-engineonmonolith runandfault calibratesim_engine_hash) and build receipt attested; engine sources pinned by full commitnexus_runtime,rapier_runtime,engine_sources,nexus_grasp_replay) emit JSON on stdoutunit/simmarkersGates run
ADRs added