-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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 update: New Algorithm checkpoints, Policy checkpoints and Policy model exports in native format #28812
[RLlib] Docs update: New Algorithm checkpoints, Policy checkpoints and Policy model exports in native format #28812
Conversation
…l_export_overhaul # Conflicts: # rllib/BUILD
…l_export_overhaul
…l_export_overhaul Signed-off-by: sven1977 <svenmika1977@gmail.com> # Conflicts: # rllib/examples/export/onnx_tf.py # rllib/examples/export/onnx_torch.py # rllib/policy/dynamic_tf_policy_v2.py # rllib/policy/eager_tf_policy_v2.py # rllib/policy/tests/test_policy.py # rllib/policy/torch_policy_v2.py
…l_export_overhaul
…l_export_overhaul
…l_export_overhaul
…l_export_overhaul
doc/source/_toc.yml
Outdated
@@ -214,6 +214,7 @@ parts: | |||
- file: rllib/user-guides | |||
sections: | |||
- file: rllib/rllib-models | |||
- file: rllib/rllib-checkpoints-and-exports |
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.
cool, thanks! @sven1977 as this is a user guide, I think you also want to add this new doc to the panels
in user-guides.rst
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.
hmm, maybe we should take a closer look at that gallery, too. For instance, I see that "connectors" are also not in this gallery, although they show up in the TOC (and main navigation of the docs). this should align
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.
- was already added to user-guides.rst
- @gjoliver on adding connectors docs to list of RLlib user guides (
user-guides.rst
).
|
||
from ray.rllib.algorithms.ppo import PPOConfig # noqa | ||
|
||
# Create a new Algorithm (which contains a Policy, which contains a NN Model). |
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.
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.
basically this import and maybe the last one suffer from this a little, other than that I think this looks great btw.
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.
I'll split it up and move the comments into the rsv file.
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.
Left the keras-related comments in the code, but split up the 3 different ways on how to save your models (direct, via policy checkpoint, via algo checkpoint).
…_update_checkpointing_and_exports
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.
Almost there, just the TOC and include file.
doc/source/rllib/rllib-examples.rst
Outdated
@@ -140,7 +140,7 @@ Serving and Offline | |||
- `Saving experiences <https://github.com/ray-project/ray/blob/master/rllib/examples/saving_experiences.py>`__: | |||
Example of how to externally generate experience batches in RLlib-compatible format. | |||
- `Finding a checkpoint using custom criteria <https://github.com/ray-project/ray/blob/master/rllib/examples/checkpoint_by_custom_criteria.py>`__: | |||
Example of how to find a checkpoint after a `Tuner.fit()` via some custom defined criteria. | |||
Example of how to find a `checkpoint <rllib-saving-and-loading-algos-and-policies.html>`__ after a `Tuner.fit()` via some custom defined criteria. |
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 a big deal, but using a :ref:
here is more stable and prevents outdated links when moving files etc.
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.
Done, but can you check, whether I did this correctly. Not sure how sphinx infers the actual html file. E.g. I don't find any serve-rllib-tutorial
either (referenced further above this one here).
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.
@sven1977 yeah, I don't like the way Sphinx solves this. essentially, if there's a file called serve-rllib-tutorial.md
in the source, relative to the same file, you can use that as doc ref, otherwise (and this is usually much better), you'd just add a .. _serve-rllib-tutorial:
tag in rst or (serve-rllib-tutorial)=
in markdown in the respective doc and reference that, see here:
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.
so in this case, please put .. _rllib-saving-and-loading-algos:
at the top of your new doc/source/rllib/rllib-saving-and-loading-algos-and-policies.rst
file. then you can reference it as:
:ref:`my reference <rllib-saving-and-loading-algos>`
this should also fix the build
@@ -0,0 +1,308 @@ | |||
# flake8: noqa | |||
|
|||
# __create-algo-checkpoint-begin__ |
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.
By convention (and I think this is a good one to keep consistent), we have a doc_code
folder like here for Tune:
https://github.com/ray-project/ray/tree/master/doc/source/tune/doc_code
which has all the code referenced in docs. You can then basically copy this block in the bazel BUILD
file to auto-test all imported snippets. I'm aware that this would likely also happen with rllib/examples
but I think it'd be great to keep this code close to doc-sources (which e.g. makes it likely easier for new contributors to understand the structure).
https://github.com/ray-project/ray/blob/master/doc/BUILD#L155-L164
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.
Should be a simple move... apart from that, this is exactly what I had in mind! :D
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.
Dumb question on this one:
If we currently run all rllib/examples/documentation/* via the CI, how would we reference into the new location (docs/source/rllib/doc_code/*) from the rllib/BUILD file?
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 simply consider those two different CI runs. I didn't realise there was an rllib/examples/documentation
folder already. We could either leave it as is, or (in a follow-up PR) move the contents of that folder into doc/source/rllib/doc_code
. We can then delete that part from the rllib/BUILD
and add the resp. bit to doc/BUILD
. that way we don't have to think about the issue you brought up. wdyt?
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 simply skip this discussion for now and fix this later, don't think it's a big deal either way
…_update_checkpointing_and_exports
…_update_checkpointing_and_exports
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.
approving, pending the ref fix
…_update_checkpointing_and_exports
…d Policy model exports in native format (ray-project#28812) Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
Docs update: New Algorithm checkpoints, Policy checkpoints and Policy model exports in native format
Details:
On Algorithm checkpoints:
my_algo.restore([some AIR checkpoint])
works as well asAlgorithm.restore([some path to a checkpoint dir])
. The checkpoint directory structure will change from:to:
from_checkpoint()
andfrom_state()
, both of which return new Algorithm objects, given a checkpoint dir or object or a state dict, respectively. I.e.:my_new_algo = Algorithm.from_checkpoint([path to AIR checkpoint OR AIR checkpoint obj])
. No original config or other information is needed other than the checkpoint.On Policy Checkpoints:
Policy.export_checkpoint()
produces an AIR Checkpoint directory with all the policy's state in it.from_checkpoint()
andfrom_state()
, both of which return new Policy objects, given a Policy checkpoint dir or object or a Policy state, respectively.On native keras/PyTorch models being part of a Policy checkpoint (optional):
config.checkpointing(checkpoints_contain_native_model_files=True)
makes Policies also try to write their NN model as native keras/torch saved model into the given checkpoint directory (under sub-dir "model"). This may still fail (gracefully) in some cases, e.g. for certain TfModelV2 where the kerasself.base_model
(of the TfModelV2) cannot be discovered easily. This problem will be fully solved by the ongoing RLModule/RLTrainer API efforts.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.