feat(groot): add UNITREE_G1_SONIC embodiment config#128
Conversation
edcb031 to
f70ff8a
Compare
3989d10 to
52cb416
Compare
Add data config for the new UNITREE_G1_SONIC embodiment from Isaac-GR00T (NVIDIA/Isaac-GR00T@3df8b38). This is a posttrain embodiment using the SONIC whole-body controller where the VLA action space consists of motion tokens + hand joint commands. Config details: - State: full body (legs, waist, arms, hands) + projected_gravity - Action: motion_token, left/right_hand_joints (SONIC latents) - Observation: single frame (index 0) - Action horizon: 40 steps References: - https://github.com/NVIDIA/Isaac-GR00T/blob/3df8b38/gr00t/data/embodiment_tags.py#L105 - https://github.com/NVIDIA/Isaac-GR00T/blob/3df8b38/gr00t/configs/data/embodiment_configs.py
52cb416 to
740ba9a
Compare
The torch mock was missing torch.manual_seed and torch.backends.cudnn which caused test failures in environments without real PyTorch. These are used by PolicyRunner's seed_rng() method.
Review notes (automated review)Small, focused PR — clean schema test, docstring update is accurate. CI green. Two items before approving: Worth fixing
Nit
Confirmed (no action)
LGTM after the self-alias is removed. |
There was a problem hiding this comment.
Summary
Adds an unitree_g1_sonic entry to data_configs.json plus a docstring update on the gr00t_inference tool, a single schema test, and (separately) a couple of torch_mock shims. The configuration itself looks faithful to NVIDIA/Isaac-GR00T@3df8b38 once translated into the project's local prefixed-key convention (video.*, state.*, action.*).
What's good
- Cited upstream commit SHA in the PR body, so future reviewers can diff against canonical.
- Followed the existing JSON schema (
video_keys/state_keys/action_keys/language_keys/observation_indices/action_indices) — no new schema fields invented. - Added a dedicated schema test next to the other G1 schema tests.
Concerns
- Scope creep.
tests/mocks/torch_mock.pyaddstorch.manual_seed,torch.backends.cudnn.deterministic, andtorch.backends.cudnn.benchmarkshims. These exist solely to plug holes used bystrands_robots/policies/groot/policy.py:612-616andstrands_robots/simulation/policy_runner.py:111-115. Useful change, but it has nothing to do with adding a new embodiment config — please either split into its own PR or call it out explicitly in the description so reviewers can audit it on its own merits. (The PR description currently doesn't mention these test-mock changes at all.) - Semantic note. Unlike every other entry in the file,
action.motion_tokenis not a joint group — it is a 64-dim SONIC latent. The JSON schema doesn't currently encode that distinction, but it's worth keeping in mind if anyone later adds shape/dtype validation across configs (e.g. assuming action keys map to per-DoF commands). A one-line JSON comment isn't possible, but a short note in the test docstring naming the dimensionality (64 latent + 7 + 7) would help future maintainers. - Test surface. The new test asserts presence of 3 of the 8 state keys and the 3 action keys, but does not pin
language_keys, does not exercise the new alias entry the way the existingtest_unitree_g1_real_aliaspattern does, and does not assertlen(state_keys) == 8. See inline.
Verification suggestions
hatch run test tests/policies/groot/test_data_config.py -v
python -c 'from strands_robots.policies.groot.data_config import DATA_CONFIG_MAP as M; c=M["unitree_g1_sonic"]; print(len(c.state_keys), c.language_keys, c.action_indices[-1])'
# expected: 8 ['annotation.human.task_description'] 39
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds the unitree_g1_sonic data config entry to data_configs.json for NVIDIA's new SONIC whole-body-controller embodiment, mirroring the upstream Isaac-GR00T modality config (video.ego_view, full-body state with projected_gravity, action triple motion_token/left_hand_joints/right_hand_joints, 40-step horizon). Updates the gr00t_inference tool docstring and adds a schema test. The PR description is unusually thorough (workflow, references, deployment) and the schema matches NVIDIA/Isaac-GR00T@3df8b38 upstream.
What's good
- Schema content matches the upstream
MODALITY_CONFIGS["unitree_g1_sonic"]faithfully (state keys, action keys,delta_indices=range(40),observation_indices=[0],annotation.human.task_descriptionlanguage key). - New regression test
test_unitree_g1_sonic_schemalands alongside the config (per the AGENTS.md "Pin regression tests" rule). - PR description cites upstream commit pinning, deployment workflow, and references — easy to audit.
Concerns
- Scope creep. The PR title and description scope this to "add UNITREE_G1_SONIC config," but the diff also (1) changes an unrelated ASCII hyphen to a Unicode em-dash in the
unitree_g1_realdocstring line, and (2) adds a duplicatetorch_mock.manual_seedassignment intests/mocks/torch_mock.py(the assignment already exists 3 lines below). Both should either be in their own PR with a clear rationale, or dropped. See inline comments. - Self-aliasing entry.
"unitree_g1_sonic": "unitree_g1_sonic"inaliasesis a no-op — the loader atstrands_robots/policies/groot/data_config.py:122-123will assign the entry to itself after the configs loop already registered it. Either remove it or document what it is meant to surface (e.g. anEmbodimentTagvalue distinct from the canonical name, like the existingreal_g1_relative_eef_relative_joints->unitree_g1_realpattern). See inline. - Test depth.
test_unitree_g1_sonic_schemaonly checks that specific keys appear viain; it doesn't pin the full key list/count or assert there are no unexpected keys. For the other Unitree configs the existing tests are similarly loose, so this is a consistency note rather than a blocker.
Verification suggestions
hatch run test -- tests/policies/groot/test_data_config.py -v
python -c "from strands_robots.policies.groot.data_config import DATA_CONFIG_MAP as M; c = M['unitree_g1_sonic']; print(c.video_keys, c.state_keys, c.action_keys, c.observation_indices, len(c.action_indices))"
# Cross-check against upstream:
# https://github.com/NVIDIA/Isaac-GR00T/blob/3df8b38/gr00t/configs/data/embodiment_configs.py (MODALITY_CONFIGS['unitree_g1_sonic'])…II, drop duplicate mock, pin full schema in test Addresses review feedback from both review rounds: 1. data_configs.json: Remove self-referential alias entry "unitree_g1_sonic": "unitree_g1_sonic" which is a no-op (loader already registers canonical name before processing aliases). 2. gr00t_inference.py: Revert Unicode em-dashes (U+2014) to ASCII hyphens on lines 139-140 per AGENTS.md Unicode & String Hygiene rule (tool docstrings are agent-facing). 3. torch_mock.py: Remove duplicate torch_mock.manual_seed assignment at line 322 (already present at line 326). This also removes the scope-creep concern -- the remaining mock shims (manual_seed, cudnn) that predate this PR are left untouched. 4. test_data_config.py: Strengthen test_unitree_g1_sonic_schema to pin full state_keys list (8 keys), full action_keys list, language_keys, and len(state_keys)==8. Prevents silent schema mutation from passing the test. Also fix em-dash in docstring.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds a unitree_g1_sonic entry to data_configs.json matching upstream NVIDIA/Isaac-GR00T@3df8b38 (embodiment_configs.py and embodiment_tags.py). Schema fields (video, state, action, language keys; observation/action indices) line up exactly with the NVIDIA reference, modulo the repo's own video. / state. / action. prefix convention which is preserved consistently. Also extends the docstring in gr00t_inference.py and adds a schema-pin test.
What's good
- Tightly scoped: 3 files, no drive-by changes.
- Schema test is exhaustive (full key lists +
language_keys+ indices), satisfying the "Pin regression tests for reviewed fixes" rule from AGENTS.md > Review Learnings (#85). - Existing
test_all_defs_are_resolvedandtest_all_configs_have_required_fieldsalready give the new entry baseline coverage at no extra cost. - Clean ASCII; em-dash regression from R1 was caught and fixed (verified against head with
grep -nP '[^\x00-\x7F]'). - Tag value (
unitree_g1_sonic) matchesEmbodimentTag.UNITREE_G1_SONIC.valueupstream verbatim, so no alias is needed and resolution will work transparently.
Concerns
- Posttrain vs pretrain distinction is invisible to the user. Upstream
EmbodimentTagis explicit thatUNITREE_G1_SONICis a posttrain tag and requires a finetuned checkpoint, whileREAL_G1(=unitree_g1_real) is a pretrain tag baked intonvidia/GR00T-N1.7-3B. The docstring ingr00t_inference.pylists both side-by-side without the distinction, so a user pointing the tool at the stock base checkpoint withembodiment_tag="unitree_g1_sonic"will silently get garbage actions instead of a fail-fast. See inline comment. - Schema drops
ActionConfigmetadata. Upstream's SONIC entry attachesActionConfig(rep=ABSOLUTE, type=NON_EEF, format=DEFAULT)to each action key. The repo's reduced JSON schema doesn't carry this — which is consistent with all other configs, but SONIC is the first entry where the action keys (motion_token,left_hand_joints,right_hand_joints) are semantically a 64-d learned latent rather than joint/EEF targets. Any downstream code that defaults to joint-angle post-processing on the action stream will produce wrong commands. Not introduced by this PR (the schema gap is pre-existing) — but worth a tracking issue, since SONIC is the first entry that makes the gap user-visible. - No integration-style test. The new test asserts the parsed
Gr00tDataConfigmatches expected keys; it does not exercise the pathGr00tPolicy(embodiment_tag="unitree_g1_sonic", ...)->load_data_config(...)that the PR description promotes. AGENTS.md > "Integration tests required - each policy needstests_integ/tests with real inference" applies to new policies and not to new configs, so this isn't blocking — but a one-linetests/policies/groot/test_*instantiation check (no model load, just construction) would catch a silent typo in the config name across the policy / tool / registry layers in one go.
Verification suggestions
# Unit test
hatch run test -- tests/policies/groot/test_data_config.py -v
# Smoke-check the new key resolves through the policy module
python -c "from strands_robots.policies.groot.data_config import load_data_config; c = load_data_config('unitree_g1_sonic'); print(c.action_keys, len(c.action_indices))"
# Confirm no non-ASCII regressions
grep -nP '[^\x00-\x7F]' strands_robots/policies/groot/data_configs.json strands_robots/tools/gr00t_inference.py…etuned checkpoint)
Per review feedback: REAL_G1 (=unitree_g1_real) is a PRETRAIN_TAG that
works with the stock nvidia/GR00T-N1.7-3B checkpoint, while
UNITREE_G1_SONIC (=unitree_g1_sonic) is a POSTTRAIN_TAG that requires
a finetuned checkpoint. Listing them side by side without that
distinction is a UX trap -- a user pointing the tool at the base
checkpoint with embodiment_tag="unitree_g1_sonic" will silently get
garbage actions.
Mirrors the upstream Isaac-GR00T docstring convention ("Pre-registered
posttrain tags (require finetuned checkpoint)") with a one-line
inline note next to the SONIC entry. Pure docstring change -- no
behaviour change, no schema change, no test impact.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds a unitree_g1_sonic entry to data_configs.json for NVIDIA Isaac-GR00T's new posttrain embodiment, plus a docstring listing in gr00t_inference and a schema-pinning test. I cross-checked the schema against upstream (Isaac-GR00T@3df8b38 gr00t/configs/data/embodiment_configs.py): the 8 state keys, 3 action keys, single ego-view video, 40-step action horizon, and annotation.human.task_description language key all match exactly (modulo the wrapper's video./state./action. prefix convention). Scope is clean: 3 files, +103/-1, no incidental changes.
What's good
- Schema is verified against the upstream commit cited in the PR body.
- The new
test_unitree_g1_sonic_schematest pins the full key list (state, action, language) verbatim, so a future drift indata_configs.jsonwould fail loudly instead of silently shipping a wrong action space. - R2 fix (posttrain disclaimer in the tool docstring) addresses a real silent-failure mode where users would get garbage actions from the stock
nvidia/GR00T-N1.7-3B. _extendswas deliberately not used here even though the state keys overlap withunitree_g1_full_body-- correct call, since the action space is fundamentally different (SONIC latents vs joint commands) and inheritance would be misleading.
Concerns
ActionConfigmetadata elision -- the upstreamunitree_g1_sonicdeclaresrep=ABSOLUTE, type=NON_EEF, format=DEFAULTper action key viaActionConfig. The wrapper'sGr00tDataConfigschema has no slot for this, so the information is dropped. This is a pre-existing gap for every config in the file and the PR description acknowledges it's filed as #211; I'm not blocking on it, but flagging that SONIC is the first config where the absolute-vs-relative distinction is load-bearing for the downstream consumer (motion tokens are post-decoder absolute, not deltas), so #211 just got more urgent.- Missing construction smoke test --
Gr00tPolicy(embodiment_tag="unitree_g1_sonic")is not exercised. The PR description marks this as out-of-scope per reviewer; the schema pin catches typo-class regressions but not policy-side wiring (e.g. ifGr00tPolicyever started rejecting unknown tags or special-casing posttrain ones). Worth a follow-up issue.
Verification suggestions
hatch run test -- tests/policies/groot/test_data_config.py -v
# Expect 30 passed (29 prior + the new sonic test).
python -c 'from strands_robots.policies.groot.data_config import DATA_CONFIG_MAP; c = DATA_CONFIG_MAP["unitree_g1_sonic"]; print(c.modality_config())'
# Confirms the modality_config() call returns the expected 4-modality dict at runtime,
# which is what the GR00T server actually consumes.…aimer (addresses thread on line 185)
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds the unitree_g1_sonic embodiment to data_configs.json, surfaces it in the gr00t_inference tool docstring with a posttrain checkpoint warning, and pins both the JSON schema and the docstring disclaimer with two tests. Schema fields (video / state / action / language keys, observation/action indices) match upstream Isaac-GR00T embodiment_configs.py at 3df8b38 exactly modulo the in-repo video. / state. / action. / annotation. prefix convention.
What's good
- Pure additive change; no existing entry is touched, so blast radius is limited to consumers explicitly opting into
embodiment_tag="unitree_g1_sonic". - Schema test (
test_unitree_g1_sonic_schema) pins the full 8-key state list, 3-key action list, language keys, and observation/action indices rather than length checks. Matches the AGENTS.md "test behaviour, not implementation" / "pin regression tests for reviewed fixes" guidance. - The R2 -> R3 turnaround (adding
test_gr00t_inference_docstring_flags_sonic_as_posttrainafter the R2 docstring fix had no pin test) is exactly the iteration loop AGENTS.md > Review Learnings (#86) > "Pin every reviewed fix with a regression test" calls for. - ASCII-only; no emoji or stray combining marks in the docstring or JSON.
Concerns
- The PR description acknowledges that upstream
ActionConfigmetadata (rep=ABSOLUTE,type=NON_EEF,format=DEFAULT) is dropped at the JSON layer. Tracked as follow-up #212 per the PR body. Worth double-checking #212 has been opened on the project board (https://github.com/orgs/strands-labs/projects/2) before merge per AGENTS.md > Project Dashboard rule. - The posttrain disclaimer is asymmetric: only
unitree_g1_sonicnotes "requires a finetuned checkpoint" even thoughunitree_g1_realand theunitree_g1_full_body_with_waist_height_nav_cmdfamily are also posttrain configs in upstream Isaac-GR00T. PR description correctly flags this as pre-existing and out of scope.
Verification suggestions
hatch run test -- tests/policies/groot/test_data_config.py -vFor an end-to-end sanity check (requires a finetuned SONIC checkpoint, hence not part of CI):
from strands_robots.policies.groot.data_config import DATA_CONFIG_MAP
cfg = DATA_CONFIG_MAP["unitree_g1_sonic"].modality_config()
assert cfg["action"].modality_keys == ["action.motion_token", "action.left_hand_joints", "action.right_hand_joints"]
assert cfg["action"].delta_indices == list(range(40))…addresses thread on line 228)
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds the unitree_g1_sonic embodiment config to data_configs.json to mirror NVIDIA Isaac-GR00T's UNITREE_G1_SONIC posttrain entry (3df8b38), updates the gr00t_inference tool docstring to list the new config and warn it requires a finetuned checkpoint, and pins the schema + the docstring disclaimer with two new tests. The data is purely additive (no changes to consumers of existing configs) and the schema matches upstream gr00t/configs/data/embodiment_configs.py field-for-field (video.ego_view, the 8-key state list ending in state.projected_gravity, the 3-key SONIC action list, obs=[0], actions=range(40)).
What's good
- Schema cross-checked against the upstream pinned commit; key ordering, key prefix convention (
video./state./action.), and indices all match. - Posttrain UX trap (using base
nvidia/GR00T-N1.7-3Bwith a posttrain tag) is now called out in the docstring, and the disclaimer is pinned bytest_gr00t_inference_docstring_flags_sonic_as_posttrainso it can't silently regress. test_unitree_g1_sonic_schemaasserts the full key lists (not justlen(...)orinmembership), so a key-name typo or reordering would fail loudly.- Scope discipline: 3 files, +116/-1, no drive-by changes; the listed
torch_mock.pycarry-over is noted as orthogonal in the PR description.
Concerns
- Asymmetric posttrain disclaimer. Upstream
embodiment_tags.pylists five tags inPOSTTRAIN_TAGS:UNITREE_G1,UNITREE_G1_SONIC,SIMPLER_ENV_GOOGLE,SIMPLER_ENV_WIDOWX,LIBERO_PANDA. This PR adds the "requires a finetuned checkpoint" note only forunitree_g1_sonic, leavingunitree_g1_full_body(alias of upstreamUNITREE_G1),libero_panda(LIBERO_PANDA->libero_sim), and any futuresimpler_env_*entries silently undocumented. The PR description acknowledges this and defers it; recommend filing a tracked follow-up so the disclaimer doesn't drift further out of sync with upstream's category sets. - Brittle layout assertion. The pin test asserts that
unitree_g1_sonicandfinetuned checkpointappear on the same line of the docstring. Inline comment below. - ActionConfig metadata loss is already tracked in #212 per the PR description; not re-raising here.
Verification suggestions
hatch run test -- tests/policies/groot/test_data_config.py -v
python -c "from strands_robots.policies.groot.data_config import DATA_CONFIG_MAP; c = DATA_CONFIG_MAP['unitree_g1_sonic']; print(c.modality_config())"For end-to-end confidence, a tests_integ test that loads the config, opens a Gr00tPolicy against a finetuned checkpoint, and confirms predict() returns the 78-dim action vector (64 motion-token + 7+7 hand joints) would pin the integration contract — but that requires the SONIC posttrain checkpoint and is out of scope for this PR.
The R4 same-line co-location assertion was too tight: it would fail on
docstring reflow (line-wrap, formatter changes) even when the disclaimer
remains semantically co-located with the unitree_g1_sonic mention.
Switch to a window-based search per review feedback:
doc = doc.replace('\n', ' ') # collapse layout
idx = doc.find('unitree_g1_sonic')
assert 'finetuned checkpoint' in doc[idx : idx + 300]
This pins co-location intent (within ~300 chars) without pinning physical
line layout. Still fails on pre-fix code where the disclaimer is absent
entirely (idx == -1 or absent in window), so the regression-pin contract
holds.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds the unitree_g1_sonic embodiment config to data_configs.json, surfaces it in the gr00t_inference tool docstring with a posttrain-checkpoint warning, and pins the schema + docstring with two new tests in test_data_config.py. The schema (8-key state with projected_gravity, 3-key SONIC latent action space, delta_indices=range(40), single-frame ego view) matches upstream Isaac-GR00T@3df8b38/gr00t/configs/data/embodiment_configs.py exactly. All 30 tests in tests/policies/groot/test_data_config.py pass; lint is clean. Scope is tight (3 files, +120/-1) and the review-round changelog is well-documented.
What's good
- Schema matches upstream verbatim (verified against
NVIDIA/Isaac-GR00T@3df8b38). - Two regression tests: one pinning the full schema (state/action/language/horizon), one pinning the posttrain-checkpoint disclaimer in the docstring. Both follow the AGENTS.md "Pin regression tests for reviewed fixes" rule.
language_keyscorrectly usesannotation.human.task_description(matches upstream, distinct from theannotation.human.action.task_descriptionused by libero/simpler).- No host paths, no emojis introduced, no new
except Exception, no new untrusted-input interpolation surfaces. - The R1–R5 review-round changelog in the PR description traces every change to a commit and a pin test, which is exactly what AGENTS.md asks for.
Concerns
- Asymmetric posttrain disclaimer. Per upstream
embodiment_tags.py,UNITREE_G1(unitree_g1_full_body_with_waist_height_nav_cmd),LIBERO_PANDA, and theSIMPLER_ENV_*tags are all inPOSTTRAIN_TAGSand require a finetuned checkpoint. The disclaimer ingr00t_inference.py:140only flagsunitree_g1_sonic, which can be misread to imply the other listed configs (unitree_g1,unitree_g1_full_body,unitree_g1_locomanip,libero_panda,oxe_google,oxe_widowx) work with the basenvidia/GR00T-N1.7-3Bcheckpoint. The PR description acknowledges this as a pre-existing gap deferred to a follow-up; please file/link the tracking issue so it isn't silently lost. (See inline comment.) - Pin-test fragility. The new docstring pin uses
doc.find("unitree_g1_sonic")which returns the first occurrence. A future docstring reorder that adds a TOC line or section header mentioning the tag could shift the substring search to a non-canonical location and silently keep the test green even if the disclaimer were dropped. Inline suggestion below. - Action-modality metadata elision. Upstream carries
ActionConfig(rep=..., type=..., format=...)per action key; the repo'sdata_configs.jsonschema drops these. PR description tracks this as follow-up #212 — confirmed scope-correct, surfacing here only so reviewers don't re-flag it.
Verification suggestions
# Run the new tests in isolation
hatch run pytest tests/policies/groot/test_data_config.py -v
# Sanity-check the JSON parses and the new entry resolves
python -c "from strands_robots.policies.groot.data_config import DATA_CONFIG_MAP; c = DATA_CONFIG_MAP['unitree_g1_sonic']; print(c.name, len(c.state_keys), len(c.action_keys), len(c.action_indices))"
# Expect: unitree_g1_sonic 8 3 40
# Confirm the docstring pin can't be defeated by a non-functional reflow
python -c "from strands_robots.tools.gr00t_inference import gr00t_inference; d=(gr00t_inference.__doc__ or '').replace('\n',' '); i=d.find('unitree_g1_sonic'); print('window:', d[i:i+300])"No integration test (tests_integ/) is required for a pure config addition since the existing tests_integ GR00T tests exercise the schema-loading pipeline.
| from strands_robots.tools.gr00t_inference import gr00t_inference | ||
|
|
||
| doc = (gr00t_inference.__doc__ or "").replace("\n", " ") | ||
| idx = doc.find("unitree_g1_sonic") |
There was a problem hiding this comment.
doc.find("unitree_g1_sonic") returns the first occurrence. This pin can silently regress if a future docstring edit adds a TOC entry, an example block, or a "Posttrain configs:" preamble that mentions unitree_g1_sonic before the canonical description — the 300-char window would land on the new mention, and the disclaimer could be removed from the canonical line without failing this test.
Three options to harden it:
- Use
doc.rfind("unitree_g1_sonic")to anchor on the last occurrence (where the disclaimer currently lives). - Assert co-location relative to the surrounding section header:
assert "finetuned checkpoint" in doc.split("**Unitree G1 humanoid:**")[1][:600]. - Use
re.findall(r"unitree_g1_sonic[^]*?finetuned checkpoint", doc)` and assert the match list is non-empty — forces the disclaimer to follow the tag mention with no other tag mentions in between.
Option (3) is the most reflow-safe. AGENTS.md "Pin regression tests for reviewed fixes" calls for tests that fail on pre-fix code; the current pin would pass on a future regressed state too.
…g (addresses thread line 227) The R5 doc.find() anchor returns the first occurrence of "unitree_g1_sonic" in the docstring. A future TOC or preamble mentioning the tag before the canonical description could defeat the 300-char window, keeping the test green even if the disclaimer were dropped from the canonical entry. Switch to re.findall with a negative lookahead that rejects matches crossing a ``tag_name`` boundary (double-backtick-quoted identifier). This pins the co-location contract without depending on positional ordering -- still fails on pre-fix code where the disclaimer is absent entirely.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds a single new posttrain embodiment entry (unitree_g1_sonic) to data_configs.json, a corresponding bullet in the gr00t_inference tool docstring with a clear "requires a finetuned checkpoint" disclaimer, and two unit tests (full-schema pin + docstring-disclaimer pin). The schema in JSON matches upstream Isaac-GR00T MODALITY_CONFIGS["unitree_g1_sonic"] at the pinned commit 3df8b38 (8 state keys including projected_gravity, 3 action keys for SONIC latents + hand joints, single ego view, 40-step action horizon, single-frame observation).
Scope is tight, schema is verified against upstream, and the round-by-round changelog (R1-R6) shows the author has already addressed self-alias, unicode, mock duplication, schema strictness, posttrain UX disclaimer, and pin-test brittleness in prior rounds. Tests pass locally (30/30 in test_data_config.py).
What's good
- Schema in
data_configs.jsonis a faithful 1:1 of the upstreamMODALITY_CONFIGSentry (verified againstNVIDIA/Isaac-GR00T@3df8b38/gr00t/configs/data/embodiment_configs.py). - New schema test (
test_unitree_g1_sonic_schema) pins the full key list, not just substrings: any upstream rename (projected_gravity->gravity, etc.) trips the test instead of silently producing garbage observations. - The posttrain disclaimer in the docstring directly addresses a real UX trap: users feeding the base
nvidia/GR00T-N1.7-3Bmodel withembodiment_tag="unitree_g1_sonic"would otherwise get silent garbage actions. AGENTS.md > Review Learnings (#85) > "Pin regression tests for reviewed fixes" is honoured bytest_gr00t_inference_docstring_flags_sonic_as_posttrain. - ASCII-clean: line 140 of
gr00t_inference.pyintroduces no new non-ASCII characters (the orphan-combining-mark issue from PR #86's review learnings does not apply). - Follow-up scope (
#212forActionConfigmetadata;#213for symmetric posttrain disclaimers) is filed and linked in the PR description.
Concerns
- JSON schema is now structurally lossy vs. upstream. Upstream
MODALITY_CONFIGS["unitree_g1_sonic"]["action"]carries anaction_configs=[ActionConfig(rep=ABSOLUTE, type=NON_EEF, format=DEFAULT), ...]list per action key. The JSON drops this entirely (and so doesGr00tDataConfig). For SONIC this is benign (all three areABSOLUTE/NON_EEF/DEFAULT), butunitree_g1_full_body_with_waist_height_nav_cmd(the other posttrain G1 config upstream) mixesRELATIVEarms withABSOLUTEhands -- and the same JSON schema cannot represent it. The author tracked this as #212; flagging here so a reviewer doesn't sign off thinking the schema round-trip is complete. - Docstring drift in the same hunk. Line 138 still lists
unitree_g1,unitree_g1_full_body,unitree_g1_locomanipun-annotated. Per upstreamembodiment_tags.py, the canonical posttrain G1 tag isunitree_g1_full_body_with_waist_height_nav_cmd(NOT in this repo's JSON), and the three configs we do ship are old N1.5/N1.6 schemas not inPOSTTRAIN_TAGSfor N1.7. This is exactly what #213 covers, so out of scope -- but a passing user reading line 138 today gets the impression those configs work with the base N1.7 model, which is the same UX trap the SONIC fix addresses. - The motion-token action space has runtime implications the docstring elides. "VLA action space is SONIC latents" is correct but understated: the
action.motion_tokenreturned by inference is not directly executable on the robot -- it must be decoded by the SONIC whole-body controller (separate C++ runtime, 50 Hz). A user who wiresGr00tPolicy(embodiment_tag="unitree_g1_sonic")straight into a Unitree G1 SDK joint-command call gets garbage. Worth one extra sentence pointing at GR00T-WholeBodyControl, since the rest of the docstring assumes the action is consumable.
Verification suggestions
# Full data-config test pass (30 tests, ~1s):
hatch run test tests/policies/groot/test_data_config.py -v
# Round-trip the JSON to confirm no syntax surprises:
python -c "import json; d = json.load(open('strands_robots/policies/groot/data_configs.json')); assert 'unitree_g1_sonic' in d['configs']; print(sorted(d['configs']['unitree_g1_sonic'].keys()))"
# Confirm the upstream schema still matches at the pinned commit:
curl -sL https://raw.githubusercontent.com/NVIDIA/Isaac-GR00T/3df8b3825d67f755e69141446f4315f281b9b7e6/gr00t/configs/data/embodiment_configs.py \
| grep -A 40 'unitree_g1_sonic'No CI-beyond-unit-tests is required; this PR is data-config only, no hardware/sim path is exercised.
| "action_keys": [ | ||
| "action.motion_token", | ||
| "action.left_hand_joints", | ||
| "action.right_hand_joints" |
There was a problem hiding this comment.
Note for posterity (no change requested in this PR): action_keys here flattens the upstream action_configs=[ActionConfig(rep=ABSOLUTE, type=NON_EEF, format=DEFAULT), ...] triple. The current Gr00tDataConfig dataclass has no field for it, so the metadata is lost on round-trip. For SONIC this is harmless (all three entries are identical ABSOLUTE/NON_EEF/DEFAULT), but the next posttrain config that mixes RELATIVE/ABSOLUTE (e.g. unitree_g1_full_body_with_waist_height_nav_cmd upstream, which has RELATIVE arms + ABSOLUTE hands) cannot be represented faithfully in this schema. Tracked in #212 — flagging on the line so a future contributor adding that config sees the trap before silently dropping it.
| ``unitree_g1``, ``unitree_g1_full_body``, ``unitree_g1_locomanip``, | ||
| ``unitree_g1_real`` (N1.7 REAL_G1 embodiment - locomotion + bimanual manipulation) | ||
| ``unitree_g1_real`` (N1.7 REAL_G1 embodiment - locomotion + bimanual manipulation), | ||
| ``unitree_g1_sonic`` (SONIC whole-body controller - VLA action space is SONIC latents; requires a finetuned checkpoint, not the base nvidia/GR00T-N1.7-3B) |
There was a problem hiding this comment.
The disclaimer is a real improvement over silence, but it conflates two distinct deployment risks. "Requires a finetuned checkpoint" warns about the model side; it doesn't warn about the action side -- action.motion_token is a SONIC latent, not an executable joint command. A user wiring this policy's output straight into a Unitree G1 SDK call (the way unitree_g1_real actions can be consumed) gets garbage even with a properly fine-tuned checkpoint, because the SONIC C++ decoder (separate process, 50 Hz, from NVlabs/GR00T-WholeBodyControl) is in the loop.
Suggest one extra clause, e.g.:
unitree_g1_sonic(SONIC whole-body controller — VLA emits 64-dim motion-token latents that must be decoded to joint commands by the SONIC runtime from GR00T-WholeBodyControl; requires a finetuned checkpoint, not the base nvidia/GR00T-N1.7-3B)
Non-blocking since the PR description has the full picture, and the existing pin test would still pass on this rewording (the substring "finetuned checkpoint" is preserved).
| # another ``<tag>`` entry in between -- position-independent, survives | ||
| # TOC additions or docstring reflow that would defeat doc.find() anchoring. | ||
| # Negative lookahead rejects matches that cross a ``tag_name`` boundary. | ||
| pattern = r"unitree_g1_sonic(?:(?!``[a-z_]+``).)*?finetuned checkpoint" |
There was a problem hiding this comment.
Minor pin-test gap: re.findall returns the FIRST unitree_g1_sonic ... finetuned checkpoint match. If a future docstring refactor adds a SECOND mention of unitree_g1_sonic (e.g. a posttrain-section header) WITHOUT the disclaimer, but keeps the original mention intact, the regex still finds the original match and the test passes — even though the new mention introduces the exact UX trap this test is meant to catch.
A stricter check is to assert that every occurrence of unitree_g1_sonic is followed by a disclaimer before the next tag boundary, e.g.:
import re as _re
occurrences = _re.findall(r"``unitree_g1_sonic``[^`]*", doc)
assert occurrences, "unitree_g1_sonic must be listed as ``...``"
for occ in occurrences:
assert "finetuned checkpoint" in occ, (
f"each unitree_g1_sonic mention must carry the posttrain disclaimer; "
f"offending span: {occ!r}"
)Non-blocking — the current test catches the most common refactor (removing the disclaimer entirely).
| "language_keys": [ | ||
| "annotation.human.task_description" | ||
| ], | ||
| "observation_indices": [ |
There was a problem hiding this comment.
Verified observation_indices: [0] matches upstream (delta_indices=[0] for video/state/language modalities). For comparison, sibling config unitree_g1_real uses [-20, 0] (T=2 history) — easy to mis-copy if cargo-culting from a neighbour. Worth one regression test that pins observation_indices == [0] (already covered by test_unitree_g1_sonic_schema line 215, so this is informational only).
Summary
Add data config for the new
UNITREE_G1_SONICembodiment from Isaac-GR00T (NVIDIA/Isaac-GR00T@3df8b38).This is a posttrain embodiment using the SONIC whole-body controller where the VLA action space consists of motion tokens + hand joint commands.
How SONIC Works
Instead of predicting raw joint angles, the VLA predicts SONIC latent motion tokens -- a compact 64-dimensional representation learned by the SONIC whole-body controller. SONIC then decodes these latents into full-body joint commands at 50 Hz.
The VLA only reasons about what to do -- SONIC handles the how: balance, locomotion, and smooth whole-body coordination all come for free from the pretrained controller.
Full action space per step: 78-dimensional = 64-dim motion token + 7-dim left hand joints + 7-dim right hand joints.
Config Details
ego_view(single frame, index 0)left_leg,right_leg,waist,left_arm,right_arm,left_hand,right_hand,projected_gravitymotion_token,left_hand_joints,right_hand_joints(SONIC latents)[0]Post-Tuning Workflow for G1 SONIC
Full tutorial: VLA Workflow: Collect, Fine-tune, Deploy
Step 1: Data Collection
Collect teleop demonstrations using VR whole-body teleoperation with the GR00T-WholeBodyControl stack:
python gear_sonic/scripts/launch_data_collection.py \ --camera-host 192.168.123.164 \ --task-prompt "pick up the soda can and place it in the bin"Output is a LeRobot v2.1 dataset. Collect 50-100+ demonstrations for reliable fine-tuning.
Step 2: Fine-tune with Isaac-GR00T
Key parameters:
--embodiment-tag UNITREE_G1_SONIC-- maps tounitree_g1_sonicconfig (this PR)--max-steps 20000-- good starting point, monitor W&B loss curve--global-batch-size 32-- total across all GPUsStep 3: Deploy for Inference
Start PolicyServer (GPU machine, from Isaac-GR00T):
uv run python gr00t/eval/run_gr00t_server.py \ --model-path /path/to/output/checkpoint-20000 \ --embodiment-tag UNITREE_G1_SONIC \ --device cuda:0 \ --port 5550Run inference (from GR00T-WholeBodyControl):
python gear_sonic/scripts/launch_inference.py \ --policy-host <gpu_machine_ip> \ --policy-port 5550 \ --camera-host 192.168.123.164 \ --prompt "pick up the soda can and place it in the bin"Using with strands-robots
With this PR, you can use the
unitree_g1_sonicembodiment directly:Or via the tool:
Changes
strands_robots/policies/groot/data_configs.json-- Addedunitree_g1_sonicconfigstrands_robots/tools/gr00t_inference.py-- Updated docstring with new embodiment (posttrain note)tests/policies/groot/test_data_config.py-- Added exhaustive schema test + docstring pin testTesting
References
S13 - Review Round Changelog
88a1380tests/policies/groot/test_data_config.py::TestDataConfigG1::test_unitree_g1_sonic_schema(pins full 8-key state list + 3-key action list + language_keys)3d115252549d24tests/policies/groot/test_data_config.py::TestDataConfigG1::test_gr00t_inference_docstring_flags_sonic_as_posttrain0ecc841tests/policies/groot/test_data_config.py::TestDataConfigMap::test_gr00t_inference_docstring_flags_sonic_as_posttrain(now asserts both substrings on same line)fff181dtests/policies/groot/test_data_config.py::TestDataConfigMap::test_gr00t_inference_docstring_flags_sonic_as_posttrain(300-char proximity window survives reflow)4643946tests/policies/groot/test_data_config.py::TestDataConfigMap::test_gr00t_inference_docstring_flags_sonic_as_posttrain(regex with negative lookahead fortagboundaries)Thread disposition (R7 -- no changes, round budget exceeded)
All remaining unresolved threads from 2026-05-24T01:51-03:19 are explicitly marked non-blocking or informational by the reviewer:
re.findallsecond-mention gapRound budget note: This PR has completed 6 review rounds. Per project tenets, the round budget is 3. The remaining feedback is marginal hardening of an already-passing pin test, not architectural. No further changes planned unless blocking feedback arrives.
Follow-up issues
feat(groot): preserve upstream ActionConfig metadata in data_configs.jsondocs(gr00t_inference): symmetric posttrain disclaimer for all POSTTRAIN_TAGS embodimentsScope note
The
tests/mocks/torch_mock.pychanges from the original commit (addingcudnnshims) are not reverted because they fix a pre-existing test gap used bypolicy_runner.py:111-115. The R1 fix only removes the duplicatemanual_seedline that was added at line 322 (already present at line 326). Thecudnnshims remain as they are orthogonal to this PR's SONIC config and existed in the codebase before the merge commit.