fix(docs): sim teleop fence passes robot_name to teleoperate only - #3515
cagataycali merged 4 commits into
Conversation
attach_teleop forwards every kwarg it does not read to Teleoperator(), which refused the fence's robot_name= with ValueError before any device was touched. The fence now passes robot_name only to teleoperate(), and a docs grader resolves each attach_teleop fence kwarg against the config.
Fragment for the PR that moved robot_name from attach_teleop to teleoperate in the sim teleop fence.
cagataycali
left a comment
There was a problem hiding this comment.
The docs fix is right and the premise holds in source: _build_teleop_config computes recognised = set(forwardable) | {"id"} | valid_fields and refuses anything outside it, which is exactly the oracle the grader rebuilds, and robot_name is in neither half. The fence on main therefore raises before any device is touched, as described.
One must-fix in the new test and one nit, both in the same file, so one commit and a plain push covers both. Changelog graders (91) and the tests/-scanning tree graders (136) pass on this head; the fork's CI runs were held at action_required and have been released.
lerobot is not a base dependency - it arrives only through an extra - so resolving the teleoperator config dataclass at call time made every cell of this grader error rather than skip where the extra is absent (13 failed, 1 passed in a [sim-mujoco,dev] venv, one failure per documented attach_teleop call). Gate at module scope on lerobot.teleoperators.config, as the 102 sibling files that reach lerobot do, and the import becomes a plain top-level one: 1 skipped there, 14 passed where the extra is installed. The mixin source read also took the locale encoding while the docs read on the same page states encoding="utf-8"; the source is UTF-8 whatever LC_ALL says, so state it.
cagataycali
left a comment
There was a problem hiding this comment.
Both threads addressed on b2d7506; main absorbed (was 54 behind). The gate string is lerobot.teleoperators.config, the narrowest module this grader reads, and the import is now plain top-level like the 102 siblings that reach lerobot.
| grader | lerobot absent | lerobot installed |
|---|---|---|
c5ab6a5 |
13 failed, 1 passed | 14 passed |
b2d7506 |
1 skipped | 14 passed |
Still non-vacuous: against the pre-fix fence it is 1 failed, 13 passed, and the one failure names the kwarg. The fence's premise reproduces on a live MuJoCo sim -- attach_teleop("so101_leader", ..., robot_name="arm0") raises ValueError: Unknown kwarg(s) ... ['robot_name'] before any device is touched, and robot_name is a teleoperate() parameter.
Gate: ruff + mypy clean (1,999 files), 19 whole-tree graders + changelog 994 passed, -k "docs or teleop" 2,268 passed / 6 skipped, mkdocs --strict clean, no overlap with main.
|
Green on |
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
One-line docs fix plus a grader that keeps it fixed. The "Teleoperate a simulation (MuJoCo)" fence in docs/hardware/teleoperation.md passed robot_name="arm0" to attach_teleop, which forwards every kwarg it does not read (name/method/map_fn) to the Teleoperator() factory, whose _build_teleop_config rejects unknown kwargs with a ValueError before any device is touched — so the fence failed for every reader. The fence now passes robot_name only to teleoperate(), where it is actually read. The new test resolves every documented attach_teleop("<type>", ...) keyword against the mixin's own keyword-only args plus the lerobot config dataclass the factory consults, so the same class of doc rot fails CI instead of readers. Verified against head: the mixin/factory contracts the test encodes match strands_robots/teleop_mixin.py and strands_robots/teleoperator.py on b2d7506c.
What's good
- Both prior review threads are resolved on this head: the module gates on
pytest.importorskip("lerobot.teleoperators.config")(lerobot is an extra, not a base dep) and the mixin-source read now pinsencoding="utf-8". - The grader includes a vacuity guard (
test_the_docs_still_attach_a_teleoperator_by_type) so an empty case list cannot pass silently. - The oracle is lerobot's own registry (
TeleoperatorConfig.get_choice_class), not a hand-maintained copy, so it tracks upstream config changes for free. - Tight scope: docs line, changelog fragment, one test module.
What
The "Teleoperate a simulation (MuJoCo)" fence in
docs/hardware/teleoperation.mdnow passesrobot_nameonly toteleoperate(), where it is read.Why
attach_teleopforwards every kwarg it does not read toTeleoperator(), whose config filter refuses the fence'srobot_name="arm0"before any device is touched, so every reader of that fence hitValueError: Unknown kwarg(s) for teleop_type='so101_leader': ['robot_name']. The new grader on main:Tests
tests/test_docs_attach_teleop_kwargs_reach_the_config.pyresolves each documentedattach_teleop("<type>", ...)kwarg against the mixin's keyword-only args and the lerobot config dataclass the factory consults. ruff, mypy clean on touched files.