Skip to content

feat(groot): add UNITREE_G1_SONIC embodiment config#128

Open
cagataycali wants to merge 11 commits into
strands-labs:mainfrom
cagataycali:feat/unitree-g1-sonic-embodiment
Open

feat(groot): add UNITREE_G1_SONIC embodiment config#128
cagataycali wants to merge 11 commits into
strands-labs:mainfrom
cagataycali:feat/unitree-g1-sonic-embodiment

Conversation

@cagataycali
Copy link
Copy Markdown
Member

@cagataycali cagataycali commented May 8, 2026

Summary

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.

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.

+--------------+    latent tokens     +--------------+    joint commands    +--------+
|  VLA Model   | ---------------------> |   SONIC      | ---------------------> | Robot  |
| (2.5 Hz)     |   (64-dim x 40)       |  Decoder     |    (50 Hz)            |        |
|              |                        |  (C++)       |                       |        |
+--------------+                        +--------------+                       +--------+

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

Field Value
Video ego_view (single frame, index 0)
State left_leg, right_leg, waist, left_arm, right_arm, left_hand, right_hand, projected_gravity
Action motion_token, left_hand_joints, right_hand_joints (SONIC latents)
Observation indices [0]
Action horizon 40 steps

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

git clone https://github.com/NVIDIA/Isaac-GR00T.git && cd Isaac-GR00T && uv sync --all-extras

export NUM_GPUS=4
uv run python gr00t/experiment/launch_finetune.py \
    --base-model-path nvidia/GR00T-N1.7-3B \
    --dataset-path /path/to/your/collected_dataset \
    --embodiment-tag UNITREE_G1_SONIC \
    --modality-config-path gr00t/configs/data/embodiment_configs.py \
    --num-gpus $NUM_GPUS \
    --output-dir /path/to/output \
    --max-steps 20000 \
    --global-batch-size 32 \
    --save-steps 5000 \
    --save-total-limit 5 \
    --use-wandb \
    --color-jitter-params brightness 0.3 contrast 0.4 saturation 0.5 hue 0.08

Key parameters:

  • --embodiment-tag UNITREE_G1_SONIC -- maps to unitree_g1_sonic config (this PR)
  • --max-steps 20000 -- good starting point, monitor W&B loss curve
  • --global-batch-size 32 -- total across all GPUs
  • 4+ GPUs recommended

Step 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 5550

Run 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_sonic embodiment directly:

from strands_robots.policies.groot import Gr00tPolicy

policy = Gr00tPolicy(
    embodiment_tag="unitree_g1_sonic",
    server_url="http://<gpu_machine>:5550"
)

# Policy automatically knows the correct state/action spaces
action = policy.predict(observation)

Or via the tool:

agent.tool.gr00t_inference(
    embodiment_tag="unitree_g1_sonic",
    server_url="http://<gpu_machine>:5550",
    action="predict"
)

Changes

  • strands_robots/policies/groot/data_configs.json -- Added unitree_g1_sonic config
  • strands_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 test

Testing

$ python -m pytest tests/policies/groot/test_data_config.py -v
============================== 30 passed in 1.07s ==============================

References


S13 - Review Round Changelog

Round Concern Fix Commit Pin Test
R1 Self-alias no-op, em-dash unicode in tool docstring, duplicate torch_mock line, weak schema assertions 88a1380 tests/policies/groot/test_data_config.py::TestDataConfigG1::test_unitree_g1_sonic_schema (pins full 8-key state list + 3-key action list + language_keys)
R2 SONIC entry missing posttrain checkpoint requirement note (UX trap for users trying base model) 3d11525 N/A (pure docstring clarity, no testable behaviour change)
R3 No regression test for R2 docstring fix (next refactor could silently drop the posttrain warning) 2549d24 tests/policies/groot/test_data_config.py::TestDataConfigG1::test_gr00t_inference_docstring_flags_sonic_as_posttrain
R4 Pin test uses independent substring matches -- tighten to co-location assertion 0ecc841 tests/policies/groot/test_data_config.py::TestDataConfigMap::test_gr00t_inference_docstring_flags_sonic_as_posttrain (now asserts both substrings on same line)
R5 Same-line assertion is layout-brittle (testing layout, not behaviour); use proximity window instead fff181d tests/policies/groot/test_data_config.py::TestDataConfigMap::test_gr00t_inference_docstring_flags_sonic_as_posttrain (300-char proximity window survives reflow)
R6 Proximity window doc.find() anchors on first occurrence -- vulnerable to TOC-poisoning 4643946 tests/policies/groot/test_data_config.py::TestDataConfigMap::test_gr00t_inference_docstring_flags_sonic_as_posttrain (regex with negative lookahead for tag boundaries)

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:

Thread File Category Disposition
Pin test re.findall second-mention gap test_data_config.py Non-blocking nit Current test catches the common refactor (full removal). Reviewer states: "Non-blocking -- the current test catches the most common refactor."
ActionConfig metadata elision note data_configs.json:991 Informational Reviewer states: "no change requested in this PR." Tracked in #212.
SONIC runtime decoder dependency note gr00t_inference.py:140 Non-blocking nit Reviewer states: "Non-blocking since the PR description has the full picture."
observation_indices verification data_configs.json:996 Informational Reviewer states: "informational only."

Round 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

Scope note

The tests/mocks/torch_mock.py changes from the original commit (adding cudnn shims) are not reverted because they fix a pre-existing test gap used by policy_runner.py:111-115. The R1 fix only removes the duplicate manual_seed line that was added at line 322 (already present at line 326). The cudnn shims remain as they are orthogonal to this PR's SONIC config and existed in the codebase before the merge commit.

@cagataycali cagataycali force-pushed the feat/unitree-g1-sonic-embodiment branch from edcb031 to f70ff8a Compare May 12, 2026 18:46
@cagataycali cagataycali moved this from Backlog to In review in Strands Labs - Robots May 12, 2026
@cagataycali cagataycali force-pushed the feat/unitree-g1-sonic-embodiment branch 3 times, most recently from 3989d10 to 52cb416 Compare May 18, 2026 13:19
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
@cagataycali cagataycali force-pushed the feat/unitree-g1-sonic-embodiment branch from 52cb416 to 740ba9a Compare May 18, 2026 19:37
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.
@yinsong1986
Copy link
Copy Markdown
Contributor

Review notes (automated review)

Small, focused PR — clean schema test, docstring update is accurate. CI green. Two items before approving:

Worth fixing

  1. Self-alias is a no-opdata_configs.json:84 adds "unitree_g1_sonic": "unitree_g1_sonic" to the aliases map. The data-config loader (data_config.py:122-123) registers DATA_CONFIG_MAP[alias_name] = DATA_CONFIG_MAP[target_name], so this is just remapping the key to itself — the entry is already directly accessible from the configs block above. Compare to siblings in the same map (real_g1_relative_eef_relative_joints → unitree_g1_real, oxe_droid_rel → oxe_droid_relative_eef_relative_joint), all of which alias an old/short name to the canonical entry. Drop the line.

Nit

  1. action_indices could be list(range(40)) in the loader, but the JSON literally lists 0..39 — consistent with the neighboring unitree_g1_real entry, so this is a style match rather than an issue. No action.

Confirmed (no action)

  • ✅ Schema fields all match Gr00tDataConfig: video_keys, state_keys, action_keys, language_keys, observation_indices, action_indices. No extras, no missing.
  • ✅ Test mirrors the test_unitree_g1_real_n17_schema pattern; assertions cover full-body state + SONIC latent action keys + horizon.
  • gr00t_inference.py docstring update is correct (all 5 G1 variants listed).
  • torch_mock.manual_seed / torch.backends.cudnn additions: needed by simulation/policy_runner.py:111-115 (which sets cudnn.deterministic=True / benchmark=False) and tests/simulation/test_policy_runner_benchmark.py:835-836 — these mocks are legit infrastructure, not tied to this PR's logic but reasonable to land alongside.

LGTM after the self-alias is removed.

Copy link
Copy Markdown
Contributor

@yinsong1986 yinsong1986 left a comment

Choose a reason for hiding this comment

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

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.py adds torch.manual_seed, torch.backends.cudnn.deterministic, and torch.backends.cudnn.benchmark shims. These exist solely to plug holes used by strands_robots/policies/groot/policy.py:612-616 and strands_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_token is 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 existing test_unitree_g1_real_alias pattern does, and does not assert len(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

Comment thread strands_robots/policies/groot/data_configs.json Outdated
Comment thread strands_robots/tools/gr00t_inference.py Outdated
Comment thread tests/policies/groot/test_data_config.py
Comment thread tests/mocks/torch_mock.py
@cagataycali cagataycali added the enhancement New feature or request label May 23, 2026
@cagataycali cagataycali added this to the v0.3.9 milestone May 23, 2026
Copy link
Copy Markdown
Contributor

@yinsong1986 yinsong1986 left a comment

Choose a reason for hiding this comment

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

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_description language key).
  • New regression test test_unitree_g1_sonic_schema lands 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_real docstring line, and (2) adds a duplicate torch_mock.manual_seed assignment in tests/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" in aliases is a no-op — the loader at strands_robots/policies/groot/data_config.py:122-123 will 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. an EmbodimentTag value distinct from the canonical name, like the existing real_g1_relative_eef_relative_joints -> unitree_g1_real pattern). See inline.
  • Test depth. test_unitree_g1_sonic_schema only checks that specific keys appear via in; 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'])

Comment thread strands_robots/policies/groot/data_configs.json Outdated
Comment thread tests/mocks/torch_mock.py Outdated
Comment thread strands_robots/tools/gr00t_inference.py Outdated
Comment thread tests/policies/groot/test_data_config.py Outdated
…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.
Copy link
Copy Markdown
Contributor

@yinsong1986 yinsong1986 left a comment

Choose a reason for hiding this comment

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

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_resolved and test_all_configs_have_required_fields already 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) matches EmbodimentTag.UNITREE_G1_SONIC.value upstream verbatim, so no alias is needed and resolution will work transparently.

Concerns

  • Posttrain vs pretrain distinction is invisible to the user. Upstream EmbodimentTag is explicit that UNITREE_G1_SONIC is a posttrain tag and requires a finetuned checkpoint, while REAL_G1 (= unitree_g1_real) is a pretrain tag baked into nvidia/GR00T-N1.7-3B. The docstring in gr00t_inference.py lists both side-by-side without the distinction, so a user pointing the tool at the stock base checkpoint with embodiment_tag="unitree_g1_sonic" will silently get garbage actions instead of a fail-fast. See inline comment.
  • Schema drops ActionConfig metadata. Upstream's SONIC entry attaches ActionConfig(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 Gr00tDataConfig matches expected keys; it does not exercise the path Gr00tPolicy(embodiment_tag="unitree_g1_sonic", ...) -> load_data_config(...) that the PR description promotes. AGENTS.md > "Integration tests required - each policy needs tests_integ/ tests with real inference" applies to new policies and not to new configs, so this isn't blocking — but a one-line tests/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

Comment thread strands_robots/tools/gr00t_inference.py Outdated
Comment thread strands_robots/policies/groot/data_configs.json
Comment thread tests/policies/groot/test_data_config.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.
Copy link
Copy Markdown
Contributor

@yinsong1986 yinsong1986 left a comment

Choose a reason for hiding this comment

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

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_schema test pins the full key list (state, action, language) verbatim, so a future drift in data_configs.json would 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.
  • _extends was deliberately not used here even though the state keys overlap with unitree_g1_full_body -- correct call, since the action space is fundamentally different (SONIC latents vs joint commands) and inheritance would be misleading.

Concerns

  • ActionConfig metadata elision -- the upstream unitree_g1_sonic declares rep=ABSOLUTE, type=NON_EEF, format=DEFAULT per action key via ActionConfig. The wrapper's Gr00tDataConfig schema 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. if Gr00tPolicy ever 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.

Comment thread strands_robots/tools/gr00t_inference.py
Comment thread tests/policies/groot/test_data_config.py
Comment thread strands_robots/policies/groot/data_configs.json
Copy link
Copy Markdown
Contributor

@yinsong1986 yinsong1986 left a comment

Choose a reason for hiding this comment

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

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_posttrain after 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 ActionConfig metadata (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_sonic notes "requires a finetuned checkpoint" even though unitree_g1_real and the unitree_g1_full_body_with_waist_height_nav_cmd family 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 -v

For 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))

Comment thread tests/policies/groot/test_data_config.py Outdated
Comment thread strands_robots/policies/groot/data_configs.json
Comment thread strands_robots/policies/groot/data_configs.json
Comment thread strands_robots/tools/gr00t_inference.py
Copy link
Copy Markdown
Contributor

@yinsong1986 yinsong1986 left a comment

Choose a reason for hiding this comment

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

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-3B with a posttrain tag) is now called out in the docstring, and the disclaimer is pinned by test_gr00t_inference_docstring_flags_sonic_as_posttrain so it can't silently regress.
  • test_unitree_g1_sonic_schema asserts the full key lists (not just len(...) or in membership), so a key-name typo or reordering would fail loudly.
  • Scope discipline: 3 files, +116/-1, no drive-by changes; the listed torch_mock.py carry-over is noted as orthogonal in the PR description.

Concerns

  • Asymmetric posttrain disclaimer. Upstream embodiment_tags.py lists five tags in POSTTRAIN_TAGS: UNITREE_G1, UNITREE_G1_SONIC, SIMPLER_ENV_GOOGLE, SIMPLER_ENV_WIDOWX, LIBERO_PANDA. This PR adds the "requires a finetuned checkpoint" note only for unitree_g1_sonic, leaving unitree_g1_full_body (alias of upstream UNITREE_G1), libero_panda (LIBERO_PANDA -> libero_sim), and any future simpler_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_sonic and finetuned checkpoint appear 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.

Comment thread tests/policies/groot/test_data_config.py Outdated
Comment thread strands_robots/tools/gr00t_inference.py
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.
Copy link
Copy Markdown
Contributor

@yinsong1986 yinsong1986 left a comment

Choose a reason for hiding this comment

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

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_keys correctly uses annotation.human.task_description (matches upstream, distinct from the annotation.human.action.task_description used 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 the SIMPLER_ENV_* tags are all in POSTTRAIN_TAGS and require a finetuned checkpoint. The disclaimer in gr00t_inference.py:140 only flags unitree_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 base nvidia/GR00T-N1.7-3B checkpoint. 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's data_configs.json schema 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.

Comment thread strands_robots/tools/gr00t_inference.py
from strands_robots.tools.gr00t_inference import gr00t_inference

doc = (gr00t_inference.__doc__ or "").replace("\n", " ")
idx = doc.find("unitree_g1_sonic")
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.

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:

  1. Use doc.rfind("unitree_g1_sonic") to anchor on the last occurrence (where the disclaimer currently lives).
  2. Assert co-location relative to the surrounding section header: assert "finetuned checkpoint" in doc.split("**Unitree G1 humanoid:**")[1][:600].
  3. 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.

Comment thread strands_robots/policies/groot/data_configs.json
…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.
Copy link
Copy Markdown
Contributor

@yinsong1986 yinsong1986 left a comment

Choose a reason for hiding this comment

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

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.json is a faithful 1:1 of the upstream MODALITY_CONFIGS entry (verified against NVIDIA/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-3B model with embodiment_tag="unitree_g1_sonic" would otherwise get silent garbage actions. AGENTS.md > Review Learnings (#85) > "Pin regression tests for reviewed fixes" is honoured by test_gr00t_inference_docstring_flags_sonic_as_posttrain.
  • ASCII-clean: line 140 of gr00t_inference.py introduces no new non-ASCII characters (the orphan-combining-mark issue from PR #86's review learnings does not apply).
  • Follow-up scope (#212 for ActionConfig metadata; #213 for 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 an action_configs=[ActionConfig(rep=ABSOLUTE, type=NON_EEF, format=DEFAULT), ...] list per action key. The JSON drops this entirely (and so does Gr00tDataConfig). For SONIC this is benign (all three are ABSOLUTE/NON_EEF/DEFAULT), but unitree_g1_full_body_with_waist_height_nav_cmd (the other posttrain G1 config upstream) mixes RELATIVE arms with ABSOLUTE hands -- 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_locomanip un-annotated. Per upstream embodiment_tags.py, the canonical posttrain G1 tag is unitree_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 in POSTTRAIN_TAGS for 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_token returned 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 wires Gr00tPolicy(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"
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.

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)
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 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"
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.

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": [
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.

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

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

Labels

enhancement New feature or request

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

3 participants