Skip to content

Commit 2d37024

Browse files
authored
fix(humanoid_bench): override _extra_spec hook, not the extra_spec property (#785)
sit/slide/stair defined a plain method extra_spec(), which shadowed the BaseTaskEnv.extra_spec @Property (it returns self._extra_spec()). The base reads queries = self.extra_spec, so it got a bound method instead of the query dict — the SitePos site queries were never registered and the rewards' states.extras['head_pos']/['imu_pos'] lookups would KeyError. crawl.py already used the correct _extra_spec name. Rename the three to _extra_spec. Adds a general regression test (class-level: _extra_spec overridden, extra_spec not shadowed and still resolves to the base property). Red on pre-fix.
1 parent fb75800 commit 2d37024

4 files changed

Lines changed: 42 additions & 3 deletions

File tree

roboverse_pack/tasks/humanoid_bench/sit.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ def __init__(self, scenario: ScenarioCfg, device: str | torch.device | None = No
126126
self.reward_functions = [SitReward(self.robot_name)]
127127
self.reward_weights = [1.0]
128128

129-
def extra_spec(self):
129+
def _extra_spec(self):
130130
"""Declare extra observations needed by SitReward."""
131131
return {
132132
"imu_pos": SitePos("imu"),

roboverse_pack/tasks/humanoid_bench/slide.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ def __init__(self, scenario: ScenarioCfg, device: str | torch.device | None = No
102102
self.reward_functions = [SlideReward(self.robot_name)]
103103
self.reward_weights = [1.0]
104104

105-
def extra_spec(self):
105+
def _extra_spec(self):
106106
"""Declare extra observations needed by SlideReward."""
107107
return {
108108
"head_pos": SitePos("head"),

roboverse_pack/tasks/humanoid_bench/stair.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ def __init__(self, scenario: ScenarioCfg, device: str | torch.device | None = No
102102
self.reward_functions = [StairReward(self.robot_name)]
103103
self.reward_weights = [1.0]
104104

105-
def extra_spec(self):
105+
def _extra_spec(self):
106106
"""Declare extra observations needed by StairReward."""
107107
return {
108108
"head_pos": SitePos("head"),
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
"""Regression: humanoid_bench tasks must override _extra_spec, not shadow extra_spec.
2+
3+
``BaseTaskEnv.extra_spec`` is a *property* (returns ``self._extra_spec()``) that the
4+
base reads as ``queries = self.extra_spec`` to register optional site queries. The
5+
sit/slide/stair tasks defined a plain method ``extra_spec`` instead of the
6+
``_extra_spec`` hook, which shadowed the property — so ``self.extra_spec`` was a
7+
bound method (never called), the SitePos queries were never registered, and the
8+
rewards' ``states.extras["head_pos"]`` / ``["imu_pos"]`` lookups would KeyError.
9+
crawl.py used the correct ``_extra_spec`` name. Backend-free: a pure class-level
10+
check (no construction needed).
11+
"""
12+
13+
from __future__ import annotations
14+
15+
import importlib
16+
import inspect
17+
18+
import pytest
19+
20+
21+
@pytest.mark.general
22+
@pytest.mark.parametrize(
23+
"module_name,class_name",
24+
[
25+
("roboverse_pack.tasks.humanoid_bench.sit", "SitEnv"),
26+
("roboverse_pack.tasks.humanoid_bench.slide", "SlideEnv"),
27+
("roboverse_pack.tasks.humanoid_bench.stair", "StairEnv"),
28+
],
29+
)
30+
def test_uses_underscore_extra_spec_hook(module_name, class_name):
31+
cls = getattr(importlib.import_module(module_name), class_name)
32+
33+
# The subclass must override the _extra_spec hook, not the extra_spec property.
34+
assert "_extra_spec" in cls.__dict__, f"{class_name} must override _extra_spec"
35+
assert "extra_spec" not in cls.__dict__, (
36+
f"{class_name} defines a plain extra_spec method, shadowing the base property; rename it to _extra_spec"
37+
)
38+
# extra_spec must still resolve to the base property (so the base reads the dict).
39+
assert isinstance(inspect.getattr_static(cls, "extra_spec"), property)

0 commit comments

Comments
 (0)