Discover environment tools on the class, not the instance

[Rome-1]

What does this PR do?

Fixes #7362

GRPOTrainer and the async rollout worker discover an environment's tools with inspect.getmembers(env, predicate=inspect.ismethod).
inspect._getmembers calls getattr on every name it lists and applies the predicate afterwards, so the predicate cannot stop an attribute from being evaluated.
Every property on an environment therefore runs: once at trainer init, on the probe instance, and again for each rollout, before every step.
A functools.cached_property runs the same way, on first access.

trl/experimental/harbor/_env.py has such a property.
HarborEnv.reward runs the Harbor task's verifier, and its self._env is None guard covers a freshly constructed instance but not a pooled one that was reset in an earlier batch and whose reward was never read.
That is the state of every pooled instance whenever reward_funcs does not consume environments.

On trl 1.13.0, transformers 5.17.0, vllm 0.28.0, one L40S, with a plain synchronous GRPOTrainer (use_vllm=True, vllm_mode="colocate") and a custom reward_funcs, the second step dies before it generates anything:

  File ".../trl/trainer/grpo_trainer.py", line 1598, in _prepare_inputs
    generation_batch = self._generate_and_score_completions(generation_batch)
  File ".../trl/trainer/grpo_trainer.py", line 2395, in _generate_and_score_completions
    for member_name, member in inspect.getmembers(self.environments[i], predicate=inspect.ismethod)
  File ".../inspect.py", line 607, in getmembers
  File ".../inspect.py", line 585, in _getmembers
    value = getter(object, key)
  File ".../trl/experimental/harbor/_env.py", line 96, in reward
    self._reward = self._run(self._verify())
  File ".../harbor/verifier/verifier.py", line 173, in verify
  File ".../harbor/utils/env.py", line 123, in resolve_env_vars
ValueError: Environment variable 'OPENAI_API_KEY' not found in host environment

The OPENAI_API_KEY error is not the bug; it is how the bug showed up.
Before it runs a task's tests, Harbor's Verifier.verify resolves the verifier's environment against the host, and harbor.utils.env.resolve_env_vars raises for any ${VAR} template with no default that the host does not set.
The task in this reproduction declares OPENAI_API_KEY for its verifier, as tasks with LLM-judged tests do.
The key was unset because this run never grades through HarborEnv.reward: its reward_funcs computes the reward itself.
So the only way to reach resolve_env_vars was for something to run the verifier uninvited, and the frames above show tool discovery doing it.
With the key set, the same run would not have failed. At the start of every step, tool discovery would have verified each pooled environment's previous rollout, paid for any judge calls that makes, and then thrown the result away when reset() cleared it, all without a word.

Running a grader is the loud case.
The quiet one is a property that is only slow or stateful, which pays that cost once per rollout per step and says nothing.

The fix

List the tool methods on the class, then bind them to the instance by name:

for member_name, _ in inspect.getmembers(type(instance), predicate=inspect.isfunction):
    ...
    methods.append(getattr(instance, member_name))

On the class a property is a descriptor object, so it is listed, rejected by the predicate, and never run.
async def methods are still inspect.isfunction, so async tools are unaffected.

Four call sites move together: the init-time probe and the per-batch lookup in trl/trainer/grpo_trainer.py, and both equivalents in trl/experimental/async_grpo/async_rollout_worker.py.

Why the trainer and not HarborEnv

HarborEnv.reward cannot defend itself.
The environment gets no end-of-rollout signal, so the property has no way to tell the trainer's tool probe from the reward function's read.
Any guard it adds has to answer "has this rollout finished?", and the environment does not know.
Widening the existing self._env is None guard would return a stale or zero reward for a pooled env that has been reset, which relocates the bug rather than fixing it.
The trainer is the side that knows it is only listing names, so that is where the change belongs.

This also keeps the fix independent of the Harbor module's fate (see Related, below): any environment with a property hits it.

Four corner cases of the public contract move

Checked against a probe class carrying each shape:

• A public staticmethod was not a tool and is one now. inspect.ismethod is false for it on an instance; inspect.isfunction is true for it on the class.
• A public classmethod was a tool and is not one now. getattr(cls, name) gives a bound method, which is not a function.
• A public bound method assigned onto the instance (self.tool = other.method) was a tool and is not one now. It does not exist on the class.
• A public functools.partialmethod was not a tool and is now collected as one, then raises AttributeError when the tool dict reads tool.__name__, because partialmethod.__get__ hands back a functools.partial. Happy to add a guard for it if reviewers want one.

No environment in this repository uses any of the four.
Every staticmethod on an environment class under examples/ is underscore-prefixed, so it was excluded before and is excluded now, and the tree contains no partialmethod at all.

inspect.getmembers_static, considered and rejected

inspect.getmembers_static lists members without triggering the descriptor protocol, and it would preserve today's contract more closely than the class lookup does.
It arrived in Python 3.11, and .github/workflows/tests_python_versions.yml still runs 3.10.
Hand-rolling it over inspect.getattr_static for that one version costs more code than the class lookup and buys four edge cases that no environment in the tree uses.

Related

examples/grpo_sudoku/grpo_sudoku.py carries this comment above its five reward properties:

# Reward properties — properties are not detected by inspect.ismethod,
# so they won't be exposed as tools.

True of exposure, false of evaluation.
Those five properties were computed for every rollout of every step; they are cheap, so nothing broke.
The comment holds in full after this PR.

• Issue [harbor/openreward] Adopt environment-owned reward (#6238) and dataset (#6349) #6354 tracks the environment-owned reward and dataset work that these environments build on.
• PR Remove the experimental Harbor integration (superseded by OpenEnv) #6948 proposes removing the experimental Harbor integration in favor of OpenEnv. This bug sits in the trainer's tool discovery, not in the Harbor module, so it outlives that removal.

Tests

Two new tests, each failing before the change with a RuntimeError raised from inside inspect._getmembers and passing after it:

• tests/test_grpo_trainer.py::TestGRPOTrainer::test_tool_discovery_does_not_evaluate_environment_properties trains two steps against an environment whose reward property is inert on a fresh instance and raises once the environment has been reset, with a reward_funcs that never reads environments and generate patched the way the neighboring environment tests patch it. The first step resets the pooled instances; the second is where the old lookup raises, with the same _prepare_inputs → _generate_and_score_completions → _getmembers frames as the traceback above.
• tests/experimental/test_async_grpo_trainer.py::TestAsyncRolloutWorkerEnvironments::test_tool_discovery_does_not_evaluate_properties covers the init-time probe in the rollout worker.

Three more places named the old expression and were updated with it: the comment on the HarborEnv.reward guard (the guard itself stays), tests/experimental/test_harbor.py::TestRewardFunc::test_fresh_env_reward_is_zero_without_backend, which mirrored instance-level discovery and now mirrors class-level discovery, and the "How tool binding works" paragraph in docs/source/openreward.md, whose adapter methods are already set on the class by setattr(cls, tool_name, fn).

Run locally on Python 3.12 with transformers 5.17.0, CPU only:

• pytest tests/test_grpo_trainer.py -k "environment or tool" — 15 passed, 2 skipped (both skips are the multimodal tool tests, which need vision extras).
• pytest tests/experimental/test_harbor.py — 14 passed.
• pytest tests/experimental/test_async_grpo_trainer.py::TestAsyncRolloutWorkerEnvironments — 3 passed.
• pre-commit run --files <every file touched> — ruff check, ruff format, and doc-builder style all pass.

The vLLM and multi-GPU paths were not exercised locally; the discovery change is independent of both.

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline, Pull Request section?
• Was this discussed/approved via a GitHub issue? Please add a link to it if that's the case.
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests?

AI writing disclosure

We welcome the use of AI tools to help with contributions. For transparency and to help us improve our review process, please indicate the level of AI involvement in this PR.

• No AI usage: the PR was written entirely by a human.
• AI-assisted: some parts were suggested or improved by AI, but the PR was written and reviewed by a human.
• AI-generated: the PR was mostly or fully generated by an AI tool.

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag members/contributors who may be interested in your PR.

[bot-ci-comment]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[albertvillanova]

Thanks, this is a real bug and the fix is the right one: listing on the class is the only way to keep getmembers from evaluating properties, and the four contract changes you list match what I get with a probe class. None of them is used by any environment in the repo.

One more data point in favor: in the async rollout worker the lookup runs after reset(), so on main a Harbor env's verifier runs against the freshly reset sandbox before every rollout, even with the default reward.

Two small requests:

• Could you shorten the new code comments? For example, # List on the class: getmembers on the instance evaluates properties at the init sites and nothing more at the per-batch sites.
• Please add Fixes #7362 to the description so the issue closes on merge.

No need for a partialmethod guard.

[albertvillanova]

Thanks for the update, LGTM.