From 7d3d1a34e0df65c86e29c1bca65adfdf35eb60af Mon Sep 17 00:00:00 2001 From: Rome Thorstenson Date: Wed, 23 Sep 2026 11:06:23 -0700 Subject: [PATCH 1/2] Discover environment tools on the class, not the instance `inspect.getmembers` calls `getattr` on every name it lists and applies the predicate afterwards, so listing an environment instance evaluates its properties. `HarborEnv.reward` runs the Harbor task's verifier from a property, and its `self._env is None` guard covers a freshly built instance but not a pooled one that an earlier batch reset and whose reward was never read. Training then died at the second step whenever `reward_funcs` did not consume `environments`. List the tool methods on the environment's class, where a property is inert, and bind them to the instance by name. Four call sites move together: the init-time probe and the per-batch lookup in GRPOTrainer, and both equivalents in the async rollout worker. --- docs/source/openreward.md | 2 +- tests/experimental/test_async_grpo_trainer.py | 29 ++++++++ tests/experimental/test_harbor.py | 9 +-- tests/test_grpo_trainer.py | 71 +++++++++++++++++++ .../async_grpo/async_rollout_worker.py | 14 ++-- trl/experimental/harbor/_env.py | 7 +- trl/trainer/grpo_trainer.py | 15 ++-- 7 files changed, 131 insertions(+), 16 deletions(-) diff --git a/docs/source/openreward.md b/docs/source/openreward.md index 51305fbd85f..005191e7dbe 100644 --- a/docs/source/openreward.md +++ b/docs/source/openreward.md @@ -167,7 +167,7 @@ spec = OpenRewardSpec("Eigent/SETA", indices=list(range(50, 100))) # range ## How tool binding works -At construction the spec calls the env's `/tools` endpoint to fetch a list of tool specs (each with a name, description, and JSON Schema for arguments). For each tool it generates a Python method on the per-rollout adapter with a typed signature and a docstring derived from the schema. So `transformers.utils.get_json_schema` and TRL's `inspect.getmembers(env, ismethod)` both produce the right tool schema for the model with no per-env wrapper code. +At construction the spec calls the env's `/tools` endpoint to fetch a list of tool specs (each with a name, description, and JSON Schema for arguments). For each tool it generates a Python method on the per-rollout adapter with a typed signature and a docstring derived from the schema. The methods are set on the adapter's class, where TRL's tool collector finds them, so `transformers.utils.get_json_schema` and TRL's `inspect.getmembers(type(env), isfunction)` both produce the right tool schema for the model with no per-env wrapper code. If a tool description contains characters that aren't safe to splice into Python source, the binder falls back to a sanitized form so binding never fails on real envs. diff --git a/tests/experimental/test_async_grpo_trainer.py b/tests/experimental/test_async_grpo_trainer.py index 8b97b4297f7..ec1e39b400f 100644 --- a/tests/experimental/test_async_grpo_trainer.py +++ b/tests/experimental/test_async_grpo_trainer.py @@ -767,6 +767,35 @@ def shout(self, text: str) -> str: finally: loop._loop.close() + def test_tool_discovery_does_not_evaluate_properties(self): + # The init-time probe lists an environment's tool methods. `inspect.getmembers` calls `getattr` on every name + # before applying the predicate, so listing the instance would evaluate this property; listing the class + # leaves it inert. + class PropertyEnvironment: + def reset(self, **kwargs): ... + + @property + def reward(self) -> float: + raise RuntimeError("`reward` must not be evaluated while discovering tools") + + def echo(self, text: str) -> str: + """Echo the text back. + + Args: + text: Text to echo. + + Returns: + The text, unchanged. + """ + return text + + loop = self._make_loop(PropertyEnvironment) + try: + assert [tool.__name__ for tool in loop._env_tools[None]] == ["echo"] + assert [tool.__name__ for tool in loop.tools] == ["echo"] + finally: + loop._loop.close() + def test_unknown_environment_raises(self): # An example whose `environment` field doesn't match any configured environment should fail with a clear error # rather than a bare KeyError mid-rollout. The check fires before any generation, so no vLLM is needed here. diff --git a/tests/experimental/test_harbor.py b/tests/experimental/test_harbor.py index f59497c8062..7e5646df600 100644 --- a/tests/experimental/test_harbor.py +++ b/tests/experimental/test_harbor.py @@ -127,12 +127,13 @@ def test_outcome_reward_uses_environment_reward_when_passed(self): assert _outcome_reward_func(environment_reward=[0.25, 0.75]) == [0.25, 0.75] def test_fresh_env_reward_is_zero_without_backend(self): - # The trainer discovers tool methods via `inspect.getmembers`, which evaluates properties. A fresh - # env (never `reset`) must expose its tools and return 0.0 from `reward` WITHOUT starting the - # Harbor backend or importing `harbor` (not installed in the trainer env). + # The trainer discovers tool methods on the environment's class (`inspect.getmembers` on the + # instance would evaluate `reward`). A fresh env (never `reset`) must expose its tools and return + # 0.0 from `reward` WITHOUT starting the Harbor backend or importing `harbor` (not installed in + # the trainer env). import inspect env = HarborBashEnv() - names = {n for n, _ in inspect.getmembers(env, predicate=inspect.ismethod)} + names = {n for n, _ in inspect.getmembers(type(env), predicate=inspect.isfunction)} assert {"bash", "reset"} <= names assert env.reward == 0.0 diff --git a/tests/test_grpo_trainer.py b/tests/test_grpo_trainer.py index 10f4d4e20cd..8ae2b300375 100644 --- a/tests/test_grpo_trainer.py +++ b/tests/test_grpo_trainer.py @@ -3465,6 +3465,77 @@ def fake_generate(input_ids, **kwargs): new_param = trainer.model.get_parameter(n) assert not torch.equal(param, new_param), f"Parameter {n} has not changed." + @pytest.mark.xfail( + condition=Version(transformers.__version__) < Version("5.2.0"), + reason="Environment factory support is not available in transformers versions below 5.2.0", + strict=True, + ) + @require_response_parsing + @patch.dict(os.environ, {"TRL_EXPERIMENTAL_SILENCE": "1"}) + def test_tool_discovery_does_not_evaluate_environment_properties(self): + # Tool discovery must list the environment's methods on its class. `inspect.getmembers` calls `getattr` on + # every name before applying the predicate, so listing the instance evaluates its properties, and a property + # may be expensive or have side effects (a Harbor env scores the rollout from one). Here `reward` is inert on + # a fresh instance and raises once the environment has been reset: the first batch resets the pooled + # instances, so a second batch that listed the instance would raise before generating anything. + class PropertyEnvironment: + def __init__(self): + self.is_reset = False + + def reset(self, **kwargs): + self.is_reset = True + + @property + def reward(self) -> float: + if self.is_reset: + raise RuntimeError("`reward` must not be evaluated while discovering tools") + return 0.0 + + def echo(self, text: str) -> str: + """ + Echo the text back. + + Args: + text: Text to echo. + + Returns: + The text, unchanged. + """ + return text + + def reward_func(completions, **kwargs): + # Never reads `environments`, so nothing but tool discovery can touch the `reward` property. + return [1.0] * len(completions) + + training_args = GRPOConfig( + output_dir=self.tmp_dir, + per_device_train_batch_size=3, # reduce the batch size to reduce memory usage + num_generations=3, # reduce the number of generations to reduce memory usage + max_steps=2, # the first batch resets the pooled instances, the second re-lists their tools + report_to="none", + ) + + trainer = GRPOTrainer( + model="trl-internal-testing/tiny-Qwen3MoeForCausalLM", + reward_funcs=reward_func, + args=training_args, + environment_factory=PropertyEnvironment, + ) + + def fake_generate(input_ids, **kwargs): + # "I won't increment<|im_end|>" — no tool call, so one generation round per step. + completion_ids = torch.tensor( + [[40, 2765, 944, 16252, 151645]] * input_ids.shape[0], device=input_ids.device + ) + return torch.cat([input_ids, completion_ids], dim=-1) + + with patch.object(trainer.model, "generate", side_effect=fake_generate): + trainer.train() + + assert trainer.state.log_history[-1]["train_loss"] is not None + # The regression is only exercised if the instances the last batch listed had really been reset before it. + assert all(environment.is_reset for environment in trainer.environments) + @pytest.mark.xfail( condition=Version(transformers.__version__) < Version("5.2.0"), reason="Environment factory support is not available in transformers versions below 5.2.0", diff --git a/trl/experimental/async_grpo/async_rollout_worker.py b/trl/experimental/async_grpo/async_rollout_worker.py index 5a6bf06d4c2..b79540d1ae3 100644 --- a/trl/experimental/async_grpo/async_rollout_worker.py +++ b/trl/experimental/async_grpo/async_rollout_worker.py @@ -414,14 +414,18 @@ def __init__( instance = factory() has_reset = False methods = [] - for member_name, member in inspect.getmembers(instance, predicate=inspect.ismethod): + # Look the tool methods up on the class, not the instance: `inspect.getmembers` evaluates every + # attribute it lists before applying the predicate, so probing the instance would run its properties + # (which may be expensive or have side effects, e.g. scoring a rollout). On the class a property is + # inert, and the functions found there are bound to the instance by name. + for member_name, _ in inspect.getmembers(type(instance), predicate=inspect.isfunction): if member_name == "reset": has_reset = True elif member_name == "get_reward": if type(instance) not in self._env_reward_types: self._env_reward_types.append(type(instance)) elif not member_name.startswith("_"): - methods.append(member) + methods.append(getattr(instance, member_name)) if not has_reset: raise ValueError( "Each environment instance returned by `environment_factory` must define a callable `reset`." @@ -573,9 +577,11 @@ async def _generate_loop(self, stop_event: asyncio.Event) -> None: # Build this rollout's tool dict: the standalone tools plus the methods of its environment. methods = [] if environment is not None: + # Looked up on the class, not the instance, for the reason given in `__init__`: the instance + # is a pooled one, and evaluating its properties here would run them before every rollout. methods = [ - member - for member_name, member in inspect.getmembers(environment, predicate=inspect.ismethod) + getattr(environment, member_name) + for member_name, _ in inspect.getmembers(type(environment), predicate=inspect.isfunction) if member_name not in ("reset", "get_reward") and not member_name.startswith("_") ] tool_dict = {tool.__name__: tool for tool in self._standalone_tools + methods} diff --git a/trl/experimental/harbor/_env.py b/trl/experimental/harbor/_env.py index fdef9800e26..d65c5b86471 100644 --- a/trl/experimental/harbor/_env.py +++ b/trl/experimental/harbor/_env.py @@ -87,9 +87,10 @@ def _exec(self, command: str, timeout: int = 180) -> str: def reward(self) -> float: # Submission = the agent wrote /workdir/answer.txt during the rollout; the verifier reads it. # Computed once, lazily, on first read (TRL reads this after the rollout via reward_funcs). - # A fresh env that was never `reset` (e.g. the trainer probing tool methods via - # `inspect.getmembers`, which evaluates properties) has no sandbox/task to verify — return 0.0 - # without invoking the verifier, which would start the Harbor backend and import `harbor`. + # A fresh env that was never `reset` has no sandbox/task to verify — return 0.0 without invoking + # the verifier, which would start the Harbor backend and import `harbor`. The trainer discovers + # tool methods on the class, so it never reads this property; the guard is for user code that + # touches a fresh env directly. if self._env is None: return 0.0 if self._reward is _NO_REWARD: diff --git a/trl/trainer/grpo_trainer.py b/trl/trainer/grpo_trainer.py index 62002b94860..3c233134293 100644 --- a/trl/trainer/grpo_trainer.py +++ b/trl/trainer/grpo_trainer.py @@ -659,13 +659,17 @@ def __init__( has_reset = False has_reward = False methods = [] - for member_name, member in inspect.getmembers(instance, predicate=inspect.ismethod): + # Look the tool methods up on the class, not the instance: `inspect.getmembers` evaluates every + # attribute it lists before applying the predicate, so probing the instance would run its properties + # (which may be expensive or have side effects, e.g. scoring a rollout). On the class a property is + # inert, and the functions found there are bound to the instance by name. + for member_name, _ in inspect.getmembers(type(instance), predicate=inspect.isfunction): if member_name == "reset": has_reset = True elif member_name == "get_reward": has_reward = True elif not member_name.startswith("_"): - methods.append(member) + methods.append(getattr(instance, member_name)) if not has_reset: raise ValueError( "Each environment instance returned by `environment_factory` must define a callable `reset`." @@ -2411,9 +2415,12 @@ def _generate_and_score_completions( for i in range(len(inputs)): methods = [] if self.environments: + # Looked up on the class, not the instance, for the reason given in `__init__`: the instance is a + # pooled one, and evaluating its properties here would run them before every rollout. + environment = self.environments[i] methods = [ - member - for member_name, member in inspect.getmembers(self.environments[i], predicate=inspect.ismethod) + getattr(environment, member_name) + for member_name, _ in inspect.getmembers(type(environment), predicate=inspect.isfunction) if member_name not in ("reset", "get_reward") and not member_name.startswith("_") ] sync_tool_dict, async_tool_dict = {}, {} From 9c10d839e2db97f319b07fa910e718f82f57de87 Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Sat, 3 Oct 2026 01:11:10 -0700 Subject: [PATCH 2/2] Shorten comments per review --- trl/experimental/async_grpo/async_rollout_worker.py | 7 +------ trl/trainer/grpo_trainer.py | 7 +------ 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/trl/experimental/async_grpo/async_rollout_worker.py b/trl/experimental/async_grpo/async_rollout_worker.py index b79540d1ae3..bd0e3f86330 100644 --- a/trl/experimental/async_grpo/async_rollout_worker.py +++ b/trl/experimental/async_grpo/async_rollout_worker.py @@ -414,10 +414,7 @@ def __init__( instance = factory() has_reset = False methods = [] - # Look the tool methods up on the class, not the instance: `inspect.getmembers` evaluates every - # attribute it lists before applying the predicate, so probing the instance would run its properties - # (which may be expensive or have side effects, e.g. scoring a rollout). On the class a property is - # inert, and the functions found there are bound to the instance by name. + # List on the class: getmembers on the instance evaluates properties for member_name, _ in inspect.getmembers(type(instance), predicate=inspect.isfunction): if member_name == "reset": has_reset = True @@ -577,8 +574,6 @@ async def _generate_loop(self, stop_event: asyncio.Event) -> None: # Build this rollout's tool dict: the standalone tools plus the methods of its environment. methods = [] if environment is not None: - # Looked up on the class, not the instance, for the reason given in `__init__`: the instance - # is a pooled one, and evaluating its properties here would run them before every rollout. methods = [ getattr(environment, member_name) for member_name, _ in inspect.getmembers(type(environment), predicate=inspect.isfunction) diff --git a/trl/trainer/grpo_trainer.py b/trl/trainer/grpo_trainer.py index 9b959ba97db..85743cae928 100644 --- a/trl/trainer/grpo_trainer.py +++ b/trl/trainer/grpo_trainer.py @@ -649,10 +649,7 @@ def __init__( has_reset = False has_reward = False methods = [] - # Look the tool methods up on the class, not the instance: `inspect.getmembers` evaluates every - # attribute it lists before applying the predicate, so probing the instance would run its properties - # (which may be expensive or have side effects, e.g. scoring a rollout). On the class a property is - # inert, and the functions found there are bound to the instance by name. + # List on the class: getmembers on the instance evaluates properties for member_name, _ in inspect.getmembers(type(instance), predicate=inspect.isfunction): if member_name == "reset": has_reset = True @@ -2279,8 +2276,6 @@ def _generate_and_score_completions( for i in range(len(inputs)): methods = [] if self.environments: - # Looked up on the class, not the instance, for the reason given in `__init__`: the instance is a - # pooled one, and evaluating its properties here would run them before every rollout. environment = self.environments[i] methods = [ getattr(environment, member_name)