fix(policies/groot): groot_version is graded in service mode too - #3577
cagataycali merged 4 commits into
Conversation
Service mode reads groot_version to choose the wire shape, but the release guard ran only on the local branch, so "N1.7" sent legacy (B, ...) tensors to an N1.7 server. The guard now runs ahead of the mode branch in both modes.
cagataycali
left a comment
There was a problem hiding this comment.
The fix is right, and I verified the premise against main rather than the description: _build_service_observation reads self._groot_version at policy.py:1295 (n_lead = 2 if self._groot_version == "n1.7" else 1), and that method's own docstring tells service callers targeting an N1.7 server to pass groot_version="n1.7". So the deleted comment's claim that service mode "never reads it" was false on the tree it was written against, and hoisting the guard ahead of the mode branch is the correct remedy. None still passes - groot_version_error treats it as the auto-detect sentinel - so a default construction is unchanged in both modes. The control cell asserting the (1, 1, ...) wire shape is the right non-vacuity check: it pins the value the guard protects, not only the refusal.
One must-fix, one commit, plain push:
The Args: entry still states the claim this PR removes. strands_robots/policies/groot/policy.py:566-571, unchanged on this branch:
Only read in local mode, so it is validated only on the
branch that reads it, asportis.
That is the sentence the deleted comment at (old) line 673 was paraphrasing, and it is the one a caller actually reads for this parameter - the Args: entry is the parameter's documentation surface (AGENTS.md, Key Conventions 13). After this change it is wrong in both halves: the parameter is read in both modes and now validated in both. Suggested wording:
Read in both modes - local mode dispatches a loader on it, and
service mode chooses the wire shape from it ("n1.7"adds the
time axis an N1.7 server requires) - so it is validated at the door
regardless of mode. A value naming no release is refused by name
rather than reported as Isaac-GR00T being absent.
docs/policies/groot.md makes no local-only claim (its one mention at line 54 is mode-neutral), so this entry is the only remaining drift I could find.
The fork runs on 7b0b72f1 were held at action_required and have been released, so CI is running on this head.
The `Args:` entry for `groot_version` said "Only read in local mode, so it is validated only on the branch that reads it, as `port` is". Service mode reads it in `_build_service_observation` to choose the wire shape (`n1.7` adds the time axis an N1.7 server requires), so the first half was already wrong on `main`, and the second stops holding once the guard is hoisted ahead of the mode branch. The entry now names both readers and keeps the `port` contrast, which does still hold: `port` is read only where the policy dials. `groot_version_error` derived the whole domain from "is read as a loader selector", and service mode loads nothing, so it states the value as a release selector whose reader differs by mode. One cell pins the entry against the falsified claim, so the documentation cannot drift back to local-only while the guard runs in both modes. Nothing else graded that claim, which is why it survived the read.
cagataycali
left a comment
There was a problem hiding this comment.
Pushed the must-fix and absorbed main (7 commits, no path overlap). I also corrected groot_version_error's own docstring, which derives the whole domain from "is read as a loader selector" - service mode loads nothing. I said the Args: entry was the only remaining drift; that was wrong, and this is the second half of the same claim.
| measurement | result |
|---|---|
your cells against main's policy.py |
11 failed, 38 passed |
| your cells on this head | 50 passed |
new entry cell against 7b0b72f1's docstring |
1 failed - Only read in local mode still present |
ruff check + format --check |
clean, 2001 files |
mypy strands_robots tests tests_integ |
Success, 2001 source files |
| groot suite + service-port domain + 3 changelog graders + 19 whole-tree graders | 1476 passed |
mkdocs build --strict / assemble_changelog --check / overlap |
clean / OK (1469 pending) / no overlap |
groot_version_error(None, ...) still returns None, so a default construction is unchanged in both modes. The port contrast in the entry is kept because it still holds - port is read only at the one line that dials. Parked fork runs on this head are released.
|
Green on @yinsong1986 over to you. |
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Moves the groot_version release guard out of the local-mode branch to run ahead of the mode branch in Gr00tPolicy.__init__, so service mode also refuses a value that names no release. The premise checks out at head: _build_service_observation reads self._groot_version to pick the wire shape (n_lead = 2 if self._groot_version == "n1.7" else 1), so a mis-cased groot_version="N1.7" in service mode previously slipped through and sent legacy (B, ...) tensors, surfacing as a server-side shape error instead of a constructor ValueError naming the parameter. The guard still passes None (the auto-detect sentinel), so no working configuration is refused; the behaviour change converts a silently-wrong wire shape into an explicit ValueError and ships with a changelog fragment. Docstrings, the groot_version_error domain description, and the tests are updated in lockstep, including a regression test that fails on main and a wire-shape test pinning the value the guard protects.
What's good
- Regression test (
test_service_mode_refuses_a_value_that_names_no_release) fails on main, passes here; the wire-shape test pins the behaviour the guard exists to protect. - Aligns with AGENTS.md "Raise on fatal errors" / "No silent defaults on error": the change is loud (
ValueErrornaming the parameter and accepted spellings), not a silent behaviour shift. - Scope discipline: the diff touches exactly the guard, the docstrings that made the falsified claim, and the tests that graded it.
What
Gr00tPolicyrefuses agroot_versionthat names no release in service mode as well as local mode.Why
Service mode reads
groot_versionin_build_service_observationto choose the wire shape (n1.7adds the time axis an N1.7 server requires), but the release guard ran only on themodel_pathbranch, under a comment saying service mode "never reads it". Sogroot_version="N1.7"was accepted and sent legacy(B, ...)tensors to the server. On main the new test reportsFailed: DID NOT RAISE ValueErrorfor every non-release spelling.Tests
tests/policies/groot/test_local_load_refusal_names_the_cause.py::TestTheVersionIsGradedInBothModes(fails on main, passes here). ruff, mypy clean on touched files.