Skip to content
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 API reference pages: rllib/execution, rllib/evaluation, rllib/models, rllib/offline. #20538

Merged
merged 31 commits into from
Dec 10, 2021

Conversation

sven1977
Copy link
Contributor

@sven1977 sven1977 commented Nov 18, 2021

Docs API reference pages: rllib/execution, rllib/evaluation, rllib/models, rllib/offline.

In a follow up PR: rllib/utils (this would then complete the API reference pages overhaul).

Why are these changes needed?

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@sven1977 sven1977 added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Nov 19, 2021
@sven1977 sven1977 changed the title [RLlib; Docs] Docs API reference pages: rllib/execution, rllib/evaluation, rllib/models, rllib/offline. [WIP!! RLlib; Docs] Docs API reference pages: rllib/execution, rllib/evaluation, rllib/models, rllib/offline. Nov 26, 2021
@sven1977
Copy link
Contributor Author

sven1977 commented Dec 1, 2021

This is ready for review now. @richardliaw
Thanks!

@sven1977 sven1977 changed the title [WIP!! RLlib; Docs] Docs API reference pages: rllib/execution, rllib/evaluation, rllib/models, rllib/offline. [RLlib; Docs] Docs API reference pages: rllib/execution, rllib/evaluation, rllib/models, rllib/offline. Dec 3, 2021
…_api_reference_pages_3

# Conflicts:
#	rllib/models/modelv2.py

All RLlib neural network models have to be provided as ModelV2 sub-classes.

.. autoclass:: ray.rllib.models.modelv2.ModelV2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, this isn't showing up properly:

classray.rllib.models.modelv2.ModelV2(obs_space: , action_space: , num_outputs: int, model_config: dict, name: str, framework: str)[source]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an existing problem (not introduced by this PR) with gym.spaces.Space, it seems. See e.g. my screenshot below. I'm not sure how to fix this. Sphinx replaces the gym.spaces.Space typehint with some <Mock name='mock.Space' id='...'>-tag ?? (could be because of the string "Space" having a special meaning for Sphinx?).

.. autoclass:: ray.rllib.models.tf.tf_modelv2.TFModelV2
:members:

TorchModelV2 (rllib.env.models.torch.torch_modelv2.TorchModelV2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2021-12-06 at 8 44 53 PM

the name here doesn't render properly, i would suggest removing the location string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me. I am getting weird text for the gym.spaces.Space typehints, like:

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw I was talking about the sidebar.

Also we have to fix the mock spaces

InputReaders
------------

The InputReader API is used by individual :py:class:`~ray.rllib.evaluation.rollout_worker.RolloutWorker`s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't render this properly:
Uploading Screen Shot 2021-12-06 at 8.45.53 PM.png…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

you can use the built-in :py:class:`~ray.rllib.offline.json_reader.JsonReader` class.
You will have to change the ``input`` config setting from "sampler" (default) to a JSON file name (str), a list of JSON files,
or a path name (str) that contains json files.
Alternatively, you can specify a callable that returns a new :py:class:`~ray.rllib.offline.input_reader.InputReader`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"s3://bucket/expert.json": 0.2,
}

.. autoclass:: ray.rllib.offline.mixed_input.MixedInput
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are missing a reference to IOContext in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good catch. Will add. ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


Returns:
MultiAgentBatch: This very MultiAgentBatch.
This very MultiAgentBatch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -342,11 +338,11 @@ def columns(self, keys: List[str]) -> List[any]:
return out

@PublicAPI
def shuffle(self) -> None:
def shuffle(self) -> "SampleBatch":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice cleanup!

Comment on lines 13 to 16
.. https://docs.google.com/drawings/d/1OewMLAu6KZNon7zpDfZnTh9qiT6m-3M9wnkqWkQQMRc/edit
.. figure:: ../../images/rllib/rollout_worker_class_overview.svg
:align: left

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is too big! can you scale it down?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:width: 600

:align: left

**A typical RLlib WorkerSet setup inside an RLlib Trainer:** Each ``WorkerSet`` contains
exactly one local ``RolloutWorker`` object and n @ray.remote ``RolloutWorkers`` (ray actors).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
exactly one local ``RolloutWorker`` object and n @ray.remote ``RolloutWorkers`` (ray actors).
exactly one local ``RolloutWorker`` object and n ray remote ``RolloutWorkers`` (ray actors).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

**A typical RLlib WorkerSet setup inside an RLlib Trainer:** Each ``WorkerSet`` contains
exactly one local ``RolloutWorker`` object and n @ray.remote ``RolloutWorkers`` (ray actors).
The workers contain a policy map (with one or more policies), and - in case a simulator
(env) is available - a vectorized BaseEnv (containing m sub-environments) and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

link out to BaseEnv?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Links to all mentioned classes added.

Comment on lines +16 to +17
.. autoclass:: ray.rllib.evaluation.rollout_worker.RolloutWorker
:special-members: __init__
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

man I would consider hiding the docstring of the constructor; it's super long and not intended for users i think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it's important to keep it for consistency. Users may ask: Why is the init docstring visible in the reference pages for e.g. TorchPolicy, but not for RolloutWorker? Like what's the rule?

@sven1977 sven1977 merged commit f814c2a into ray-project:master Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants