Skip to content

Artifact readiness, and five defects it turned up - #1

Merged
tactino merged 24 commits into
mainfrom
chore/artifact-readiness
Sep 11, 2026
Merged

tactino merged 24 commits into
mainfrom
chore/artifact-readiness

Conversation

@tactino

@tactino tactino commented Sep 10, 2026 •

Copy link
Copy Markdown
Member

Artifact-readiness work, and five defects it turned up. Two of them stopped
the package doing its job at all.

Three environment families could not start

atari, classic and robomimic never defined an action space. The
rollout refactor of 2026-03-20 introduced _get_action_spec, which requires
one, so:

$ plugrl-run-env-client classic-v1
ValueError: Env must expose action space via single_action_space/action_space

Nothing caught it because the only environment the test suite exercised was
the one that happened to define a space. classic-v1 now completes episodes,
and tests/test_action_space_contract.py constructs every registered
environment and makes the same call rollout makes. (d4rl defines it as a
property, which is easy to miss when grepping for an assignment — three
families, not four.)

_get_action_spec was guessing at a batch axis

It asked whether the leading axis of single_action_space happened to equal
num_envs, and stripped it if so. But single_action_space is unbatched by
Gymnasium's own definition, and every environment family here declares it
that way — so the branch could only ever be a false positive. It fires
whenever an action dimension coincides with the environment count, which for
a 7-DoF arm means --num-envs 7, and silently drops a real dimension.

Found by writing tests for the protocol's exchange rules, where two
environments with a two-dimensional action hit it immediately.

A finished run ended in a traceback

Running a server for N steps and pointing a client at it is the documented
happy path. It ended like this:

plugrl_env_client.agent.websocket_env_client_agent.ServerStopped:
Server requested env client shutdown after algorithm stop.

exit 1, on a run where the server had just saved its checkpoint and shut
down cleanly. ServerStopped is a normal end condition — its own docstring
says so. Worse under run_multiprocess: a non-zero child exit makes the
parent terminate its siblings, so one process seeing the stop first tore the
others down mid-episode. Anything that is not a server stop still
propagates, and there is a test for that.

There was no seeding at all

Bare env.reset(), no seed on RunnerArgs, DummyEnv drawing from global
np.random. "Three seeds per configuration" was not something this package
could deliver. Now: --runner.seed, threaded to the first reset only so the
whole rollout is reproducible rather than just its first episode, and
derive_process_seed gives each client process a disjoint slice — handing
every process the same base seed would have them replay one trajectory,
which looks like data collection and is not.

robomimic raises on a seed rather than accepting one, because robosuite's
randomness is not reachable from there. Refusing beats pretending.

A bare install could not import

Optional extras leaked into the base install path, and a dead
remote_viewer_wrapper imported an undeclared PIL. Both fixed; every git
dependency is now pinned to a full commit SHA, so a fresh clone resolves to
what was tested.

Tests worth reviewing rather than skimming

  • test_terminal_observation.py — BaseEnv declares
    AutoresetMode.NEXT_STEP, which is a promise rollout depends on: the done
    step returns the terminal observation. An environment that resets inside
    its own step() breaks it silently, and the only symptom is corrupt
    training data. None of the six do; these tests pin that, and a deliberately
    misbehaving fake demonstrates the corruption executably.
  • test_protocol_alternation.py — makes three claims from
    plugrl-protocol/SPEC.md observable rather than derived, by recording every
    message rollout would put on the wire: strict alternation; a feedback env
    set that need not match the infer's, and can name an environment the infer
    did not; chunk-summed reward. Each has a companion test for the
    configuration that hides it — equal episode lengths keep the sets equal,
    and a horizon of 1 makes chunk reward equal step reward.

Also

Apache-2.0, CI (lint + test), CITATION.cff, README corrected to match the
package, rollout timing exported to summary.json with per-stage fractions,
and the server's metadata message logged at startup with a warning when
replan_steps exceeds the reported action horizon.

Tests: 112 passing, 8 skipped.


🤖 Generated with Claude Code

tactino and others added 9 commits September 9, 2026 15:32
The repos shipped with no license at all, which makes them legally unusable
and is a straight deduction in artifact review. Apache-2.0 matches openpi and
lerobot, the two Apache-2.0 dependencies, and adds a patent grant.

Also fills in the empty package descriptions and raises the setuptools floor
to 77, the first release that understands the PEP 639 license fields used here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three related problems, all found by the new import test:

- utils/wrappers/__init__.py eagerly imported remote_viewer_wrapper, which
  imports PIL. Pillow is not a declared dependency - it only arrives
  transitively via imageio - and the wrapper itself is unreachable: the CLI
  no longer has --use-remote-viewer or any other entry point for it. Deleted.

- d4rl_env.py imported the legacy `gym` package above its try block, so a
  bare install got "No module named 'gym'" instead of the message on the next
  line telling the user to install the d4rl extra. Moved inside the guard.

- test_cli_main_keeps_top_level_env_for_runtime drove the d4rl-v1 subcommand,
  which only exists once that env registers, so it failed on any machine
  without the d4rl extra. Now skips, matching test_libero_env.

The new test encodes both invariants: core modules import on a bare install,
and optional modules fail with a message naming the extra they need.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The forks these depend on were referenced by bare branch, so a rebase or
force push on any of them would silently change what gets installed and make
past results unreproducible. lerobot was pinned to a 7-character prefix,
which can go ambiguous as a repository grows; expanded to the full 40.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Dropped the "Vision Language Action Reinforcement Learning Infrastructure"
  expansion, left over from when this was called VLARL.
- Removed the Remote Viewer section: it documented a --use-remote-viewer flag
  the CLI no longer has and a plugrl-viewer project that exists in no repo.
- Completed the environment table, which listed 4 of the 6 registered envs and
  omitted d4rl and libero, and added the extra each one needs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CITATION.cff validates against CFF schema 1.2.0. The author list is taken from
pyproject.toml and lists one person; co-authors should be added before any
submission.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing on the environment side was seeded. rollout() called a bare
env.reset(), RunnerArgs had no seed field, and DummyEnv drew from the global
np.random - so two runs of the same configuration produced different
trajectories and no result could be reproduced. "Three seeds per
configuration" was not something this package could deliver.

- RunnerArgs.seed, surfaced as --runner.seed, threaded through run() into
  rollout()'s first reset. Later resets continue the stream the env
  established, so the whole rollout is reproducible rather than just its
  first episode.
- derive_process_seed gives each client process a disjoint slice, spaced by
  num_envs. Handing every process the same base seed would have them replay
  one trajectory N times, which looks like data collection and is not.
- BaseEnv.seed_rngs sets self.np_random via gymnasium's mechanism. Envs must
  draw from that rather than np.random, which is global state nothing owns.
- DummyEnv and LiberoEnv now do. atari, classic and d4rl already forwarded
  the seed to the gym env underneath.
- RobomimicEnv raises when given a seed. The robosuite simulation underneath
  is not reachable from this wrapper, and a run that looks reproducible but
  is not is worse than one that refuses to start.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rollout loop already measured where its time went, but only printed it as
a log line - readable for one run, useless for comparing ninety. Comparing
configurations across a sweep meant parsing logs.

Collects the six loose float counters into a RolloutTiming dataclass and
writes it into the run summary the Recorder already produces. Adds per-stage
fractions, which are the part worth reading: whether a configuration is bound
by the environment, by waiting on the server, or by packing observations
differs by orders of magnitude across environment families, and throughput
alone does not show it.

A seeded dummy-v1 run now reports 89% infer_wait, 9% env_step - as expected
when the environment is trivial, and exactly the comparison that was
previously unavailable.

feedback_obs_pack and feedback_info_pack are excluded from the denominator;
they are already inside feedback and would otherwise be counted twice.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rollout() calls _get_action_spec() before its first step to size the action
plan. It reads single_action_space, falls back to action_space, and raises
ValueError when neither exists. Atari, Classic and Robomimic defined neither,
so those envs could not start:

  $ plugrl-run-env-client classic-v1
  ValueError: Env must expose action space via single_action_space/action_space

Not a regression - they never had one. _get_action_spec arrived with the
rollout refactor in March and three of the six environment families never
satisfied the requirement it introduced. The test suite only ever constructed
dummy-v1, which happened to define a space, so nothing noticed.

Classic and Atari take the space from the gym env they wrap. Robomimic builds
a Box from EnvRobosuite.action_dimension; only shape and dtype are read, and
robosuite normalises actions to [-1, 1] anyway. d4rl already exposed one as a
property and is unchanged.

classic-v1 now completes an episode against a real server. Robomimic is
written from its documented API but not run here - it needs MuJoCo.

The new contract test covers every registered env rather than a chosen one,
and skips those whose extras are absent. That skipping is itself the blind
spot that hid this: in a bare dev venv only dummy-v1 registers, so the test
is only as good as the environment it runs in. CI or a sweep venv with extras
installed is where it earns its keep.

Also makes BaseEnv.fake_action/fake_obs raise instead of returning None -
same class of silent failure, reported far from its cause - and gitignores
checkpoints/, which a server started from this directory writes into.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tactino
tactino force-pushed the chore/artifact-readiness branch from 2d9f577 to c0dce52 Compare September 10, 2026 16:04
tactino and others added 5 commits September 10, 2026 12:05
pre-commit builds its hook environments with a system interpreter, and
setup-uv does not provide one, so the hooks failed with:

  RuntimeError: failed to find interpreter for Builtin discover of
  python_spec='python3.10'

This repository's .pre-commit-config pins default_language_version to
python3.10, while .python-version says 3.11. CI installs 3.10, matching what
pre-commit actually asks for rather than quietly picking a side - but the two
files disagreeing is worth reconciling separately.

plugrl-protocol's lint passed throughout because its config has no such pin.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
plugrl-protocol is public now, so uv can fetch it without credentials and the
conditional git-config step has nothing left to do. plugrl-server dropped the
same step; this one was missed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BaseEnv declares autoreset_mode NEXT_STEP, which is a promise: the step that
reports done returns the terminal observation, and the reset happens later.
rollout() depends on it - feedback is sent with what step() just returned, and
only then does reset() run.

An env that resets inside its own step() breaks that promise silently. Nothing
raises; the terminal transition simply carries the first observation of the
next episode, which is corrupt training data that nobody sees.

Checked every env in this package: none of them do it. Each delegates to its
backend and returns what it gets, and the Atari EpisodicLifeEnv marks a lost
life terminal without resetting - the promise holds today. These tests keep it
that way:

- the declared mode is pinned, on BaseEnv and on all six subclasses, because
  an env quietly switching to SAME_STEP would make rollout wrong for it alone
- a deliberately misbehaving fake env demonstrates the corruption end to end,
  so the failure mode is executable rather than described

The second test asserts the bad env *does* leak its reset observation. That
reads oddly, and the docstring says why: if rollout later grows a defence
against misbehaving envs, the test should be inverted rather than deleted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
single_action_space is unbatched by Gymnasium's own definition, so asking
whether its leading axis happens to equal num_envs can only ever be wrong.
It fires as a false positive whenever an environment's action dimension
coincides with the number of environments - for a 7-DoF arm, --num-envs 7 -
and silently drops a real dimension. rollout() then either fails with
"Expected action shape tail (), got (7,)" or, for a one-dimensional action,
sizes the action plan with an empty shape and does not fail at all.

Every environment family in this package declares single_action_space
unbatched, so the stripping branch has never once been correct there. The
action_space fallback keeps it: on a Gymnasium vector env that attribute
really is the batched space.

Found by writing tests for the protocol's exchange rules, where two
environments with a two-dimensional action hit the collision immediately.

Also adds tests/test_protocol_alternation.py, which makes three claims from
plugrl-protocol/SPEC.md observable rather than derived, by recording every
message rollout() would put on the wire:

  * infer and feedback strictly alternate, and at most one infer is ever
    outstanding - the server's handler is straight-line code with no
    dispatcher, so a client that sends two infers in a row has the second
    parsed as a feedback and is disconnected;
  * a feedback's environment set need not match that of the infer it
    follows, and can name an environment the infer did not. One early
    termination is enough to desynchronise two environments permanently.
    An implementation that indexes feedback by position in the action it
    just received corrupts exactly here;
  * the reward reported for a chunk is the sum over the chunk, not the last
    step's - reporting the last step's would train a different MDP without
    failing anything.

Each has a companion test for the configuration that hides it: equal
episode lengths keep the env sets equal, and a horizon of 1 makes chunk
reward equal step reward. Those are the settings under which the hazards
are invisible.

Corrects the count in test_action_space_contract.py's docstring: three
families lacked an action space, not four. d4rl declares it as a property.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running a server for N steps and pointing a client at it is the documented
happy path, and until now it ended like this on the client:

    plugrl_env_client.agent.websocket_env_client_agent.ServerStopped:
    Server requested env client shutdown after algorithm stop.

exit code 1, on a run where the server had just saved its checkpoint and
shut down cleanly. ServerStopped is a normal end condition - its own
docstring says so - and the server reaches it by taking every step it was
asked for. run() now treats it as one, logs how far collection got, and
returns.

The multiprocess path made this worse than cosmetic: a non-zero child exit
makes run_multiprocess terminate its siblings, so one process seeing the
stop first tore the others down mid-episode.

Everything that is not a server stop still propagates. A client that died
has to look different from one that finished, and there is a test for it.

Also uses the metadata message, now that plugrl-server fills it in. The
client logs what it connected to, so a run records the policy and action
shape it collected against, and warns when replan_steps exceeds the
reported action_horizon - which currently fails inside rollout, but only
after a full inference round trip, with a message that mentions neither
the policy nor the horizon.

That check warns rather than raises. SPEC.md section 5.1 says a client
must not require any metadata key, so an absent, stale or wrong-typed
action_horizon has to stay survivable; the real check is still the one in
rollout, against the chunk that actually arrives.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tactino tactino changed the title Artifact readiness: licence, CI, reproducible deps, and fixes found on the way Artifact readiness, and five defects it turned up Sep 10, 2026
tactino and others added 10 commits September 10, 2026 19:30
Two defects of the same kind as the openpi one already fixed in
plugrl-protocol.

The package pinned plugrl-protocol at c7961da - what `main` still points at,
and a tree with no LICENSE and no NOTICE. So an install got msgpack_numpy.py
with neither a licence nor the openpi attribution regardless of what the
source branch says. Repinned to a64ef63 and relocked with `uv lock`. Repin to
a main commit once plugrl-protocol#1 merges.

NOTICE named only PlugRL, while envs/atari/atari_wrappers.py is adapted from
Stable-Baselines3 and carries their MIT text in its header. NOTICE now records
it.

CITATION.cff claimed PlugRL exists so that environments whose dependencies
"cannot coexist with a modern deep learning stack" can be used - the claim
plugrl-server's own E1 experiment disproved. Replaced with what survived.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This package could not show a policy learning anything. Of six environment
families, one is discrete-action, three need assets or a Linux-only stack,
and the dummy one has no reward to learn from - so there was nothing to point
a continuous-control policy at.

`MuJoCo-v1` fills that gap. It is dense-reward continuous control, needs no
assets, no display and no GPU, and runs on Windows. `HalfCheetah-v5` has a
17-dimensional observation and a 6-dimensional action, which are exactly
`FPOPolicyConfig`'s defaults, so the shipped flow policy points at it with no
config surgery.

An unregistered draft existed at `examples/mujoco/mujoco_env.py`. It could
not have run: it defined no action space, which is precisely the defect that
stopped atari, classic and robomimic starting, and `_get_action_spec` would
have raised before the first step. This version also renders only when asked
- a state-only policy never looks at the frames, and producing them costs
more per step than the physics does. SPEC.md section 5.2 allows an
observation with no images, so they simply are not sent.

## The bug that had to be fixed first

`robomimic_env.py` set `MUJOCO_GL=egl` unconditionally on line 3 and failed
its own import on line 10. Both halves matter:

* `plugrl_env_client.envs` imports every `*_env.py` it can find so their
  registration decorators run, and downgrades ImportError to a warning. So
  line 3 ran on every platform, for everyone, even though robomimic was not
  installed - and the caught ImportError left the value behind.
* `egl` exists only on Linux. On Windows the legal values are wgl, glfw and
  osmesa, and `mujoco` raises `RuntimeError: invalid value for environment
  variable MUJOCO_GL: egl` as soon as anything touches its rendering module.

So an uninstalled environment family was killing every other MuJoCo-based
environment in the process. It is now set only on Linux, and only with
`setdefault`, so a caller who chose glfw keeps it.

`tests/test_env_import_side_effects.py` pins this in a subprocess, because by
the time an ordinary test runs the import has already happened.

## Verified end to end

A real `plugrl-run-server fpo-policy` and a real `plugrl-run-env-client
mujoco-v1` completed 3 episodes of HalfCheetah-v5 over 3000 steps. The server
reported `{'algorithm': 'FPOAlgorithm', 'policy': 'FPOPolicy',
'action_dim': 6, 'action_horizon': 1}` in its metadata, ran an FPO update on
the collected data - 46 scalar tags with real values, `cfm_loss_mean` 0.5692,
`policy_ratio_mean` 0.9996 - and wrote a checkpoint containing
`model.safetensors`. Both processes exited 0.

Tests: 130 passing, 9 skipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The environment table omitted it. It is the only family here that is
dense-reward continuous control needing no assets, no display and no GPU,
and its default task HalfCheetah-v5 is obs 17 / act 6 - exactly
plugrl-server's fpo-policy defaults, so the pair runs unconfigured.

Also records that rendering is off unless asked for: a state-only policy
never looks at the frames, and producing them costs more per step than the
physics does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The import not raising is the whole test, and pytest does enforce that, but
an empty body reads as an oversight and invites deletion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`uv sync --extra robomimic --extra atari --extra classic --extra libero`
omitted mujoco, which is the environment every quickstart now uses. Someone
following that line would not have been able to run the example.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MUJOCO_LOG.TXT has been in the repository since fa9b38f in December 2025.
It is 2676 lines of simulator warnings that MuJoCo writes beside whatever
directory it was started in - including a few hundred repetitions of
"WARNING: Nan, Inf or huge value in CTRL at ACTUATOR 0. The simulation is
unstable."

Untracked and ignored. It stays in history, which is the right trade
against rewriting a branch other people have.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit untracked it but the ignore rule did not make it into
the same commit, so the next MuJoCo run would have re-added it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI caught two things this machine could not, because it has mujoco installed
and CI does not.

The first is a real bug in yesterday's fix. Guarding the assignment by
platform stopped it breaking Windows, but on Linux it still ran on every
machine, for everyone, because plugrl_env_client.envs imports every *_env.py
it can find and downgrades the resulting ImportError to a warning. So an
environment family nobody installed still chose a rendering backend for the
whole process. It now checks find_spec("robomimic") first and raises before
touching os.environ, which is also where the not-installed message belonged.

The test that caught it was itself wrong: it asserted MUJOCO_GL is never set,
while the fix deliberately still sets it on Linux when robomimic is present,
which robosuite needs. The test now states the real contract - a family that
is not installed must not set it - and skips where robomimic is.

The second is smaller: the new mujoco env module was not in
OPTIONAL_ENV_MODULES, so test_package_imports treated it as core and required
it to import on a bare install. It is behind the mujoco extra like the other
five.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The citation and package metadata named one author. The commit history of
these three repositories names two: Chenhao Lu, and Zuo Gou, whose 31
commits here include the GRPO-diffusion policy and the policy-gradient base
class that DPPO, FPO and PI0 all derive from - and who is the largest
contributor to plugrl-protocol, at 12 commits to 9.

Order is by who started the project, not by volume; reorder if you would
rather it were otherwise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
plugrl-protocol#1 is merged, so the pin can point at a commit on main rather
than one on a feature branch.

Verified on the installed artifact rather than the source tree, because that
is where this was broken: a fresh sync now produces a plugrl_protocol
distribution containing LICENSE and NOTICE, and an msgpack_numpy.py whose
header attributes the file to Physical Intelligence. Until today an install
got none of the three no matter what the branch said.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tactino
tactino marked this pull request as ready for review September 11, 2026 15:17
@tactino
tactino merged commit 5f35a8a into main Sep 11, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant