-
Notifications
You must be signed in to change notification settings - Fork 7k
[RLlib] Cleanup examples folder #10: Add custom_rl_module.py example script and matching RLModule example class (tiny CNN)..
#45774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
…nup_examples_folder_10_custom_rl_module.py
…nup_examples_folder_10_custom_rl_module.py Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
…nup_examples_folder_10_custom_rl_module.py
Signed-off-by: sven1977 <[email protected]>
…with RLlib default ones. Signed-off-by: sven1977 <[email protected]>
simonsays1980
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
| # The default implementation is to return TorchCategorical for Discrete action | ||
| # spaces and TorchDiagGaussian for Box action spaces. For all other spaces, | ||
| # raise a NotImplementedError | ||
| if isinstance(self.config.action_space, gym.spaces.Discrete): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using TorchMultiCategorical and TorchMultiDistribution - things that get assembled inside of the Catalog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure either, tbh. I just wanted to get the most simple setup automated. I feel like users that just want to "hack together an RLModule" should not be concerned about picking the categorical distr for their CartPole action space :)
Yes, we should extend this method to even more decent defaults, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's continue brainstorming how to simplify the general RLModule experience for the user ...
| try: | ||
| module_spec: SingleAgentRLModuleSpec = self.config.rl_module_spec | ||
| module_spec.observation_space = self._env_to_module.observation_space | ||
| # TODO (simon): The `gym.Wrapper` for `gym.vector.VectorEnv` should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that this is gone now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it didn't seem to be a problem anymore (e.g. for PPO Pendulum, everything looks completely fine w/o any weird space errors on the Box actions). So I removed this comment.
…nup_examples_folder_10_custom_rl_module.py
…nup_examples_folder_10_custom_rl_module.py
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
…nup_examples_folder_10_custom_rl_module.py
….py` example script and matching RLModule example class (tiny CNN).. (ray-project#45774) Signed-off-by: Richard Liu <[email protected]>
Cleanup examples folder #10: Add
custom_rl_module.pyexample script and matching RLModule example class (tiny CNN)..Why are these changes needed?
Related issue number
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.