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 de6a4db8ed6..4454bf90787 100644 --- a/tests/test_grpo_trainer.py +++ b/tests/test_grpo_trainer.py @@ -3457,6 +3457,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..bd0e3f86330 100644 --- a/trl/experimental/async_grpo/async_rollout_worker.py +++ b/trl/experimental/async_grpo/async_rollout_worker.py @@ -414,14 +414,15 @@ def __init__( instance = factory() has_reset = False methods = [] - for member_name, member in inspect.getmembers(instance, predicate=inspect.ismethod): + # 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 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`." @@ -574,8 +575,8 @@ async def _generate_loop(self, stop_event: asyncio.Event) -> None: methods = [] if environment is not None: 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 5bf473520b2..85743cae928 100644 --- a/trl/trainer/grpo_trainer.py +++ b/trl/trainer/grpo_trainer.py @@ -649,13 +649,14 @@ def __init__( has_reset = False has_reward = False methods = [] - for member_name, member in inspect.getmembers(instance, predicate=inspect.ismethod): + # 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 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`." @@ -2275,9 +2276,10 @@ def _generate_and_score_completions( for i in range(len(inputs)): methods = [] if self.environments: + 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 = {}, {}