Route raw observation keys through a canonical positronic.keys module - #505
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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 |
![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: 057298fbdd
ℹ️ 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". |
Define the wire's raw observation-signal keys once and migrate every consumer, so a rename is a single-site change instead of a literal duplicated across codecs, evals, configs, adapters and datasets. Ticket: Positronic-Robotics/internal#90 #refs
The keys migration line-shifted 20 baselined basedpyright errors in the touched test files; fix them rather than re-baseline (no grandfathering). Narrow `dataset[0]` to `Episode` at the two Episode-access sites, type `last_obs` as `dict[str, Any]`, and pass a mapping (not a list) for the harness state key. The resolved entries drop out of the ratchet baseline, which shrinks by 20. Ticket: Positronic-Robotics/internal#90 #refs
…e baseline The `basedpyright` ratchet runs in a dev-only `.venv-typecheck` (base + dev, `--exact`) so the check is deterministic regardless of which optional extras a contributor synced. Under that env `torch` and `lerobot.*` are unresolved, but three of those foreign-import errors (`cfg/policy.py`, `simulator/robolab/env.py`, `simulator/robolab/validate.py`) were absent from the baseline — it had been regenerated in an env where those extras resolved, so the entries auto-pruned. Recapture them in the canonical env. Pre-existing on `main`; CI does not run the ratchet, so it went unnoticed. No source change. Ticket: none pre-existing baseline drift surfaced while landing internal#90
Add `test_no_raw_observation_key_literals`: an AST walk over the package that fails if any namespaced observation key (`robot_state.q`/`dq`/`ee_pose`, `image.wrist`/`exterior`) appears as a bare string literal instead of the `positronic.keys` constant. Matching exact `ast.Constant` values skips docstring examples and comments, so it flags only live code. `keys.GRIP`/`keys.TASK` are not guarded — their bare-token values are reused across the action-command, vendor-state and scene/reset namespaces, so a literal match cannot distinguish the observation key from those. The guard caught one occurrence the migration missed (`image.wrist` in `test_wrappers.py`); migrate it to `keys.WRIST_IMAGE`. Ticket: Positronic-Robotics/internal#90 #refs
…s literal Addresses two Codex findings that both stem from `'task'` naming several concepts. Split them along the read/write seam: - Recorded-episode task FIELD (written by `data_collection` as `keys.TASK`) is now read through `keys.TASK` everywhere it is queried, grouped or filtered: `cfg/analysis.py`, `cfg/ds/internal.py` filter, `cfg/policy.py` group_fields, `cfg/server.py`/`cfg/phail/v1_0.py` group readers, `gui/eval.py`, `server/positronic_server.py`. So a `keys.TASK` rename now propagates to every consumer instead of silently breaking analysis/filters/grouping (finding 1). - Output/display/model field NAMES stay literal `'task'` — the LeRobot model field (`ObservationCodec.task_field` default, reverted from `keys.TASK`), the server's display-column registries and group-result output keys. These are a separate schema bridged from the recorded field by the read, mirroring `encode`'s `obs['task'] = inputs[keys.TASK]` (finding 2: a wire-key rename must not rename the model field). Ticket: Positronic-Robotics/internal#90 #refs
The module must stay a dependency-free leaf so an out-of-repo consumer can import it without pulling in the rest of positronic (or its optional torch/lerobot deps). Assert its AST carries no import statement. Requested-by: Vladimir Yakunin Ticket: Positronic-Robotics/internal#90 #refs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 262f52466c
ℹ️ 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".
The repo-wide keys migration missed the two `utilities/` consumers (`convert_ds.py`, `fake_dataset_generator.py`) that still spelled the five namespaced observation keys as literals. Migrate them and extend the reintroduction guard to scan `utilities/` alongside `positronic/`, so the single-source-of-truth invariant holds across all first-party code. Requested-by: Vladimir Yakunin Ticket: Positronic-Robotics/internal#90 #refs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0cae79056
ℹ️ 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".
1 participant
Defines the wire's raw observation-signal keys once in a leaf
positronic/keys.pyand migrates every consumer, so a rename is a single-site change the type checker propagates instead of a literal duplicated across codecs, evals, configs, adapters and datasets. This is internal#90 — filed from #495's review, where the molmo adapter had to redefine the keys as its own module-level constants because nothing was importable.The seven keys:
JOINTS/JOINT_VEL/EE_POSE/GRIP/TASK/WRIST_IMAGE/EXTERIOR_IMAGE.Scope: repo-wide (every occurrence)
~301 literal→constant substitutions across 44 files (30 source, 14 test) — value positions, config-dict keys (
state_features={keys.JOINTS: 7}), codec param defaults,.override(tgt_joints_key=...), camera dicts, obs-dict output keys,binarize_grip.from positronic import keysat top level (leaf module, no imports — no circular risk, mirrorsfrom positronic import geom);as obs_keysin the four files that already bindkeys.The
grip/taskfalse-positive line (the risk of "every occurrence")Only the exact literal naming the observation signal was migrated. Deliberately left as literals (different namespaces):
{'command': …, 'grip': …}step payloads,action['grip'], env-server raw obs (libero/robolabenv.py/validate.py/e2e.py,test_remote_env.py).keys.GRIPis the observation grip, not the gripper command.state['grip'], modality dicts) — vendor-encoded, siblings aren't canonical either.task(proxy.meta['task'], reset tokens, env-scene sim), grouping/server-displaytask(group_fields), generic test-datatask, vendor codec-outputtask(fixed lerobot column / vendor contract).robot_state.q/ee_poseattribute accesses and error strings.Tests
ruff checkclean.pytest --no-cov: 676 passed, 7 skipped, 1 failed — the failure (pimm/tests/test_world.py::test_background_process) is a pre-existing fork-in-multithreaded flake, unrelated (0 pimm files in the diff; passes in isolation, verified).Recut from #500's
keyswork without thepackages/positronic-clientsplit (parked until a real out-of-repo consumer exists). Do not merge — pending review.