-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[RLlib; docs] Docs do-over (new API stack): RLModules vol 03; Actual rst rewrite and upgrade. #49387
[RLlib; docs] Docs do-over (new API stack): RLModules vol 03; Actual rst rewrite and upgrade. #49387
Conversation
…edo_rl_module_002 # Conflicts: # rllib/algorithms/appo/torch/appo_torch_rl_module.py
…edo_rl_module_002
…_redo_rl_module_001_default_rl_modules
…edo_rl_module_002
…_redo_rl_module_002
…edo_rl_module_002
…_redo_rl_module_003_actual_docs
…_redo_rl_module_003_actual_docs # Conflicts: # rllib/algorithms/appo/default_appo_rl_module.py # rllib/algorithms/appo/torch/default_appo_torch_rl_module.py # rllib/algorithms/bc/torch/default_bc_torch_rl_module.py # rllib/algorithms/dqn/default_dqn_rl_module.py # rllib/algorithms/ppo/default_ppo_rl_module.py # rllib/algorithms/ppo/torch/default_ppo_torch_rl_module.py
Co-authored-by: angelinalg <122562471+angelinalg@users.noreply.github.com> Signed-off-by: Sven Mika <sven@anyscale.io>
doc/source/rllib/rllib-rlmodule.rst
Outdated
"action_dist_inputs": ... | ||
# RLlib: | ||
# - Generates distribution from ACTION_DIST_INPUTS parameters. | ||
# - Samples from the (stochastic) distribution. |
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.
# - Samples from the (stochastic) distribution. | |
# - Samples from the stochastic distribution. |
…_redo_rl_module_003_actual_docs
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. Very nice changes!
|
||
RLlib turns on the new API stack by default for all RLlib algorithms. To deactivate it, use the `api_stack()` method | ||
in your `AlgorithmConfig` object like so: | ||
RLlib turns on the new API stack by default for all RLlib algorithms. |
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.
Didn't I change this already in here: #49304 ?
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, but I removed the "to deactivate it (<- the new API stack), ...". I think we shouldn't describe to users how to deactivate the new API stack on the migration guide site. It's too confusing. Basically, we are saying now: "It's already on, don't worry! But you can double-check your config by doing xyz ..."
|
||
.. testcode:: | ||
.. note:: | ||
To **deactivate** the new API stack and switch back to the old one, use the |
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.
We should leave here a comment for when we see the old stack being deprectaed and not supported anymore. I see way too many users on the discussion forums to go with the old stack, even when starting their journey. I always mention that the old stack will be deprecated soon, but without a date.
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.
Yup! Let's change the "new stack" header everywhere describing that they should ONLY use the new stack from here on (unless they have very good reasons to still use the old one). ...
module_class=PPOTorchRLModule, | ||
catalog_class=MARWILCatalog, | ||
) | ||
return RLModuleSpec(module_class=PPOTorchRLModule) |
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.
DefaultPPOTorchRLModule
?
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 catch. These are covered, though, by backward compatible imports in the old py-files.
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.
changed
|
||
@classmethod | ||
@override(Learner) | ||
def rl_module_required_apis(cls) -> list[type]: |
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.
Nice!
@@ -1565,6 +1549,36 @@ def _make_module(self) -> MultiRLModule: | |||
|
|||
return module | |||
|
|||
def rl_module_is_compatible(self, module: RLModule) -> bool: |
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.
Yes, this is what we can use now where almost everything is defined by interfaces.
@@ -1,9 +1,7 @@ | |||
import torch |
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.
No try_import_torch
anymopre needed?
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.
Good point, but I think we can leave the bare-metal import torch
everywhere that's not in the "common" import path (and thus doesn't break anything for non-torch users).
…_redo_rl_module_003_actual_docs
…_redo_rl_module_003_actual_docs
…rst rewrite and upgrade. (#49387)
…rst rewrite and upgrade. (ray-project#49387) Signed-off-by: Puyuan Yao <williamyao034@gmail.com>
Docs do-over (new API stack): RLModules vol 03
Why are these changes needed?
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.