Repository navigation
fix(cli): route non-RL scene demos away from the RL play helper (P8/P9) - #154
Merged
Merged
Conversation
P8/P9: `genelab play GeneLab-Rubiks-Play-v0 --agent zero --steps 5` (and the Wuji equivalent) crashed in env construction — first on `cfg.device` (fixed superficially in #153), then on `cfg.decimation`, etc. Root cause is architectural, not a missing field. Rubiks/Wuji are force-driven scene-playback demos whose cfgs subclass the non-RL base ManagerBasedEnvCfg and have their own RegisteredTask.play() → RubiksPlayEnv/WujiHandPlaybackEnv.play() that builds a Genesis scene directly (no managers, no policy). `_dispatch_play` already routes to task.play() when no RL flags are present, so the bare command worked — but RL-only flags (--agent/--checkpoint/--num-envs/--prof) forced the fall-through to play_task → build_env → ManagerBasedRlEnv, which requires the full RL cfg surface these demos don't have. Bolting more fields onto the base cfg would only move the crash to the next field. Fix at the routing layer: gate RL routing on whether the play env cfg is a ManagerBasedRlEnvCfg. Non-RL tasks always run their own .play(); RL-only options are meaningless for a fixed scene demo, so they are warned-and-ignored rather than crashing. --steps/--vis/--gpu and dotted overrides still apply (they are resolved into the cfg before dispatch). Defense-in-depth: build_env now raises a clear TypeError naming the required surface instead of a cryptic AttributeError. Tests: - test_examples_{rubiks,wuji}: exercise the real `genelab play … --agent zero --steps 5` dispatch path with Genesis stubbed (headless), asserting the scene demo runs, honors --steps, and is NOT routed through play_task. These guard the actual P8/P9 bug, which the prior #153 cfg-only tests did not. - test_cli_routing: non-RL task + --agent runs the demo with a warning; build_env rejects a non-RL cfg. Added an RL-capable fake task so the existing agent-routing tests still validate the RL path (they previously asserted the buggy contract on a non-RL cfg). Docs: clarify in cli/play-train.{en,zh} that policy options apply only to RL tasks; scene demos run built-in playback.
Wrap the External-Fake-RL-Task-v0 play args list to satisfy ruff format --check.
KraHsu
added a commit
that referenced
this pull request
Jun 5, 2026
fix(cli): route non-RL scene demos away from the RL play helper (P8/P9)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
genelab play GeneLab-Rubiks-Play-v0 --agent zero --steps 5and the Wuji equivalent crash during env construction. After PR #153 addeddeviceto the base cfg, the crash just moved one field along:Corrected root cause (architectural, not a missing field)
Rubiks/Wuji are force-driven scene-playback demos. Their cfgs subclass the non-RL base
ManagerBasedEnvCfgand have a completely different shape (robot: RubiksRobotCfg,interaction,force_controller,reset_interval— not the RL surface). They run via a dedicated path:RegisteredTask.play()→RubiksPlayEnv.play()/WujiHandPlaybackEnv.play(), which builds a Genesis scene directly (no managers, no policy)._dispatch_play(cli/_dispatch.py) already routes totask.play()when no RL flags are present — sogenelab play GeneLab-Rubiks-Play-v0with no args works. The bug: RL-only flags (--agent/--checkpoint/--num-envs/--prof) force the fall-through toplay_task→build_env→ManagerBasedRlEnv, which requires the full RL cfg surface (decimation,episode_length_s,actions_cfg, …) these demos don't have. Continuing to bolt fields onto the base cfg (as #153 did fordevice) only moves the crash to the next field.Fix (Option A — gate routing on RL-capability)
_dispatch_playnow routes through the RL play helper only when the play env cfg is aManagerBasedRlEnvCfg. Non-RL tasks always run their own.play(). RL-only options are meaningless for a fixed scene demo, so they are warned-and-ignored rather than crashing:--steps/--vis/--gpuand dotted config overrides still apply to both kinds of task (they're resolved into the cfg before dispatch), so--steps 5correctly runs the demo for 5 steps.Defense-in-depth:
build_envnow raises a clearTypeErrornaming the required surface if ever handed a non-RL cfg, instead of a crypticAttributeErrordeep insideManagerBasedRlEnv.__init__.Considered and rejected: (B) converting Rubiks/Wuji into
ManagerBasedRlEnvCfgsubclasses — they aren't RL (the cube isn't an actuated articulation); (C) removing them from the registry — they're working, runnable demos.Reference audit
find_referencing_symbols(build_env)— all callers are RL paths, so the type-guard is safe:ManagerBasedRlEnv.__init__reads the full RL surface (device,decimation,episode_length_s,scale_rewards_by_dt,actions_cfg,observations_cfg,commands_cfg,rewards_cfg,terminations_cfg,events_cfg,curriculum_cfg,metrics_cfg) — none of which exist on the baseManagerBasedEnvCfg. ConfirmedRubiksEnvCfg/WujiEnvCfgsubclass the base, not the RL cfg.Tests
tests/test_examples_{rubiks,wuji}.py— exercise the realgenelab play … --agent zero --steps 5dispatch path with Genesis stubbed out (headless): assert the scene demo runs, honors--steps, and is not routed throughplay_task. These guard the actual P8/P9 bug, which the fix(envs): move device cfg field onto base ManagerBasedEnvCfg #153 cfg-only tests never touched (false confidence — they never invokedgenelab play).tests/test_cli_routing.py— non-RL task +--agentruns the demo with a warning;build_envrejects a non-RL cfg with a clearTypeError.tests/fake_extension.py— added an RL-capable fake task (External-Fake-RL-Task-v0, env subclassesManagerBasedRlEnvCfg). The existing agent-routing tests (test_cli_routes_agent_through_play_task,test_play_invalid_agent_falls_back_to_picker,test_play_task_argument_preserves_dashed_runner_flags) now point at it — they previously asserted the buggy contract on a non-RL cfg.uv run pytest -q→ 506 passed (headless). The new example tests also pass in isolation (hardened against a pre-existing ordering fragility: fakingsys.modules["genelab.rl"]breaks entry-point loading ofgenelab_unitree, so the new tests monkeypatch theplay_taskattribute on the real module instead).GPU verification still recommended:
Docs
docs/cli/play-train.{en,zh}.md: policy options (--agent/--checkpoint/…) apply only to RL tasks; non-RL scene demos run their built-in playback.🤖 Generated with Claude Code