Skip to content

Commit 50ef9bf

Browse files
cagataycaliDevDuck
andauthored
feat(training): RL parity remainder — staged_reward, gym adapter, vectorized PPO, render output_path + precise env union (#915)
Co-authored-by: DevDuck <devduck@strands.local>
1 parent e4d2643 commit 50ef9bf

15 files changed

Lines changed: 1375 additions & 18 deletions

File tree

pyproject.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,7 @@ ignore_missing_imports = false
467467

468468
# Third-party libs without type stubs
469469
[[tool.mypy.overrides]]
470-
module = ["lerobot.*", "gr00t.*", "draccus.*", "msgpack.*", "websockets", "websockets.*", "zmq.*", "huggingface_hub.*", "serial.*", "psutil.*", "torch.*", "torchvision.*", "transformers.*", "einops.*", "robot_descriptions.*", "mujoco.*", "imageio.*", "libero.*", "zenoh.*", "boto3", "boto3.*", "awscrt", "awscrt.*", "awsiot", "awsiot.*", "botocore.*", "strands_robots.policies.cosmos3._msgpack_numpy", "diffusers", "diffusers.*", "accelerate", "accelerate.*", "openpi_client", "openpi_client.*", "openpi_server", "openpi_server.*", "openpi", "openpi.*", "device_connect_edge", "device_connect_edge.*", "device_connect_agent_tools", "device_connect_agent_tools.*", "rclpy", "rclpy.*", "cyclonedds", "cyclonedds.*", "rosidl_runtime_py", "rosidl_runtime_py.*", "moveit", "moveit.*", "moveit_configs_utils", "moveit_configs_utils.*", "geometry_msgs", "geometry_msgs.*", "mink", "mink.*", "qpsolvers", "qpsolvers.*", "onnxruntime", "onnxruntime.*", "motionbricks", "motionbricks.*", "trimesh", "trimesh.*"]
470+
module = ["lerobot.*", "gr00t.*", "draccus.*", "msgpack.*", "websockets", "websockets.*", "zmq.*", "huggingface_hub.*", "serial.*", "psutil.*", "torch.*", "torchvision.*", "transformers.*", "einops.*", "robot_descriptions.*", "mujoco.*", "imageio.*", "libero.*", "zenoh.*", "boto3", "boto3.*", "awscrt", "awscrt.*", "awsiot", "awsiot.*", "botocore.*", "strands_robots.policies.cosmos3._msgpack_numpy", "diffusers", "diffusers.*", "accelerate", "accelerate.*", "openpi_client", "openpi_client.*", "openpi_server", "openpi_server.*", "openpi", "openpi.*", "device_connect_edge", "device_connect_edge.*", "device_connect_agent_tools", "device_connect_agent_tools.*", "rclpy", "rclpy.*", "cyclonedds", "cyclonedds.*", "rosidl_runtime_py", "rosidl_runtime_py.*", "moveit", "moveit.*", "moveit_configs_utils", "moveit_configs_utils.*", "geometry_msgs", "geometry_msgs.*", "mink", "mink.*", "qpsolvers", "qpsolvers.*", "onnxruntime", "onnxruntime.*", "motionbricks", "motionbricks.*", "trimesh", "trimesh.*", "stable_baselines3", "stable_baselines3.*"]
471471
ignore_missing_imports = true
472472

473473
# Device Connect drivers - thin wrappers over the untyped device_connect_edge

strands_robots/simulation/benchmark_spec.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,13 @@ def on_episode_start(self, sim: SimEngine, rng: random.Random) -> None:
236236
raise RuntimeError(
237237
f"DeclarativeBenchmark '{self._name}': load_scene({self._scene!r}) failed: {msg}"
238238
)
239+
# Reset any stateful reward terms (e.g. a staged_reward phase machine)
240+
# so per-episode phase state does not leak across episodes. Stateless
241+
# function terms have no reset() and are skipped.
242+
for term in self._reward_terms:
243+
term_reset = getattr(term, "reset", None)
244+
if callable(term_reset):
245+
term_reset()
239246
super().on_episode_start(sim, rng)
240247

241248
def on_step(self, sim: SimEngine, obs: dict[str, Any], action: dict[str, Any]) -> StepInfo:

strands_robots/simulation/mujoco/rendering.py

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -571,10 +571,21 @@ def _scale_ctrl_for_actuator(model: Any, ai: int, value: float, mj: Any) -> floa
571571
return lo + frac * span
572572

573573
def render(
574-
self, camera_name: str = "default", width: int | None = None, height: int | None = None
574+
self,
575+
camera_name: str = "default",
576+
width: int | None = None,
577+
height: int | None = None,
578+
output_path: str | None = None,
575579
) -> dict[str, Any]:
576580
"""Render a camera view to a PNG image.
577581
582+
When ``output_path`` is given the PNG is ALSO written to that file path
583+
(parent dirs created) and the saved path is reported in the ``json``
584+
block as ``saved_path`` and in the text summary. This lets an agent (or
585+
a human) persist a render for independent verification instead of only
586+
receiving the bytes inline.
587+
588+
578589
Returns an agent-tool dict with ``status`` and a ``content`` list; on
579590
success the content holds an ``image`` block carrying PNG bytes
580591
(``{"image": {"format": "png", "source": {"bytes": ...}}}``) plus a
@@ -662,12 +673,36 @@ def render(
662673
pixel_var = float(_np.var(img))
663674
pixel_mean = float(_np.mean(img))
664675

676+
saved_path: str | None = None
677+
if output_path:
678+
import os as _os
679+
680+
# Validate against shell/path-traversal injection (LLM-supplied).
681+
bad = {";", "|", "$", "`", ">", "<", "\n", "\r", "\x00"}
682+
if any(b in output_path for b in bad) or ".." in output_path.split("/"):
683+
return {
684+
"status": "error",
685+
"content": [{"text": f"render: unsafe output_path {output_path!r}"}],
686+
}
687+
_dir = _os.path.dirname(_os.path.abspath(output_path))
688+
_os.makedirs(_dir, exist_ok=True)
689+
with open(output_path, "wb") as _f:
690+
_f.write(png_bytes)
691+
saved_path = _os.path.abspath(output_path)
692+
693+
summary = f"{w}x{h} from '{label}' at t={self._world.sim_time:.3f}s"
694+
if saved_path:
695+
summary += f" -> saved {saved_path}"
696+
json_block = {"pixel_variance": pixel_var, "pixel_mean": pixel_mean, "camera": label}
697+
if saved_path:
698+
json_block["saved_path"] = saved_path
699+
665700
return {
666701
"status": "success",
667702
"content": [
668-
{"text": f"{w}x{h} from '{label}' at t={self._world.sim_time:.3f}s"},
703+
{"text": summary},
669704
{"image": {"format": "png", "source": {"bytes": png_bytes}}},
670-
{"json": {"pixel_variance": pixel_var, "pixel_mean": pixel_mean, "camera": label}},
705+
{"json": json_block},
671706
],
672707
}
673708
except Exception as e:

strands_robots/simulation/predicates.py

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -554,6 +554,161 @@ def term(_sim: SimEngine) -> float:
554554
return term
555555

556556

557+
# Stateful reward terms (declarative phase machine)
558+
#
559+
# A plain RewardTerm is stateless: ``(SimEngine) -> float``. Some rewards need
560+
# memory across steps - a pick-place curriculum advances Reach -> Grasp ->
561+
# Transport -> Place, awards a one-time bonus on each transition, and only ever
562+
# moves forward. Rather than hardcode any specific task, we expose ONE
563+
# generic primitive, ``staged_reward``, that composes EXISTING registry
564+
# predicates into a phase machine. The task itself is then authored as data
565+
# (a spec dict / YAML) by a human or LLM - never as shipped code, and never via
566+
# ``eval`` (sub-predicates are compiled through :func:`make_predicate`, the same
567+
# closed-registry path as every other DSL call).
568+
569+
570+
class StatefulRewardTerm:
571+
"""A reward term that carries per-episode state and must be ``reset()``.
572+
573+
Duck-typed by consumers: anything with ``__call__(sim) -> float`` AND a
574+
zero-arg ``reset()`` is treated as episode-stateful. ``SimEnv.reset`` and
575+
``DeclarativeBenchmark.on_episode_start`` call ``reset()`` on any reward
576+
term that has it, so stateless plain-function terms are unaffected.
577+
"""
578+
579+
def __call__(self, sim: SimEngine) -> float: # pragma: no cover - interface
580+
raise NotImplementedError
581+
582+
def reset(self) -> None: # pragma: no cover - interface
583+
raise NotImplementedError
584+
585+
586+
class _StagedReward(StatefulRewardTerm):
587+
"""Monotonic multi-stage (phase-machine) reward built from sub-predicates.
588+
589+
Each stage declares:
590+
- ``reward``: a float-valued registry predicate giving the dense
591+
shaping signal while the machine is IN that stage.
592+
- ``advance_when``: a bool-valued registry predicate; the FIRST step it
593+
returns True the machine awards ``bonus`` once and advances to the
594+
next stage. Phases only ever move forward (no regression), matching
595+
curriculum semantics and giving a stable, non-oscillating signal.
596+
- ``bonus``: a one-time scalar added on the transition out of the stage
597+
(default 0.0).
598+
599+
The last stage has no ``advance_when`` gate (the task is "done" there for
600+
reward purposes; episode termination is a separate ``success`` predicate).
601+
Per step the emitted reward is ``current_stage.reward(sim) +
602+
(bonus if this step advanced else 0.0)``.
603+
"""
604+
605+
def __init__(
606+
self,
607+
stages: list[tuple[RewardTerm, BoolPredicate | None, float]],
608+
) -> None:
609+
self._stages = stages
610+
self._phase = 0
611+
612+
def reset(self) -> None:
613+
self._phase = 0
614+
615+
@property
616+
def phase(self) -> int:
617+
"""Current stage index (0-based). Exposed for logging / tests."""
618+
return self._phase
619+
620+
def __call__(self, sim: SimEngine) -> float:
621+
if not self._stages:
622+
return 0.0
623+
phase = min(self._phase, len(self._stages) - 1)
624+
reward_fn, advance_fn, bonus = self._stages[phase]
625+
r = float(reward_fn(sim))
626+
# Advance (and award the one-time bonus) only if there IS a next stage
627+
# and this stage declares a gate that now fires.
628+
if self._phase < len(self._stages) - 1 and advance_fn is not None and bool(advance_fn(sim)):
629+
self._phase += 1
630+
return r + float(bonus)
631+
return r
632+
633+
634+
def _staged_reward(stages: list[Any]) -> RewardTerm:
635+
"""Factory: compile a declared stage list into a :class:`_StagedReward`.
636+
637+
This is the single new primitive that turns the stateless DSL into a
638+
declarative phase machine. It recursively compiles each stage's ``reward``
639+
and ``advance_when`` through :func:`make_predicate`, so the whole thing
640+
stays inside the closed-registry / no-``eval`` safety contract: a spec can
641+
only ever reference predicates that already exist in the registry.
642+
643+
Args:
644+
stages: Ordered list of stage dicts. Each stage::
645+
646+
{
647+
"reward": {"predicate": <float-term name>, **kwargs},
648+
"advance_when": {"predicate": <bool-pred name>, **kwargs}, # omit on last stage
649+
"bonus": <float>, # optional, default 0.0
650+
}
651+
652+
Returns:
653+
A callable+resettable :class:`_StagedReward`.
654+
655+
Raises:
656+
ValueError: stages is not a non-empty list, a stage is malformed, a
657+
non-final stage omits ``advance_when``, or ``bonus`` is non-numeric.
658+
TypeError: surfaced from :func:`make_predicate` for bad sub-kwargs.
659+
"""
660+
if not isinstance(stages, list) or not stages:
661+
raise ValueError("staged_reward: 'stages' must be a non-empty list of stage dicts")
662+
663+
compiled: list[tuple[RewardTerm, BoolPredicate | None, float]] = []
664+
n = len(stages)
665+
for i, stage in enumerate(stages):
666+
if not isinstance(stage, dict):
667+
raise ValueError(f"staged_reward: stage[{i}] must be a dict, got {type(stage).__name__}")
668+
unknown = set(stage.keys()) - {"reward", "advance_when", "bonus"}
669+
if unknown:
670+
raise ValueError(
671+
f"staged_reward: stage[{i}] has unknown keys {sorted(unknown)}; allowed: reward, advance_when, bonus"
672+
)
673+
674+
reward_call = stage.get("reward")
675+
if not isinstance(reward_call, dict) or "predicate" not in reward_call:
676+
raise ValueError(
677+
f"staged_reward: stage[{i}].reward must be a predicate-call dict "
678+
"like {predicate: distance_neg, body_a: ..., body_b: ...}"
679+
)
680+
reward_name = reward_call["predicate"]
681+
reward_kwargs = {k: v for k, v in reward_call.items() if k != "predicate"}
682+
reward_fn = make_predicate(reward_name, **reward_kwargs)
683+
684+
advance_call = stage.get("advance_when")
685+
advance_fn: BoolPredicate | None
686+
if advance_call is None:
687+
if i != n - 1:
688+
raise ValueError(
689+
f"staged_reward: stage[{i}] is not the final stage and must declare "
690+
"'advance_when' (a bool predicate gating the transition to the next stage)"
691+
)
692+
advance_fn = None
693+
else:
694+
if not isinstance(advance_call, dict) or "predicate" not in advance_call:
695+
raise ValueError(
696+
f"staged_reward: stage[{i}].advance_when must be a predicate-call dict "
697+
"like {predicate: distance_less_than, body_a: ..., body_b: ..., threshold: ...}"
698+
)
699+
advance_name = advance_call["predicate"]
700+
advance_kwargs = {k: v for k, v in advance_call.items() if k != "predicate"}
701+
advance_fn = make_predicate(advance_name, **advance_kwargs)
702+
703+
bonus_raw = stage.get("bonus", 0.0)
704+
if isinstance(bonus_raw, bool) or not isinstance(bonus_raw, (int, float)):
705+
raise ValueError(f"staged_reward: stage[{i}].bonus must be a number, got {bonus_raw!r}")
706+
707+
compiled.append((reward_fn, advance_fn, float(bonus_raw)))
708+
709+
return _StagedReward(compiled)
710+
711+
557712
# Registry
558713

559714
PREDICATE_REGISTRY: dict[str, PredicateFactory] = {
@@ -574,6 +729,8 @@ def term(_sim: SimEngine) -> float:
574729
"distance_neg": _distance_neg,
575730
"joint_progress": _joint_progress,
576731
"constant": _constant,
732+
# stateful (phase machine)
733+
"staged_reward": _staged_reward,
577734
}
578735

579736

@@ -634,6 +791,7 @@ def make_predicate(name: str, **kwargs: Any) -> Callable[[SimEngine], Any]:
634791
"BoolPredicate",
635792
"PredicateFactory",
636793
"RewardTerm",
794+
"StatefulRewardTerm",
637795
"make_predicate",
638796
"register_predicate",
639797
]

strands_robots/training/rl/__init__.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
from strands_robots.training.rl.base_algo import BaseRLAlgo, RLTrainSpec
2525
from strands_robots.training.rl.env import SimEnv
2626
from strands_robots.training.rl.fast_sac import FastSacTrainer
27+
from strands_robots.training.rl.gym_env import GymSimEnv
2728
from strands_robots.training.rl.normalization import EmpiricalNormalization
2829
from strands_robots.training.rl.ppo import PpoTrainer
2930
from strands_robots.training.rl.replay_buffer import SimpleReplayBuffer
@@ -36,6 +37,7 @@
3637
"FastSacTrainer",
3738
"SimpleReplayBuffer",
3839
"SimEnv",
40+
"GymSimEnv",
3941
"VecSimEnv",
4042
"EmpiricalNormalization",
4143
]

strands_robots/training/rl/base_algo.py

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import torch
3535

3636
from strands_robots.training.rl.env import SimEnv
37+
from strands_robots.training.rl.vec_env import VecSimEnv
3738

3839

3940
@dataclass
@@ -132,10 +133,11 @@ class BaseRLAlgo(Trainer):
132133
"""
133134

134135
steps_per_iter: int = 1
135-
# Subclass-provided attributes (set during setup)
136+
# Subclass-provided attributes (set during setup()); declared so the shared
137+
# train()/evaluate()/load_checkpoint() type-check against the abstract base.
136138
actor_critic: Any # torch.nn.Module (actor-critic network)
137-
env: Any # SimEnv or VecSimEnv
138-
device: Any # torch.device or str
139+
env: SimEnv | VecSimEnv
140+
device: torch.device
139141

140142
@abstractmethod
141143
def setup(self, spec: RLTrainSpec) -> None:
@@ -303,7 +305,9 @@ def _norm(x: torch.Tensor) -> torch.Tensor:
303305
# env even when training used a VecSimEnv (N>1). A VecSimEnv returns
304306
# (N,)-batched rewards/dones that cannot be scalarised here; use its
305307
# first sub-env, which is a plain SimEnv with the (1,)-shaped contract.
306-
eval_env = self.env.envs[0] if hasattr(self.env, "envs") else self.env
308+
from strands_robots.training.rl.vec_env import VecSimEnv
309+
310+
eval_env = self.env.envs[0] if isinstance(self.env, VecSimEnv) else self.env
307311

308312
returns: list[float] = []
309313
lengths: list[int] = []

strands_robots/training/rl/env.py

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,9 @@ class SimEnv:
4747
(symmetric).
4848
max_episode_steps: Steps before the episode is truncated (time-out).
4949
action_scale: Scalar multiplier applied to actions before sending.
50+
n_substeps: Physics control substeps per env step. The action is a
51+
position target; the PD controller needs several substeps to track
52+
it, so a single substep barely moves the arm. Default 5.
5053
success_fn: Optional predicate; when it returns ``True`` the episode
5154
terminates (a genuine terminal, not a time-out).
5255
reset_fn: Optional callable run on ``engine`` at each reset (e.g. to
@@ -67,6 +70,7 @@ def __init__(
6770
critic_obs_keys: Sequence[str] | None = None,
6871
max_episode_steps: int = 200,
6972
action_scale: float = 1.0,
73+
n_substeps: int = 5,
7074
success_fn: Callable[[SimEngine], bool] | None = None,
7175
reset_fn: Callable[[SimEngine], None] | None = None,
7276
device: torch.device | str = "cpu",
@@ -82,6 +86,9 @@ def __init__(
8286
self.reward_terms = list(reward_terms)
8387
self.max_episode_steps = int(max_episode_steps)
8488
self.action_scale = float(action_scale)
89+
if int(n_substeps) < 1:
90+
raise ValueError(f"n_substeps must be >= 1, got {n_substeps}")
91+
self.n_substeps = int(n_substeps)
8592
self.success_fn = success_fn
8693
self.reset_fn = reset_fn
8794
self.device = torch.device(device)
@@ -120,11 +127,21 @@ def _obs_dict(self) -> dict[str, torch.Tensor]:
120127
}
121128

122129
def reset(self) -> dict[str, torch.Tensor]:
123-
"""Reset the episode and return the initial ``{actor_obs, critic_obs}``."""
130+
"""Reset the episode and return the initial ``{actor_obs, critic_obs}``.
131+
132+
Stateful reward terms (any term exposing a zero-arg ``reset()``, e.g. a
133+
``staged_reward`` phase machine) are reset here so per-episode state
134+
(current phase, awarded bonuses) does not leak across episodes. Plain
135+
stateless function terms have no ``reset`` and are left untouched.
136+
"""
124137
if self.reset_fn is not None:
125138
self.reset_fn(self.engine)
126139
else:
127140
self.engine.reset()
141+
for term in self.reward_terms:
142+
term_reset = getattr(term, "reset", None)
143+
if callable(term_reset):
144+
term_reset()
128145
self._step_count = 0
129146
return self._obs_dict()
130147

@@ -141,7 +158,7 @@ def step(self, action: torch.Tensor) -> tuple[dict[str, torch.Tensor], torch.Ten
141158
should be value-bootstrapped, not treated as a terminal state).
142159
"""
143160
act = action.detach().reshape(-1).to("cpu").numpy().astype(np.float64) * self.action_scale
144-
self.engine.send_action(act.tolist(), robot_name=self.robot_name)
161+
self.engine.send_action(act.tolist(), robot_name=self.robot_name, n_substeps=self.n_substeps)
145162
self._step_count += 1
146163

147164
reward = sum(term(self.engine) for term in self.reward_terms)
@@ -158,3 +175,9 @@ def step(self, action: torch.Tensor) -> tuple[dict[str, torch.Tensor], torch.Ten
158175
# true terminal obs (a time-out is value-bootstrapped, a real terminal is
159176
# not). See ``PpoTrainer.collect_rollout``.
160177
return obs, reward_t, done_t, info
178+
179+
def close(self) -> None:
180+
"""Release env resources. No-op: the engine/Robot lifecycle is owned by
181+
the caller (mirrors ``GymSimEnv.close`` / ``VecSimEnv.close`` so SimEnv
182+
and VecSimEnv present one interface to the trainers)."""
183+
return None

0 commit comments

Comments
 (0)