[RLlib] Clarify extra model output docstrings#63524
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the docstrings for the set_extra_model_outputs method in both MultiAgentEpisode and SingleAgentEpisode to clarify that the method is intended for overriding existing keys rather than inserting new ones. The review feedback suggests further refining these docstrings to correctly reflect that extra_model_outputs are stored within individual agents in the multi-agent context and to improve the phrasing for better readability in the single-agent context.
| key: The existing `key` within `self.extra_model_outputs` to override | ||
| data on. |
There was a problem hiding this comment.
The docstring refers to self.extra_model_outputs, but MultiAgentEpisode does not have an extra_model_outputs attribute (it is stored within the individual SingleAgentEpisode objects). It is clearer to refer to the extra_model_outputs dictionary of each agent.
| key: The existing `key` within `self.extra_model_outputs` to override | |
| data on. | |
| key: The existing `key` within each agent's `extra_model_outputs` dict | |
| to override data on. |
| postprocessing steps, the entirety (or a slice) of an existing episode | ||
| `extra_model_outputs` entry might have to be rewritten, which is when |
There was a problem hiding this comment.
The phrasing "existing episode extra_model_outputs entry" is slightly awkward. Referring to an "existing entry in the episode's extra_model_outputs" improves readability and clarity.
| postprocessing steps, the entirety (or a slice) of an existing episode | |
| `extra_model_outputs` entry might have to be rewritten, which is when | |
| postprocessing steps, the entirety (or a slice) of an existing entry in | |
| the episode's `extra_model_outputs` might have to be rewritten, which is when |
Signed-off-by: GoparapukethaN <goparapukethan01@gmail.com>
GoparapukethaN
force-pushed
the
clarify-extra-model-outputs-docstring
branch
from
d050e51 to
a708514
Compare
|
Thanks for the review. I updated the wording to make the single-agent overwrite behavior clearer and to avoid implying that |
ray-gardener
Bot
added
rllib
pseudo-rnd-thoughts
left a comment
pseudo-rnd-thoughts
left a comment
There was a problem hiding this comment.
LGTM, thanks for the PR @GoparapukethaN
## Why `set_extra_model_outputs` currently asserts that the provided key already exists before calling `InfiniteLookbackBuffer.set`. The docstrings for `SingleAgentEpisode` and `MultiAgentEpisode` also said the method could insert a new key, which made the behavior ambiguous. This updates the docstrings to describe the current overwrite-only behavior. Closes ray-project#63217 ## Tests Not run. Documentation-only change. Signed-off-by: GoparapukethaN <goparapukethan01@gmail.com>
3 participants
Why
set_extra_model_outputscurrently asserts that the provided key already exists before callingInfiniteLookbackBuffer.set. The docstrings forSingleAgentEpisodeandMultiAgentEpisodealso said the method could insert a new key, which made the behavior ambiguous.This updates the docstrings to describe the current overwrite-only behavior.
Closes #63217
Tests
Not run. Documentation-only change.