diff --git a/docs/cli/play-train.en.md b/docs/cli/play-train.en.md index bfa69421..c816851e 100644 --- a/docs/cli/play-train.en.md +++ b/docs/cli/play-train.en.md @@ -19,6 +19,13 @@ Policy sources: | `random` | Uniform random actions in `[-1, 1]`. | | `trained` | Load a checkpoint and use the runner inference policy. | +The policy options (`--agent`, `--checkpoint`, `--num-envs`, `--prof*`) apply only to RL +tasks — those whose play env config is a `ManagerBasedRlEnvCfg`. Non-RL **scene-playback +demos** (e.g. `GeneLab-Rubiks-Play-v0`, `GeneLab-Wuji-Hand-Playback-v0`), whose config +subclasses the base `ManagerBasedEnvCfg`, run their own built-in playback; passing those +options prints a warning and they are ignored. `--steps` / `--vis` / `--gpu` and dotted +config overrides still apply to both. + Checkpoint replay: ```bash diff --git a/docs/cli/play-train.zh.md b/docs/cli/play-train.zh.md index d3c38ac9..5b8f47ad 100644 --- a/docs/cli/play-train.zh.md +++ b/docs/cli/play-train.zh.md @@ -18,6 +18,12 @@ uv run genelab play TASK_ID --agent random --steps 128 | `random` | `[-1, 1]` 均匀随机动作。 | | `trained` | 加载 checkpoint 并使用 runner inference policy。 | +策略相关选项（`--agent`、`--checkpoint`、`--num-envs`、`--prof*`）仅适用于 RL 任务，即 play +环境配置为 `ManagerBasedRlEnvCfg` 的任务。对于配置继承自基类 `ManagerBasedEnvCfg` 的非 RL +**场景回放示例**（如 `GeneLab-Rubiks-Play-v0`、`GeneLab-Wuji-Hand-Playback-v0`），将运行其 +自带的回放逻辑；传入这些选项会打印告警并被忽略。`--steps` / `--vis` / `--gpu` 以及点号配置 +覆盖对两类任务都生效。 + checkpoint 回放： ```bash diff --git a/src/genelab/cli/_dispatch.py b/src/genelab/cli/_dispatch.py index 7b05fa99..f6b616d1 100644 --- a/src/genelab/cli/_dispatch.py +++ b/src/genelab/cli/_dispatch.py @@ -17,6 +17,7 @@ from __future__ import annotations import os +import sys from pathlib import Path from typing import TYPE_CHECKING, Any, Final, cast @@ -65,6 +66,22 @@ def _coerce_prof_kwargs(prof_args: dict[str, str]) -> dict[str, Any]: } +def _is_rl_play_cfg(env_cfg: Any) -> bool: + """Whether ``env_cfg`` can back the RL play helper (``play_task`` → ``build_env`` → + ``ManagerBasedRlEnv``). + + Scene-playback demos (Rubiks, Wuji, downstream non-RL tasks) subclass the *non-RL* + base ``ManagerBasedEnvCfg`` and lack the RL surface (``decimation``, + ``episode_length_s``, ``actions_cfg``, …), so they must run their own ``task.play()`` + instead — routing them through the RL helper crashes in env construction. See P8/P9. + """ + if env_cfg is None: + return False + from genelab.envs.manager_based_rl_env import ManagerBasedRlEnvCfg + + return isinstance(env_cfg, ManagerBasedRlEnvCfg) + + def _dispatch_play(task: Runnable, runner_args: dict[str, str], prof_args: dict[str, str]) -> None: task_cfg = getattr(task, "cfg", None) agent_cfg = getattr(task_cfg, "agent", None) if task_cfg is not None else None @@ -79,6 +96,40 @@ def _dispatch_play(task: Runnable, runner_args: dict[str, str], prof_args: dict[ # play is always single-process; either flag is accepted but mapped through the same # resolver so the mutual-exclusion guard fires on misuse. num_envs_per_rank = _resolve_per_rank_num_envs(runner_args, gpus=1) + + # The CLI's already-overridden cfg (play_env when configured): TASKS.get returns a + # fresh task each call, so the runner re-resolving would discard the --vis / --gpu / + # --a.env.* overrides applied above. + play_env_cfg = getattr(task_cfg, "play_env", None) + if play_env_cfg is None: + play_env_cfg = getattr(task_cfg, "env", None) + + # Scene-playback demos (non-RL: env cfg is a base ManagerBasedEnvCfg) can't go through + # the RL play helper — ManagerBasedRlEnv requires an RL cfg surface they don't have, so + # play_task → build_env would crash in env construction. Run their own .play() instead. + # The RL-only options below have no meaning for a fixed scene demo, so warn and ignore + # them rather than crashing. See P8/P9. + if not _is_rl_play_cfg(play_env_cfg): + ignored = [ + name + for name, present in ( + ("--checkpoint", checkpoint_raw is not None), + ("--num-envs", num_envs_per_rank is not None), + ("--agent", agent_raw is not None), + ("--prof*", bool(prof_args)), + ) + if present + ] + if ignored: + task_name = getattr(task_cfg, "name", None) or "task" + print( + f"warning: {task_name} is a non-RL scene-playback task; ignoring " + f"{', '.join(ignored)} and running its built-in playback.", + file=sys.stderr, + ) + task.play() + return + if ( checkpoint_raw is None and num_envs_per_rank is None @@ -93,12 +144,6 @@ def _dispatch_play(task: Runnable, runner_args: dict[str, str], prof_args: dict[ task_id = getattr(task_cfg, "name", None) if not isinstance(task_id, str): raise SystemExit("task config is missing 'name'; cannot route through RL play helper") - # Pass the CLI's already-overridden cfg (play_env when configured): TASKS.get - # returns a fresh task each call, so the runner re-resolving would discard the - # --vis / --gpu / --a.env.* overrides applied above. - play_env_cfg = getattr(task_cfg, "play_env", None) - if play_env_cfg is None: - play_env_cfg = getattr(task_cfg, "env", None) play_task( task_id, env_cfg=play_env_cfg, diff --git a/src/genelab/rl/_helpers.py b/src/genelab/rl/_helpers.py index 1db579e9..96b69d5c 100644 --- a/src/genelab/rl/_helpers.py +++ b/src/genelab/rl/_helpers.py @@ -142,8 +142,15 @@ def resolve_env_cfg(task_id: str, play: bool) -> Any: def build_env(env_cfg: Any) -> "ManagerBasedRlEnv": - from genelab.envs.manager_based_rl_env import ManagerBasedRlEnv - + from genelab.envs.manager_based_rl_env import ManagerBasedRlEnv, ManagerBasedRlEnvCfg + + if not isinstance(env_cfg, ManagerBasedRlEnvCfg): + raise TypeError( + f"build_env requires a ManagerBasedRlEnvCfg (the RL env surface: decimation, " + f"episode_length_s, actions_cfg, …); got {type(env_cfg).__name__}, which subclasses " + f"the non-RL base ManagerBasedEnvCfg. Scene-playback demos must run their own " + f".play() — they are not routed through the RL play helper. See P8/P9." + ) return ManagerBasedRlEnv(env_cfg) diff --git a/tests/fake_extension.py b/tests/fake_extension.py index 58a23bc1..86c89ad9 100644 --- a/tests/fake_extension.py +++ b/tests/fake_extension.py @@ -3,6 +3,7 @@ from dataclasses import dataclass, field from genelab.configs import ManagerBasedEnvCfg, SimulationCfg, TaskCfg +from genelab.envs.manager_based_rl_env import ManagerBasedRlEnvCfg from genelab.registry import register_env, register_robot, register_task @@ -17,6 +18,16 @@ class FakeEnvCfg(ManagerBasedEnvCfg): label: str = "fake-extension" +@dataclass +class FakeRlEnvCfg(ManagerBasedRlEnvCfg): + """RL-capable env cfg (subclasses the RL base) used to test that RL play options + route through the ``play_task`` helper — the counterpart to ``FakeEnvCfg``, which is + a non-RL scene demo.""" + + simulation: SimulationCfg = field(default_factory=lambda: SimulationCfg(steps=0)) + label: str = "fake-extension-rl" + + @dataclass class FakeRobot: cfg: FakeRobotCfg = field(default_factory=FakeRobotCfg) @@ -41,6 +52,26 @@ def train(self) -> None: raise NotImplementedError("training is not implemented for fake extension") +class FakeRlTask: + """An RL-capable task: its env cfg is a ``ManagerBasedRlEnvCfg``, so RL play options + (``--agent`` / ``--checkpoint`` / …) must route through the ``play_task`` helper.""" + + def __init__(self) -> None: + self.cfg = TaskCfg( + name="External-Fake-RL-Task-v0", + env_name="fake-extension-rl-env", + robot_name="fake-extension-robot", + env=FakeRlEnvCfg(), + trainable=False, + ) + + def play(self) -> None: + raise AssertionError("RL-capable task must route through play_task, not task.play()") + + def train(self) -> None: + raise NotImplementedError("training is not implemented for fake extension") + + def register() -> None: register_robot( "fake-extension-robot", @@ -64,3 +95,9 @@ def register() -> None: "genelab play External-Fake-Task-v0 --steps 7", ], ) + register_task( + "External-Fake-RL-Task-v0", + FakeRlTask, + description="RL-capable task from a fake external package.", + cfg_type=TaskCfg, + ) diff --git a/tests/test_cli_args.py b/tests/test_cli_args.py index f517d5b5..4437b09c 100644 --- a/tests/test_cli_args.py +++ b/tests/test_cli_args.py @@ -146,13 +146,13 @@ def _fake_play(task_id: str, **kwargs: object) -> None: "--import", "tests.fake_extension", "play", - "External-Fake-Task-v0", + "External-Fake-RL-Task-v0", "--num-envs", "8", ] ) - assert captured["task_id"] == "External-Fake-Task-v0" + assert captured["task_id"] == "External-Fake-RL-Task-v0" assert captured["num_envs"] == 8 diff --git a/tests/test_cli_routing.py b/tests/test_cli_routing.py index cdf2e33a..33ca014d 100644 --- a/tests/test_cli_routing.py +++ b/tests/test_cli_routing.py @@ -53,13 +53,49 @@ def _fake_play(task_id: str, **kwargs: object) -> None: monkeypatch.setitem(__import__("sys").modules, "genelab.rl", fake_rl) cli_module.main( - ["--import", "tests.fake_extension", "play", "External-Fake-Task-v0", "--agent", "random"] + [ + "--import", + "tests.fake_extension", + "play", + "External-Fake-RL-Task-v0", + "--agent", + "random", + ] ) - assert captured["task_id"] == "External-Fake-Task-v0" + assert captured["task_id"] == "External-Fake-RL-Task-v0" assert captured["agent"] == "random" +def test_play_non_rl_task_ignores_rl_flags_and_runs_demo( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """P8/P9 regression: a non-RL scene-playback task (env cfg is a base + ``ManagerBasedEnvCfg``) must run its own ``task.play()`` even when RL-only flags like + ``--agent`` are passed — routing it through ``play_task`` → ``build_env`` would crash + constructing ``ManagerBasedRlEnv`` from a cfg with no RL surface.""" + from genelab import cli as cli_module + + # Patch the real module's attribute (not sys.modules) so the dispatcher's + # `from genelab.rl import play_task` resolves to the sentinel while entry-point + # extension loading can still `from genelab.rl import RslRlModelCfg`. + import genelab.rl as genelab_rl + + def _fake_play(*_args: object, **_kwargs: object) -> None: + raise AssertionError("non-RL task must not route through the RL play helper") + + monkeypatch.setattr(genelab_rl, "play_task", _fake_play) + + cli_module.main( + ["--import", "tests.fake_extension", "play", "External-Fake-Task-v0", "--agent", "zero"] + ) + + out = capsys.readouterr() + assert "played External-Fake-Task-v0" in out.out + assert "non-RL scene-playback task" in out.err + assert "--agent" in out.err + + def test_core_namespaces_do_not_import_example_objects() -> None: import genelab.envs as envs import genelab.robots as robots @@ -210,7 +246,7 @@ def _fake_play(task_id: str, **kwargs: object) -> None: "--import", "tests.fake_extension", "play", - "External-Fake-Task-v0", + "External-Fake-RL-Task-v0", "--agent", "bogus", ] @@ -273,3 +309,14 @@ def test_play_unknown_override_path_exits_without_picker( ] ) assert "env.simulation.step" in str(excinfo.value) + + +def test_build_env_rejects_non_rl_cfg() -> None: + """Defense-in-depth backstop for P8/P9: handing the RL env builder a non-RL cfg fails + with a clear TypeError naming the required surface, not a cryptic AttributeError deep + inside ``ManagerBasedRlEnv.__init__``.""" + from genelab.configs import ManagerBasedEnvCfg + from genelab.rl.runner import build_env + + with pytest.raises(TypeError, match="ManagerBasedRlEnvCfg"): + build_env(ManagerBasedEnvCfg()) diff --git a/tests/test_examples_rubiks.py b/tests/test_examples_rubiks.py index 08bb839f..7066fb4e 100644 --- a/tests/test_examples_rubiks.py +++ b/tests/test_examples_rubiks.py @@ -10,6 +10,7 @@ import numpy as np from numpy.typing import NDArray +import pytest from genelab.configs import apply_overrides from genelab.registry import TASKS, load_extension_module @@ -170,14 +171,55 @@ def test_rubiks_robot_registry_writes_turnable_mjcf(tmp_path: Path) -> None: def test_rubiks_env_cfg_inherits_device_default() -> None: - """Regression: the Rubiks play scene uses the base ``ManagerBasedEnvCfg`` but is - built via ``ManagerBasedRlEnv``, which reads ``cfg.device`` — so the base cfg - must carry ``device`` (no longer RL-subclass-only).""" + """The Rubiks play scene uses the base ``ManagerBasedEnvCfg``; ``device`` lives on the + base cfg (PR #153), so the demo cfg inherits it.""" from genelab_examples.rubiks.config import RubiksEnvCfg assert RubiksEnvCfg().device == "cuda" +def test_rubiks_play_with_rl_flags_runs_scene_demo(monkeypatch: pytest.MonkeyPatch) -> None: + """P8/P9 regression: ``genelab play GeneLab-Rubiks-Play-v0 --agent zero --steps 5`` must + run the force-driven scene demo (``RubiksPlayEnv.play``), honoring ``--steps``, and must + NOT route through the RL play helper — ``play_task`` → ``build_env`` would crash building + ``ManagerBasedRlEnv`` from the non-RL ``RubiksEnvCfg``. This exercises the same dispatch + path the CLI uses, with Genesis stubbed out so it runs headless.""" + from genelab.cli import main + from genelab_examples import envs + + captured: dict[str, object] = {} + + def _fake_play(self: envs.RubiksPlayEnv) -> None: + captured["steps"] = self.cfg.simulation.steps + + monkeypatch.setattr(envs.RubiksPlayEnv, "play", _fake_play) + + # Patch the real module's attribute (not sys.modules) so the dispatcher's + # `from genelab.rl import play_task` resolves to the sentinel while entry-point + # extension loading can still `from genelab.rl import RslRlModelCfg`. + import genelab.rl as genelab_rl + + def _no_play_task(*_args: object, **_kwargs: object) -> None: + raise AssertionError("non-RL Rubiks demo must not route through play_task") + + monkeypatch.setattr(genelab_rl, "play_task", _no_play_task) + + main( + [ + "--import", + "genelab_examples.tasks", + "play", + "GeneLab-Rubiks-Play-v0", + "--agent", + "zero", + "--steps", + "5", + ] + ) + + assert captured["steps"] == 5 + + def test_rubiks_robot_registry_can_write_welded_mjcf(tmp_path: Path) -> None: load_extension_module("genelab_examples.tasks") task = TASKS.get("GeneLab-Rubiks-Play-v0") diff --git a/tests/test_examples_wuji.py b/tests/test_examples_wuji.py index 643bc198..c95dcd12 100644 --- a/tests/test_examples_wuji.py +++ b/tests/test_examples_wuji.py @@ -66,14 +66,55 @@ def test_wuji_default_trajectory_is_bundled_with_package_assets() -> None: def test_wuji_env_cfg_inherits_device_default() -> None: - """Regression: the Wuji playback scene uses the base ``ManagerBasedEnvCfg`` but is - built via ``ManagerBasedRlEnv``, which reads ``cfg.device`` — so the base cfg - must carry ``device`` (no longer RL-subclass-only).""" + """The Wuji playback scene uses the base ``ManagerBasedEnvCfg``; ``device`` lives on the + base cfg (PR #153), so the demo cfg inherits it.""" from genelab_examples.wuji_hand.config import WujiEnvCfg assert WujiEnvCfg().device == "cuda" +def test_wuji_play_with_rl_flags_runs_scene_demo(monkeypatch: pytest.MonkeyPatch) -> None: + """P8/P9 regression: ``genelab play GeneLab-Wuji-Hand-Playback-v0 --agent zero --steps 5`` + must run the fixed-trajectory scene demo (``WujiHandPlaybackEnv.play``), honoring + ``--steps``, and must NOT route through the RL play helper — ``play_task`` → ``build_env`` + would crash building ``ManagerBasedRlEnv`` from the non-RL ``WujiEnvCfg``. Exercises the + same dispatch path the CLI uses, with Genesis stubbed out so it runs headless.""" + from genelab.cli import main + from genelab_examples import envs + + captured: dict[str, object] = {} + + def _fake_play(self: envs.WujiHandPlaybackEnv) -> None: + captured["steps"] = self.cfg.simulation.steps + + monkeypatch.setattr(envs.WujiHandPlaybackEnv, "play", _fake_play) + + # Patch the real module's attribute (not sys.modules) so the dispatcher's + # `from genelab.rl import play_task` resolves to the sentinel while entry-point + # extension loading can still `from genelab.rl import RslRlModelCfg`. + import genelab.rl as genelab_rl + + def _no_play_task(*_args: object, **_kwargs: object) -> None: + raise AssertionError("non-RL Wuji demo must not route through play_task") + + monkeypatch.setattr(genelab_rl, "play_task", _no_play_task) + + main( + [ + "--import", + "genelab_examples.tasks", + "play", + "GeneLab-Wuji-Hand-Playback-v0", + "--agent", + "zero", + "--steps", + "5", + ] + ) + + assert captured["steps"] == 5 + + def test_wuji_default_description_assets_are_bundled_with_package_assets() -> None: assert "genelab_examples/wuji_hand/description" in DEFAULT_DESC_DIR.as_posix() assert resolve_mjcf_path(DEFAULT_DESC_DIR, "left").exists()