Repository navigation
Conversation
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Greptile SummaryThis change adds the G1 SONIC v1.1 whole-body-control task, coordinator commands, ZMQ command ingestion, motion playback, the G1 blueprint, and MuJoCo support. It is not ready to merge because malformed SONIC ZMQ frames with zero-length fields can raise unhandled exceptions during controller dispatch. Earlier reports that SONIC was included in Ubuntu blueprint validation, that velocity RPC bypassed declared-command validation, and that command-wire heading changes were ignored were disproved by exercising the current behavior. Confidence Score: 3/5Not safe to merge until malformed SONIC wire frames are rejected before controller dispatch. One verified input-driven controller crash remains in the SONIC wire decoder and dispatch path. Files Needing Attention: dimos/control/tasks/g1_sonic_wbc_task/zmq_wire.py
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _SONIC_RELEASE_DIR = Path(_env_model_dir) if _env_model_dir else LfsPath("sonic") | ||
| _env_planner = os.environ.get("SONIC_PLANNER_PATH") | ||
| _SONIC_PLANNER_PATH = Path(_env_planner) if _env_planner else LfsPath("sonic/planner_sonic.onnx") |
There was a problem hiding this comment.
unitree-g1-sonic-wbc is not in SELF_HOSTED_BLUEPRINTS, so it is included in UBUNTU_BLUEPRINTS. The normal Ubuntu validation imports this blueprint, and module-level task configuration converts these SONIC LfsPath values to strings. That invokes get_data() and fails with RuntimeError: Missing required tools: git-lfs on the Ubuntu runner. Add this blueprint to SELF_HOSTED_BLUEPRINTS so the LFS-backed configuration is validated in the intended environment.
Artifacts
Narrow SONIC blueprint validation reproduction script
- Authored script computes Ubuntu selection from the production allowlist and invokes the production `_check_blueprint` path, with an in-memory self-hosted comparison mode; takeaway: it exercises the finding without changing production code.
Captured SONIC reproduction script source
- Executed SHA-256 and source capture of the authored reproduction script; takeaway: the test harness used for both observed runs is traceable.
Ubuntu-selected SONIC blueprint import fails without Git LFS
- The executed production `_check_blueprint("unitree-g1-sonic-wbc")` run reports `selected_ubuntu=True`, no Git LFS, and fails through `LfsPath.__str__` and `get_data`; takeaway: the reported import-time LFS failure is reproduced.
Exact pytest SONIC Ubuntu blueprint validation failure
- The exact `test_blueprint_is_valid[unitree-g1-sonic-wbc]` pytest node fails at module line 245 after `LfsPath` calls `get_data` and detects missing `git-lfs`; takeaway: normal Ubuntu blueprint validation has the same failure.
In-memory self-hosted routing excludes SONIC from Ubuntu validation
- The same reproduction script with only an in-memory self-hosted routing addition reports `selected_ubuntu=False` and does not call the import check; takeaway: adding the blueprint to the self-hosted allowlist prevents this Ubuntu failure.
There was a problem hiding this comment.
The SONIC blueprint is now assigned to self-hosted validation, and SONIC model downloads happen explicitly through dimos-sonic-models rather than during blueprint import.
| "start", | ||
| "set_locomotion_mode", |
There was a problem hiding this comment.
G1SonicWBCTask implements set_velocity_command, but TASK_EXPOSES does not declare it. As a result, ControlCoordinator.task_invoke takes the undeclared reflective-dispatch path, emits an undeclared-command warning, and does not apply the coordinator's command signature validation. Add set_velocity_command to the exposure list so callers receive the normal validated RPC behavior.
Artifacts
Narrow coordinator and registry harness source
- This authored harness registers a task with the production velocity-command signature and invokes it through the real registry and ControlCoordinator path, showing the manifest-dependent behavior.
Coordinator output with the shipped g1_sonic_wbc manifest
- The executed shipped-manifest run shows the command absent from coordinator commands, undeclared-dispatch warnings, and the raw method bad-keyword error, confirming the finding.
Coordinator output after declaring set_velocity_command
- The executed comparison run adds only the missing registry exposure and shows coordinator signature validation rejecting the bad keyword while the valid command still dispatches, proving the intended fix.
There was a problem hiding this comment.
set_velocity_command is now included in TASK_EXPOSES, so coordinator calls use the declared command validation.
| if cmd.delta_heading is not None: | ||
| logger.info( | ||
| "ZMQ delta_heading received (not yet applied)", | ||
| task=self._name, | ||
| delta_heading=cmd.delta_heading, | ||
| ) |
There was a problem hiding this comment.
_on_wire_command receives a decoded cmd.delta_heading but only logs it as received; it never calls self._pipeline.apply_heading_increment(...). A SONIC command message can therefore successfully carry an operator yaw adjustment while the controller silently ignores it. Forward the decoded value to the pipeline when it is present.
Artifacts
SONIC command delta-heading reproduction script
- This authored script constructs a packed SONIC command message, decodes it with the repository codec, and invokes the repository command handler to assert that delta heading reaches the pipeline; takeaway: the script is the executable reproduction.
SONIC command delta-heading decode capture
- This executed decode-only capture shows the packed command message preserves `planner=True` and `delta_heading=0.75` before command handling; takeaway: the wire decoder correctly receives the heading increment.
SONIC command delta-heading handler failure
- This executed handler capture shows `heading_increments=[]` after handling the decoded command and ends in the assertion that the heading increment was not applied; takeaway: the handler discards a valid operator heading command.
SONIC prior concerns validation log
- This executed source assertion log confirms `set_velocity_command` is in `TASK_EXPOSES`, the SONIC blueprint is self-hosted, and Ubuntu selection subtracts self-hosted blueprints; takeaway: both prior concerns are fixed.
There was a problem hiding this comment.
The ZMQ command handler has been removed, so the code path that discarded heading commands no longer exists.
| shape = tuple(int(v) for v in f.get("shape", [1])) | ||
| nbytes = int(np.prod(shape)) * dtype.itemsize | ||
| if offset + nbytes > len(payload): | ||
| raise ValueError( | ||
| f"{topic}: payload underrun at field {name!r} ({offset + nbytes} > {len(payload)})" | ||
| ) | ||
| arr = np.frombuffer(payload, dtype=dtype, count=int(np.prod(shape)), offset=offset) | ||
| msg.fields[name] = arr.reshape(shape).copy() |
There was a problem hiding this comment.
decode() accepts a declared shape such as [0] and returns an empty array. The task poller assumes decoded control arrays have elements: command fields raise IndexError at flat[0], planner movement raises during reshape(3), and pose data reaches pose handling before failing. A malformed SONIC ZMQ frame can therefore abort a controller tick instead of being rejected. Reject empty, zero, negative, and otherwise invalid dimensions in the decoder, and validate the expected control-field schemas before dispatch.
There was a problem hiding this comment.
The ZMQ decoder and its controller dispatch path have been removed from this integration.
TomCC7
left a comment
TomCC7
left a comment
There was a problem hiding this comment.
I don't see why we need zmq protocol involved here. @Nabla7 wondering why you choosed this from the first place. do you intend to let our integration work with some external nvidia tool? if not I think only porting the model will be the better option.
There was a problem hiding this comment.
I have no idea why we keep adding model (which was on huggingface) to lfs. should just pull from huggingface
There was a problem hiding this comment.
Models now download from nvidia/GEAR-SONIC on Hugging Face using a pinned revision and SHA-256 verification, and the SONIC model archive has been removed from Git LFS.
| if len(body) < HEADER_SIZE: | ||
| raise ValueError(f"{topic}: body shorter than header ({len(body)} < {HEADER_SIZE})") | ||
| header_json = body[:HEADER_SIZE].rstrip(b"\x00") | ||
| header: dict[str, Any] = json.loads(header_json) |
There was a problem hiding this comment.
can use pydantic model directly instead of manual decode and field validation
There was a problem hiding this comment.
The ZMQ wire decoder has been removed entirely, so there is no longer a manual decoder to replace with Pydantic.
| error: str | None = None | ||
|
|
||
|
|
||
| def infer_protocol_version(fields: dict[str, NDArray]) -> int: |
There was a problem hiding this comment.
this should better be determined once and cached instead of inferring every frame?
There was a problem hiding this comment.
NVIDIA also remembers the first version and checks later messages for changes; our equivalent check only involves a few dictionary lookups.
| if self._active_protocol is None: | ||
| self._active_protocol = protocol | ||
| elif self._active_protocol != protocol: | ||
| result.error = f"protocol version changed {self._active_protocol} -> {protocol}" |
There was a problem hiding this comment.
I wonder in what case would version change
There was a problem hiding this comment.
The format shouldn’t change during a stream, so this check rejects inconsistent input, following NVIDIA’s rule.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
go2 replay realtime
Details
| Benchmark suite | Current: 1ae8524 | Previous: 576d547 | Ratio |
|---|---|---|---|
first frame wall |
8.203 s |
9.922 s |
0.83 |
first frame cpu |
17.113 s |
20.847 s |
0.82 |
run cpu |
26.06 s |
29.144 s |
0.89 |
run cpu (user) |
19.527 s |
21.535 s |
0.91 |
run cpu (system) |
6.533 s |
7.609 s |
0.86 |
peak memory |
2705.773 MB |
2715.977 MB |
1.00 |
peak threads |
403 threads |
409 threads |
0.99 |
disk read |
19.816 MB |
29.492 MB |
0.67 |
disk write |
1374.336 MB |
753.289 MB |
1.82 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'go2 replay realtime'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.08.
| Benchmark suite | Current: 1ae8524 | Previous: 576d547 | Ratio |
|---|---|---|---|
disk write |
1374.336 MB |
753.289 MB |
1.82 |
This comment was automatically generated by workflow using github-action-benchmark.
7dc65b2 to
4ff2529
Compare
Nabla7
changed the title
Nabla7
changed the title
Nabla7
changed the title
Nabla7
changed the title
(cherry picked from commit b2e277d)
There was a problem hiding this comment.
go2 replay realtime (arm64)
Details
| Benchmark suite | Current: 5c3c04f | Previous: c3f0add | Ratio |
|---|---|---|---|
peak memory |
1810.316 MB |
1762.234 MB |
1.03 |
peak threads |
352 threads |
359 threads |
0.98 |
network (transport) |
2736.232 MB |
2735.794 MB |
1.00 |
disk write |
1.699 MB |
1.699 MB |
1 |
instructions |
137.594 G |
137.654 G |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
What is this feature?
Run NVIDIA GEAR-SONIC v1.1 through a standalone
G1SonicPolicyModule, with Jetson clock configuration treated as a hardware startup prerequisite. Theunitree-g1-sonic-wbcblueprint does not use ControlCoordinator or coordinator tasks.MovementManagerselects navigation/teleop velocity commands. Named arm references condition the whole-body policy, which owns all 29 motor targets.SonicControllerowns the 50 Hz policy loop, measured-pose activation ramp and fault latch;SonicPipelinehandles inference. Hardware motor IO remains in the dedicatedG1WholeBodyConnectionworker; simulation uses the existing MuJoCo adapter.disarm()returns to measured-pose hold; emergency stops latch damping until the stack is restarted.set_dry_run(False)followed byarm()requests live control. Simulation arms automatically.dimos-jetson-clocks.service, ordered after NVIDIA's power-mode service. It runs NVIDIA's clock utility as root; DimOS remains unprivileged. It preserves the selected power mode and fan policy and does not start or restart robot control.Stack: #4304 -> #4305 -> #3557. The shared motor-fault interface comes from #4304 and inference comes from #4305. The unmerged ControlCoordinator refactor and sim2 migration are not dependencies.
Setup, streams and RPC examples.
Why do we need this?
Navigation and teleop need to command SONIC while its policy runs continuously. On Jetson, selecting MAXN alone still allows dynamic clock scaling, and manually applied clock locks are lost at reboot. An installed GPU runtime and a selected power mode therefore do not establish the required inference timing.
On 2026-10-18, the Orin NX G1 deployment was already in MAXN. The same publisher-free, 250-step standing inference harness measured the following before and after
jetson_clocks:The deployment trial had ZED HD720 at 15 FPS with NEURAL LIGHT depth and DimOS Desktop running, with Rerun disabled. It used synthetic feedback and sent no motor commands. GPU clocks were mostly 510 MHz before and fixed at 918 MHz afterward; CPU limits were locked to 1,984 MHz.
These measurements describe that deployed workload, not an end-to-end validation of this PR's blueprint. At 50 Hz the step budget is 20 ms. Even after locking, the standing start-to-start interval reached 27.36 ms, and a separate synthetic turning workload reached a 36.31 ms inference step. Clock locking does not prove physical walking stability or guarantee every deadline. The startup check verifies CPU/GPU limits, not power-mode adequacy, EMC, cooling or ongoing thermal throttling.
How to Test
On a Jetson with the documented runtime and models installed, install the clock service once from the checkout:
The service applies clocks immediately and enables them at boot.
active (exited)is expected. The installer requires NVIDIA'sjetson_clocks, an enablednvpmodel.service, systemd and sudo; no passwordless sudo rule is needed. Use the provisioned SONIC environment'sdimosdirectly on Jetson.For simulation on macOS or a CUDA-enabled Linux host with dependencies and models installed. macOS selects CPU inference automatically; hardware remains CUDA-only. On a Mac, install with
uv sync --extra control --extra simand fetch models withuv run --no-sync dimos-sonic-models:Focused verification:
pytest dimos/robot/unitree/g1/test_jetson_clocks.py \ dimos/robot/unitree/g1/test_sonic_policy.py \ dimos/robot/unitree/g1/test_wholebody_connection.py \ -m 'not (self_hosted or mujoco or self_hosted_large)' -q --no-covsystemd-analyze verifyon the G1, and the clock reader passed against the G1's locked clocks under Python 3.10. Boot-time execution has not yet been verified by a reboot.Mac CPU verification
Restores the CPU-simulation path from the older sim2 SONIC work without adding a sim2 dependency. All three models use
CPUExecutionProviderwith four threads; this is explicit simulation support, not a silent hardware fallback.Native WBC blueprint on this Mac, MuJoCo and actual ONNX models, Rerun disabled: 15 seconds, 0.607 m travelled, stopped, remained upright, no policy fault. Policy step mean 5.0 ms, p99 17.3 ms; the maximum exceeded the 20 ms budget, so this is not a hard-real-time guarantee. Commands were sent through the policy RPC, not the Desktop UI. Workers exited, with existing legacy shared-memory resource-tracker cleanup warnings in the log.
A later repeat under severe host load (load average 150.8; 18.5 GB swap in use) hit the existing planner timeout and latched damping. The CPU path is functional, not a hard-real-time guarantee; no watchdog threshold was relaxed.
Latest layer-only policy checks: 24 passed, plus 4 real/sim Mac/Linux composition cases, targeted mypy and commit hooks. Physical-robot acceptance remains separate.
Which issue(s) does this PR close?
No separate issue is closed.
Checklist
Stack update (2026-10-22)
Review and merge in order:
Controller app companion: jeff-hykin/dim-controller#1, separate repository.
Main at
42ca50dfefis merged through the original three branches without rewriting history. No sim2 or unmerged ControlCoordinator dependency was introduced. The native Pico/XRoboToolkit workaround is excluded.Previous checkpoint, before the Mac CPU fix: 224 focused Python checks passed (SONIC/WebXR/movement, gateway/G1 safety/clocks/sim adapter, composition and registry); one hardware-specific check was skipped. Targeted mypy and commit hooks passed. The companion UI passed 290 tests and production-bundle desktop/mobile browser checks using mocked gateway replies.
Mac CPU simulation is now supported and verified with the actual SONIC models in MuJoCo (walking, stopping and staying upright). Hardware still requires CUDA. Enterprise-Pico tracking and physical-robot acceptance remain pending; no robot deployment or actuation was performed.