[RLlib] Fix calling of callback on_episode_created to conform to docstring (after reset).
#45651
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
…fter the 'env.reset' instead before. The docstring in the callback clearly states, it should come before the reset. Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
sven1977
reviewed
May 31, 2024
| # Create a new multi-agent episode. | ||
| _episode = self._new_episode() | ||
| self._make_on_episode_callback("on_episode_created", _episode) | ||
| _shared_data = { | ||
| "agent_to_module_mapping_fn": self.config.policy_mapping_fn, | ||
| } | ||
|
|
||
| # Reset the environment. |
There was a problem hiding this comment.
Hey @simonsays1980 , thanks for looking into this problem. I think this issue in general here is unfortunately more complex that what meets the eye right now :( . Let me explain:
- On single-agent, users are currently not even allowed to override the on_episode_created callback :D . This is because in single-agent, we use gym's vector env, which resets envs automatically after a terminal is hit, which makes it impossible to call the on_episode_created callback before this auto-reset happens. See here and here.
- For multi-agent (where currently we don't use gym.vector) this does actually work and I therefore would suggest, we only fix this for now on the multi-agent env runner.
- In MultiAgentEnvRunner, however, we should then also fix it for
_sample_episodes(). - We should update the docstring in callbacks.py to reflect that this callback is NOT currently valid for new API stack + single-agent.
- We should remove the on_episode_created callback call entirely from single-agent env runner.
sven1977
reviewed
May 31, 2024
There was a problem hiding this comment.
Thanks for raising this issue @simonsays1980 , an important one.
I wrote my thoughts and the current problems with this particular callback below and suggested some changes. Then we can merge this. :)
Signed-off-by: Simon Zehnder <[email protected]>
sven1977
changed the title
on_episode_created to conform to docstring (after reset).on_episode_created to conform to docstring (after reset).
sven1977
approved these changes
May 31, 2024
sven1977
reviewed
May 31, 2024
Signed-off-by: Simon Zehnder <[email protected]>
Signed-off-by: Simon Zehnder <[email protected]>
simonsays1980
added
bug
richardsliu
pushed a commit
to richardsliu/ray
that referenced
this pull request
Jun 12, 2024
…cstring (after reset). (ray-project#45651) Signed-off-by: Richard Liu <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
The docstring of the
on_episode_createdcallback states clearly that this callback should be called before theenv.reset.Related issue number
Closes #45544
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.