Skip to content

Model env-server command types as an enum, parsed at the wire boundary #592

Description

@vertix

The env-server wire vocabulary defines its arm-command tags as five loose strings plus a hand-written tuple restating the whole set:

# positronic/simulator/env_server/protocol.py
CARTESIAN = 'cartesian'
CARTESIAN_DELTA = 'cartesian_delta'
JOINT_POS = 'joint_pos'
JOINT_VEL = 'joint_vel'
HOLD = 'hold'
CANONICAL_COMMAND_TYPES = (CARTESIAN, CARTESIAN_DELTA, JOINT_POS, JOINT_VEL, HOLD)

This is a closed, dispatched-on domain modelled by its representation — primitive-type in CODE_RULES.md.

What is worth changing

Not the constant names. The value is parsing the tag at the wire boundary: a decoded command becomes a CommandType, and an unknown tag raises where it arrives instead of falling through to a hand-written case _ inside every decoder. The accepted domain and its advertised set then come from one definition rather than two.

The constraint that shapes the fix

protocol.py is the one module every adoption's interpreter imports, and they do not share a Python version:

Adoption Interpreter
LIBERO (simulator/libero/env.py) 3.10 (requires-python = "==3.10.*", run via uv run --no-project)
MolmoSpaces (_MOLMO_PYTHON) 3.11
RoboLab (_ROBOLAB_PYTHON) 3.11

So enum.StrEnum is not usable — it is 3.11+, and adding one to this module breaks LIBERO's server at import. Use class CommandType(str, Enum), which works on all three.

Note the footgun that comes with it: on (str, Enum), str(member) renders CommandType.HOLD, not hold. Every format/join/repr site that shows a tag needs checking. Values still serialize correctly — members are str, so msgpack packs the value, and a decoded plain string still compares equal to its member.

Scope

  • protocol.py: the enum, and CANONICAL_COMMAND_TYPES derived from it
  • Parse at the boundary in the three decoders: env_server/adapter.py, molmo_spaces/mapping.wire_command_to_arm_action, env_server/tests/mujoco_env.py
  • ~45 protocol.CARTESIANprotocol.CommandType.CARTESIAN sites across 8 files, mostly tests
  • The format sites noted above

Verification

The dispatch paths are covered locally (env_server/tests/test_remote_env.py, molmo_spaces/tests/test_mapping.py). The failure mode that is not covered by CI is a foreign venv failing to import protocol.py — none of the three sim servers run in CI, since they need their own environments and GPU boxes. So this change needs each of the three servers actually launched before merge, not just a green suite.

Not urgent

The drift this would prevent is already caught by tests: env_server/tests/test_remote_env.py:323 drives every canonical command through the adapter and asserts the emitted tags equal set(CANONICAL_COMMAND_TYPES), and molmo_spaces/tests/validate.py:121 makes the same check against its payload table. A sixth command added to one and not the other fails today. The enum makes that structural rather than tested, which is better — but it is not closing an open hole.

Raised by Codex on #504 and deferred there: #504 (comment)

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions