Add the MolmoSpaces env-server integration - #504
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb89f73cdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e1e345190
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address Codex on #504: - `_episode_count` mirrors `load_all_episodes`' two layouts (benchmark.json, else legacy `house_*/episode_*.json`), so a whole-benchmark run no longer fails in the client for legacy dirs. - `ObservationCodec` lowercases the derived training task when `lowercase_task` is set, matching the served prompt — so a DROID codec trains and infers on one text distribution (the same-keys contract). Ticket: Positronic-Robotics/internal#91 #refs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 612bde39db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be622e51bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # variants replace the defaults and MolmoSpaces' own Pi policy prefers them when present, so the adapter must too. | ||
| MOLMO_WRIST_CAMERA = 'wrist_camera' | ||
| MOLMO_EXTERIOR_CAMERA = 'exo_camera_1' | ||
| MOLMO_WRIST_CAMERA_VARIANTS = ('wrist_camera_zed_mini',) |
There was a problem hiding this comment.
When running MolmoSpaces RandCam/filament benchmarks with the default camera_dict, the exterior observation is recorded as randomized_zed2_analogue_1, not exo_camera_1. Since this variant list only tries droid_shoulder_light_randomization and then exo_camera_1, MolmoAdapter.observations() raises a KeyError before the policy sees any frame; add the randomized Zed exterior name to the fallback list or make the default mapping match those benchmarks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 373041b. Verified against the pinned molmo source: randomized_zed2_analogue_1 is a real exterior camera (camera_configs.py:474) and the RandCam --camera_names exterior (mb-bench.md:67). Added it to MOLMO_EXTERIOR_CAMERA_VARIANTS, so the default camera_dict now resolves the exterior across the base, light-randomization, and RandCam suites. (The wrist side already covers the RandCam pairing wrist_camera_zed_mini.) Regression-tested.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3f8224af3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f55da7a6c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 507c40b732
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 588658d9e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9141b77d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Include it in `to_spec` so a codec reconstructed via `from_spec` (the server-declared local-stack path) keeps the prompt normalization instead of silently reverting to `lowercase_task=False`. Ticket: none molmo env-server (PR #504) — standalone PR, no tracker ticket
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5300722fc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`env.py` imports `env_server` (websockets server + msgpack codec) off PYTHONPATH; the launcher now installs `websockets`/`msgpack` explicitly instead of relying on MolmoSpaces to pull them, so the wire contract holds if MolmoSpaces ever drops those deps. Ticket: none molmo env-server (PR #504) — standalone PR, no tracker ticket
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b00784446f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| | Layer | Owns | Examples | | ||
| |---|---|---| | ||
| | **Adapter** (per sim/rig, e.g. `simulator/molmo_spaces/adapter.py`) | Rig semantics ONLY: mapping the rig's observation/action vocabulary onto positronic's raw keys | Camera-key mapping, gripper qpos → `[0, 1]` closure, decoded commands → the rig's action format | |
There was a problem hiding this comment.
Adapter is the detail of sims, and has nothing in common with offboard library.
For the policies all the envs are the same, and policy has no means to tell sim env from real env.
There was a problem hiding this comment.
Agreed on the substance, and the fix is bigger than a wording change — leaving this open for your call on where it lands.
Two separate things are wrong with that row, and you named both:
- Placement. The Adapter layer is a sim/rig concern. This README documents the offboard inference package, and an adapter never touches it — a real rig's adapter has no offboard dependency at all.
- Framing. "per sim/rig" implies the policy sees a difference. It does not: everything reaching a codec is positronic's raw keys, and nothing downstream can tell a sim frame from a real one. Calling the row "per sim/rig" smuggles in a distinction the contract explicitly does not have.
The section's audience is whoever writes an adapter, so the honest home looks like positronic/simulator/README.md, with the offboard README keeping only the codec and wire-client halves plus a pointer — and the row renamed to something frame-of-reference-free ("per embodiment"). CLAUDE.md:98 points at this file for exactly this section, so it moves too.
I have not done it: the section is load-bearing for adapter authors and splitting it across two READMEs is a judgement about which half each audience needs, which is yours. Say the word on the destination and I will move it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d57c35cb6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a34997c17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c9dcd0ab7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc3f94abd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| JOINT_POS = 'joint_pos' | ||
| JOINT_VEL = 'joint_vel' | ||
| HOLD = 'hold' | ||
| CANONICAL_COMMAND_TYPES = (CARTESIAN, CARTESIAN_DELTA, JOINT_POS, JOINT_VEL, HOLD) |
There was a problem hiding this comment.
Rule primitive-type violated:
CANONICAL_COMMAND_TYPES defines a closed, dispatched-on domain as unrelated string constants and then restates its membership in a tuple. Define a StrEnum, serialize and parse its values at the wire boundary, dispatch on enum members, and iterate the enum so additions cannot leave the accepted domain and its advertised contract out of sync.
AGENTS.md reference: AGENTS.md:L7-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The rule reading is right, the prescribed fix is not applicable, and the correct version is bigger than this round. Leaving open.
StrEnum breaks the LIBERO server. positronic/simulator/libero/env.py declares # requires-python = "==3.10.*" and runs under uv run --no-project, importing server — and therefore protocol — from PYTHONPATH. enum.StrEnum is 3.11+, so this module gaining one takes that server down at import. protocol.py is the one file that must stay importable by every adoption's interpreter, and today those are 3.10 (LIBERO), 3.11 (MolmoSpaces), 3.11 (RoboLab). class CommandType(str, Enum) works on all three and is what the fix would have to use.
The drift you name is already enforced. test_remote_env.py:323 builds every canonical command through the adapter and asserts tags == set(protocol.CANONICAL_COMMAND_TYPES), so a sixth command added to one and not the other fails a test today. validate.py:121 makes the same check against its payload table. The tuple is not a silently-drifting restatement; it is a checked one. An enum would make it structural instead of tested, which is better, but it is not closing an open hole.
What the real change is. Not a type swap: the value of your suggestion is parse at the wire boundary, so a decoded tag becomes a CommandType and an unknown one raises there rather than falling through to a hand-written case _ in each decoder. That means the enum, plus the parse in EnvAdapter, molmo_spaces/mapping.wire_command_to_arm_action and env_server/tests/mujoco_env.py, plus the ~45 protocol.CARTESIAN -> protocol.CommandType.CARTESIAN sites across 8 files, plus the format sites that would start rendering CommandType.HOLD instead of hold under (str, Enum).
That is a coherent change and worth making. It is not one to land as the seventh round of a review loop on a 71-commit branch, where the failure mode I care about most — a foreign venv failing to import protocol — has no CI coverage on any of the three, so a mistake surfaces on a GPU box rather than here.
Flagging it to the owner as its own change rather than silently dropping it.
There was a problem hiding this comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37bf14e5a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 951c4b5318
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # whatever its own controller natively takes. Owned here because both interpreters spell the tags: | ||
| # positronic's ``EnvAdapter`` writes them, an env venv's own decoder reads them, and this is the module both | ||
| # sides import. |
There was a problem hiding this comment.
Rule hardcoded-keys violated:
protocol now owns the wire names because both interpreters import it, but production client.py/server.py still duplicate the request fields and command tags, while libero/env.py and robolab/env.py still parse and emit action/frame fields using bare literals instead of the ACTION_*, COMMAND_*, and FRAME_* constants. Define the remaining request-envelope constants here and migrate all production readers and writers so the shared wire contract has one spelling.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Split verdict — the client.py/server.py half is fixed in df65d5e, the foreign-env.py half is the sweep I backed out last round for a reason that still holds.
Done. protocol now owns the request envelope: REQUEST_CMD, REQUEST_TOKEN, REQUEST_ACTION, CMD_RESET, CMD_STEP, CMD_CLOSE, RESPONSE_OK, RESPONSE_ERROR. Both sides of the socket use them, including the dispatch match in _handle and the client's error re-raise. server.py's dual-shape import block picks up the module alongside decode/encode in both its package and flat forms.
That pass turned up something worth fixing rather than re-baselining. encode was functools.partial(msgpack.packb, default=_pack), so it typed as bytes | None — packb's streaming mode, which no caller here uses — and my edits shifted the columns of the baselined reportArgumentType entries that papered over it. It is a function returning bytes now, with the assert stating why. The baseline drops 5 entries, in client.py, server.py and robolab/launcher.py.
Not done: libero/env.py and robolab/env.py. This is the same migration I attempted and reverted in 951c4b5, and the blocker is unchanged: libero/env.py cannot resolve import protocol under the current pyright config, because LIBERO has no executionEnvironments entry the way RoboLab does. Adding one is the right fix and would clear two baselined reportMissingImports — but it makes pyright newly able to see server and protocol in a ~330-line file, which can surface diagnostics that were invisible, in an adoption this PR does not otherwise touch and that no CI job runs.
Both files also sit in interpreters (LIBERO 3.10, RoboLab's Isaac stack) whose only failure signal is a server that will not start on a GPU box. molmo_spaces/env.py is at zero wire literals — that is this PR's adoption, done. The other two are a follow-up with the type config as its first step, not a line in this round.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df65d5e649
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2374f80ae9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76d1ea70d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| from positronic.simulator.env_server import protocol | ||
| except ImportError: | ||
| import protocol # pyright: ignore[reportMissingImports] |
There was a problem hiding this comment.
Rule swallowed-error violated:
The broad ImportError handler can hide a failure raised while importing the packaged protocol and then report a misleading missing top-level protocol; select the packaged or flat import from __package__ instead, so genuine dependency failures surface unchanged.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| ee_pose = geom.Transform3D(raw_obs[mapping.OBS_EEF_POS], geom.Rotation.from_quat(raw_obs[mapping.OBS_EEF_QUAT])) | ||
| state = MujocoFrankaState() | ||
| state.encode(raw_obs[mapping.OBS_JOINT_POS], raw_obs[mapping.OBS_JOINT_VEL], ee_pose) | ||
| obs: dict[str, Any] = {'robot_state': state, keys.GRIP: float(raw_obs[mapping.OBS_GRIP])} |
There was a problem hiding this comment.
Rule hardcoded-keys violated:
MolmoAdapter.observations emits the shared canonical 'robot_state' channel as a bare literal while remote_franka_embodiment and the other adapters independently spell the same contract; add a constant such as keys.ROBOT_STATE and use it at every producer and consumer.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| horizon, | ||
| min(timeout, backstop), | ||
| ) | ||
| timeout = backstop if timeout is None else min(timeout, backstop) |
There was a problem hiding this comment.
When --eval.timeout is zero or negative, this accepts it and the harness reaches its deadline before executing a policy step; when it is nan, both deadline comparisons remain false, so even an env-reported terminal is ignored and the runaway backstop never fires. Validate that an explicit timeout is finite and greater than zero before applying min.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11205b2366
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _IK_TOL = 1e-4 | ||
|
|
||
|
|
||
| class _DroidPickEvalConfig(JsonBenchmarkEvalConfig): |
There was a problem hiding this comment.
Rule misleading-name violated:
_DroidPickEvalConfig is used for every manifest task, and its own documentation says the sampler replaces task_type from each episode specification, so Pick falsely narrows this general configuration to one task family; rename it to _DroidEvalConfig or _MolmoEvalConfig and update the parity reference accordingly.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| 'cam_hashes': cam_hashes, | ||
| 'reported_horizon': reported_horizon, |
There was a problem hiding this comment.
Rule hardcoded-keys violated:
'cam_hashes' and 'reported_horizon' form the result contract passed from _drive_positronic to _assert_parity, but both names are repeated as literals at their readers; define named constants beside this writer and use them at both ends so the parity comparison cannot silently drift.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 648228e3ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| from . import protocol | ||
| from .protocol import decode, encode | ||
| except ImportError: | ||
| import protocol # pyright: ignore[reportMissingImports] |
There was a problem hiding this comment.
Rule swallowed-error violated:
The rewritten import block catches any ImportError raised while loading the packaged protocol and then attempts an unrelated flat import, potentially hiding a broken packaged dependency or loading the wrong module; select the packaged or flat shape from __package__ so genuine import failures surface unchanged.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| def _checkout_lock() -> Iterator[None]: | ||
| """Serialize checkout + ``uv sync`` across processes sharing the cache, so a warm-cache fan-out of eval jobs | ||
| mounting one ``~/.cache/positronic/molmospaces`` filesystem does not race a forced checkout against a sync.""" |
There was a problem hiding this comment.
Rule stale-doc violated:
_checkout_lock claims to serialize uv sync, but its only caller performs uv pip install and this launcher explicitly avoids uv sync; describe this as serializing checkout and venv installation so the documented concurrency guarantee matches the protected operation.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| The fidelity check ``docs/architecture.md`` ("Benchmarks are native; adoptions are faithful") mandates for every | ||
| sim-env integration, added here for MolmoSpaces. It drives one pinned benchmark episode twice — natively through |
There was a problem hiding this comment.
Rule diff-comments violated:
The parity module says the check was “added here,” wording meaningful only while reading this change, and points to a nonexistent docs/architecture.md heading; state the durable parity invariant and reference the root ARCHITECTURE.md decision “An adoption loses nothing” instead.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| This asserts fidelity against the pinned ``_MOLMO_COMMIT`` (``launcher.py``); re-run it on any bump of that pin | ||
| before merge — a sim version change can silently shift the horizon or the rollout. |
There was a problem hiding this comment.
Rule stale-doc violated:
The module tells maintainers to rerun the check “before merge,” a transient status instruction that becomes meaningless once the commit lands; keep only the durable requirement to rerun whenever _MOLMO_COMMIT changes.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f853d7c60
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if value <= 0: | ||
| raise ValueError(f'benchmark declares a non-positive {MOLMO_TASK_HORIZON_SEC} of {value}s') |
There was a problem hiding this comment.
When a single-episode manifest contains task_horizon_sec: NaN or Infinity—both accepted by Python's JSON decoder—the value <= 0 check passes and this function returns a non-finite horizon. The eval is then constructed with a non-finite timeout, while the env server later fails during round(sec * 1000 / policy_dt_ms), after its costly setup; validate math.isfinite(value) here so malformed benchmarks fail immediately and clearly.
Useful? React with 👍 / 👎.
| def _assert_measures_at_grasp_site(robot_view: Any) -> None: | ||
| """Fail unless the arm move group's leaf frame is ``mapping.MOLMO_GRASP_SITE``. |
There was a problem hiding this comment.
Rule stranded-definition violated:
_assert_measures_at_grasp_site is used only by MolmoSpacesEnv._build but remains at module scope; move it into MolmoSpacesEnv beside _build, for example as a static method. The later # rules-allow is attached to _leaf_pose, not this definition or its enclosing class, so it does not waive this site.
AGENTS.md reference: AGENTS.md:L24-L25
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32845207a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| mocap bodies, actuator activation, controls and the solver warm-start. Object poses in it let analysis | ||
| recompute success. Positions start at index 1, after the scalar time.""" | ||
| data = self._robot_view.mj_data | ||
| model = data.model |
There was a problem hiding this comment.
On every MolmoSpaces reset, _observe() calls this method, but robot_view.mj_data is a raw MuJoCo MjData object and doesn't expose a model attribute (the repository's generated MuJoCo 3.5 stubs confirm this). Consequently data.model raises AttributeError before the reset frame can be returned, preventing any rollout from starting; obtain the model from the robot view or its arm move group, as the kinematics methods above already do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7417179993
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| comparison adds it rather than the launcher knowing about a check. | ||
| """ | ||
| env = launcher.molmo_subprocess_env() | ||
| return {**env, 'PYTHONPATH': os.pathsep.join([env['PYTHONPATH'], str(Path(__file__).parent)])} |
There was a problem hiding this comment.
Rule hardcoded-keys violated:
molmo_subprocess_env() writes the child environment under bare 'PYTHONPATH', while _native_env() independently reads and rewrites that same key; define the shared environment-variable name once in launcher.py and import it here so the launcher and parity runner cannot drift.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc8cfbc6f4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Every sim-env adoption ships a native-vs-Positronic parity test that drives one pinned episode | ||
| through both stacks and asserts this, re-run on every bump of the sim's pinned version. |
There was a problem hiding this comment.
Rule stale-doc violated:
ARCHITECTURE.md says every sim-env adoption ships a native-vs-Positronic parity test, but a repo-wide search finds such a test only under molmo_spaces/tests/parity.py; the existing LIBERO and RoboLab scripts validate command transforms rather than comparing complete native and Positronic episode runs. Either add parity coverage for those adoptions or phrase this as a requirement for new adoptions instead of a property that already holds.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
Squash of the molmo-env-server branch (PR #504), rebased onto main after the tasks(spec) and keys restructuring: - `MolmoSpacesEnv` answers `tasks(spec)` with the benchmark's episode records; the config no longer reads benchmark.json itself - `MolmoAdapter.task_params` maps records to trial params under the new `molmo_spaces/keys.py` leaf (`eval.episode_index`, `eval.task_horizon`) - The env-server wire vocabulary lives in `protocol` constants on both sides, the `tasks` verb included - `serve_subprocess` waits for the bind and reports a server that dies before it
044d141 to
9401b66
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9401b6693f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **Bandwidth is not the adapter's problem.** The client already downsizes to what the server says | ||
| it needs: every `Codec` advertises its expected input sizes via the reserved `image_sizes` meta | ||
| key (see `Codec.meta`), the server returns it in the session handshake, and the client fits | ||
| frames to it before sending. This is default-on — an adapter that resizes "to keep the wire |
There was a problem hiding this comment.
In the inspected offboard path, InferenceClient only serializes observations and RemotePolicy builds the server-declared local_stack; neither uses image_sizes to resize frames. Downscaling occurs only when the server explicitly declares RestrictImageSize before the remote marker, so maintainers following this “default-on” claim can omit that layer and send full-resolution frames over the wire. Document the RestrictImageSize/local_stack mechanism instead.
Useful? React with 👍 / 👎.
| eval_keys.TASK: record['name'], | ||
| molmo_keys.EPISODE_INDEX: record['episode_index'], | ||
| molmo_keys.TASK_HORIZON: record['task_horizon_sec'], |
There was a problem hiding this comment.
Rule hardcoded-keys violated:
MolmoAdapter.task_params reads the task-list response fields 'name', 'episode_index', and 'task_horizon_sec' as literals while MolmoSpacesEnv.tasks independently writes them in the other interpreter. Define these record-field names in the shared mapping.py module and use them at the producer, consumer, and fixture sites so the task-list contract cannot drift.
AGENTS.md reference: AGENTS.md:L7-L22
Useful? React with 👍 / 👎.
| f'{mapping.MOLMO_BENCHMARK_MANIFEST} or a legacy house_*/episode_*.json layout.{_discovery_hint()}' | ||
| ) | ||
| count = len(self._episodes) | ||
| selection = spec.get('episodes') |
There was a problem hiding this comment.
Rule hardcoded-keys violated:
MolmoSpacesEnv.tasks reads the shared task-spec field 'episodes' as a literal while the eval config independently writes it through spec(episodes=...). Define the selection-field name in the positronic-free mapping.py module and use it on both sides of the subprocess boundary.
AGENTS.md reference: AGENTS.md:L7-L22
Useful? React with 👍 / 👎.
| _SITE_FRAME = 'site' | ||
|
|
||
|
|
||
| def _discovery_hint() -> str: |
There was a problem hiding this comment.
Rule stranded-definition violated:
_discovery_hint exists only to build the empty-benchmark error in MolmoSpacesEnv.tasks, but it is left at module scope well above that sole user. Move it into MolmoSpacesEnv beside tasks, for example as a static method, so the definition remains attached to the entity whose diagnostic it serves.
AGENTS.md reference: AGENTS.md:L7-L22
Useful? React with 👍 / 👎.
| _CAMERA_VARIANTS = { | ||
| mapping.MOLMO_WRIST_CAMERA: mapping.MOLMO_WRIST_CAMERA_VARIANTS, | ||
| mapping.MOLMO_EXTERIOR_CAMERA: mapping.MOLMO_EXTERIOR_CAMERA_VARIANTS, | ||
| } |
There was a problem hiding this comment.
Rule stranded-definition violated:
_CAMERA_VARIANTS is private and used only by MolmoAdapter.observations, so leaving it at module scope separates the lookup table from its sole owning entity. Move it into MolmoAdapter as a class attribute or inline it beside the lookup.
AGENTS.md reference: AGENTS.md:L7-L22
Useful? React with 👍 / 👎.
Take main's `_UrdfRow` rows in `roboarm/models.py`, and carry the 2F-85 grasp site over as one
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abb0bbb18e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1,56 @@ | |||
| # /// script | |||
| # requires-python = ">=3.11" | |||
| # dependencies = ["numpy"] | |||
There was a problem hiding this comment.
Rule hidden-dependency violated:
The documented uv run --no-project positronic/simulator/molmo_spaces/tests/make_fixture.py invocation isolates this script with only NumPy, yet line 23 imports positronic.simulator.molmo_spaces.mapping; uv help run confirms that inline dependency metadata is installed into “an isolated, ephemeral environment,” so a clean checkout fails with ModuleNotFoundError instead of regenerating the fixture. Either make the generator self-contained or remove the inline environment and invoke it through the project environment, as make_replay_fixture.py does.
AGENTS.md reference: AGENTS.md:L7-L22
Useful? React with 👍 / 👎.
|
|
||
| | Layer | Owns | Examples | | ||
| |---|---|---| | ||
| | **Adapter** (per sim/rig, e.g. `simulator/molmo_spaces/adapter.py`) | Rig semantics ONLY: mapping the rig's observation/action vocabulary onto positronic's raw keys | Camera-key mapping, gripper qpos → `[0, 1]` closure, decoded commands → the rig's action format | |
There was a problem hiding this comment.
Rule stale-doc violated:
This required adapter guide says the client-side adapter converts decoded commands into the rig's native action format, but the cited Molmo adoption explicitly forwards the shared tagged command and converts it to Molmo joint targets in MolmoSpacesEnv.step because the server owns the live model and IK. Describe the adapter as producing the shared wire action and the env server as owning controller-native conversion, otherwise new integrations following this table will place model-dependent action logic on the wrong side of the process boundary.
AGENTS.md reference: AGENTS.md:L7-L22
Useful? React with 👍 / 👎.
|
Please include MolmoSpaces in the shared frame migration tracked by #550, coordinated with LIBERO's correction in #557. Declaring The checks should cover several arm orientations and robot placements, recorded-pose/model agreement, and native policy input/action parity through the adapter and codec. Command/observation round trips alone can hide matching frame errors. This is a verification requirement, not a confirmed MolmoSpaces geometry defect. #550 now explicitly includes this integration in its migration scope. |
`main` named the env-server wire vocabulary in #737, and so had this branch. `main`'s spelling wins on every name both sides give the same thing: the request envelope is `CMD`/`SPEC`/`TOKEN`/`ACTION`, the verbs are the `Command` enum, the replies are `OK`/`TASKS`/`ERROR`, and `encode` stays the cast partial. Those names reached only `client.py` and `server.py`, so nothing else on the branch moves. This branch keeps the four families `main` gives no name: the canonical command contract, the action and command fields, the spawn options, and the reply frames. Fourteen files read those, unchanged. `test_remote_env.py` takes both sides whole — `main`'s heartbeat fixture and its two tests, and its parametrized failure test, which also covers a verb the server does not know. `_settle` keeps this branch's body: `main` only reworded the docstring, while the fix here returns the first step's frame, so a caller asking for no settling steps reads a frame rather than `None`. The baseline drops `proxy.py`, whose finding this branch fixes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee4e918289
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Command side: the ``MolmoAdapter`` forwards a joint command (the DROID rig runs the joint-position controller); | ||
| this server integrates it onto the measured joints and steps the per-move-group ``{arm, gripper}`` action. |
There was a problem hiding this comment.
Rule stale-doc violated:
The module says that MolmoAdapter forwards only a joint command and that the server integrates it, but MolmoSpacesEnv.step accepts every canonical command type, passes absolute joints through, and converts Cartesian commands through IK. Describe forwarding the shared tagged command and converting each tag to native joint targets.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
2 participants
Serves AllenAI's MolmoSpaces MuJoCo manipulation benchmark behind the env-server wire, so positronic owns the control loop (per
ARCHITECTURE.md, #503).Shape
molmo_spaces/holds the serving path;molmo_spaces/tests/holds everything that verifies the adoption — the parity check and its native reference, the e2e socket drive, the command-transform validation, the replay test, the fixture builders and unit tests.launcher.py— pinsallenai/molmospaces@c2f1b58and installs themujocoextra into its own venv against a committed constraints file.env_server+ the puremappingmodule ridePYTHONPATH, so molmo's venv installs zero positronic packages.env.py(imports only molmo + mujoco/numpy) — drives oneBaseMujocoTaskper episode:sample_task()builds the sim/scene/renderer, and we drivereset/step/is_done/judge_successin place of molmo'sJsonEvalRunner.adapter.py—MolmoAdapter(WireCommandAdapter), thin like libero's: raw payload →MujocoFrankaState+ grip + camera signals. It ownsDEFAULT_CAMERA_DICT, the logical-name → MolmoSpaces-camera pairing the eval config and the checks read.mapping.py— the framework-free wire mappings and this adoption's own names: the payload's fields, the reset token, MolmoSpaces' camera/move-group names, the grasp site.env_server/protocol.py— the vocabulary every adoption shares, imported from both interpreters;libero/env.pyandrobolab/env.pystill write the frame themselves (Positronic-Robotics/internal#220).serve_subprocessnow waits for the port to accept, so a server that raised at startup is distinguishable from one still loading assets.cfg/eval/sim/molmo.py— one CLI:--eval.benchmark_dirnames the benchmark and sweeps its episodes into positronic datasets.What the env owns
--eval.task_horizon_steps, else the benchmark'stask_horizon_sec, else a loud failure. The task enforces it, and reports it at reset so a too-shortTask.timeoutfails loud.MjDatacopied from the live buffer so the stepped sim is never perturbed.mjSTATE_INTEGRATION, the complete integrable state, reaches the recorder and never the policy, so success can be recomputed offline.The command contract is total:
tests/validate.pydrives every type inprotocol.CANONICAL_COMMAND_TYPESthrough the real conversion against a live scene (libero and robolab owe the same —Positronic-Robotics/internal#149,#150). The rig measures and drives at MolmoSpaces' grasp site, so that is where its model declaresDEFAULT_FRAME; a policy frame is aChangeEEFrameaway, as #485 intends.Validation
1488 passed / 11 skipped without molmo or a GPU,
ruffand the basedpyright ratchet clean. Parity derives what it compares from upstream — its observation fields and gripper closure transcribed, not read frommapping— so it cannot compare code with itself.The asset-dependent checks have not run on this head: parity drove episode 3 natively and through the wire, and a pi05 rollout replayed at zero deviation, both earlier; the packs are absent from the box that runs them (
internal#476). Neither reachesEnvAdapter— both stepEnvConnectiondirectly — so main's move of trajectory playing into the harness (#591) does not change what they replay.