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] Cleanup examples folder #13. Fix main examples docs page for RLlib. #45382

Merged

Conversation

sven1977
Copy link
Contributor

@sven1977 sven1977 commented May 16, 2024

Cleanup examples folder #13. Fix main examples docs page for RLlib.

Why are these changes needed?

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • 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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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 :(

Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
…p_examples_folder_11_fractional_gpus

# Conflicts:
#	rllib/utils/test_utils.py
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
@sven1977 sven1977 added rllib RLlib related issues rllib-docs-or-examples Issues related to RLlib documentation or rllib/examples rllib-newstack rllib-oldstack-cleanup Issues related to cleaning up classes, utilities on the old API stack labels May 16, 2024
Signed-off-by: sven1977 <[email protected]>
Copy link
Contributor

@angelinalg angelinalg left a comment

Choose a reason for hiding this comment

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

Just some style nits and a few typos.

doc/source/rllib/rllib-advanced-api.rst Outdated Show resolved Hide resolved
doc/source/rllib/rllib-advanced-api.rst Outdated Show resolved Hide resolved
doc/source/rllib/rllib-advanced-api.rst Outdated Show resolved Hide resolved
doc/source/rllib/rllib-advanced-api.rst Outdated Show resolved Hide resolved
doc/source/rllib/rllib-advanced-api.rst Outdated Show resolved Hide resolved
`num_gpus_per_worker` to 0 (they may be set to 1 by default for your
particular RL algorithm)."""
machine does not have any GPUs, you should set the config keys
`num_gpus_per_learner` and `num_gpus_per_env_runner` to 0 (they may be set to
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
`num_gpus_per_learner` and `num_gpus_per_env_runner` to 0 (they may be set to
`num_gpus_per_learner` and `num_gpus_per_env_runner` to 0. They may be set to

particular RL algorithm)."""
machine does not have any GPUs, you should set the config keys
`num_gpus_per_learner` and `num_gpus_per_env_runner` to 0 (they may be set to
1 by default for your particular RL algorithm)."""
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
1 by default for your particular RL algorithm)."""
1 by default for your particular RL algorithm."""

particular RL algorithm)."""
machine does not have any GPUs, you should set the config keys
`num_gpus_per_learner` and `num_gpus_per_env_runner` to 0 (they may be set to
1 by default for your particular RL algorithm)."""

ERR_MSG_INVALID_ENV_DESCRIPTOR = """The env string you provided ('{}') is:
a) Not a supported/installed environment.
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
a) Not a supported/installed environment.
a) Not a supported or installed environment.

@@ -1346,6 +1347,11 @@ def run_rllib_example_script_experiment(
tune_callbacks: A list of Tune callbacks to configure with the tune.Tuner.
In case `args.wandb_key` is provided, will append a WandB logger to this
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
In case `args.wandb_key` is provided, will append a WandB logger to this
In case `args.wandb_key` is provided, appends a WandB logger to this

keep_config: Set this to True, if you don't want this utility to change the
given `base_config` in any way and leave it as-is. This is helpful
for example script that want to demonstrate how to set those settings
that are usually taken care of automatically in this function (e.g.
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
that are usually taken care of automatically in this function (e.g.
that are usually taken care of automatically in this function (e.g.,

…nup_examples_folder_13_folder_readme

Signed-off-by: sven1977 <[email protected]>

# Conflicts:
#	doc/source/rllib/rllib-advanced-api.rst
#	doc/source/rllib/rllib-learner.rst
#	rllib/BUILD
#	rllib/algorithms/algorithm.py
#	rllib/algorithms/algorithm_config.py
#	rllib/algorithms/dreamerv3/tests/test_dreamerv3.py
#	rllib/core/learner/learner.py
#	rllib/core/learner/scaling_config.py
#	rllib/examples/checkpoints/restore_1_of_n_agents_from_checkpoint.py
#	rllib/examples/gpus/fractional_gpus_per_learner.py
#	rllib/tuned_examples/dreamerv3/atari_100k.py
#	rllib/tuned_examples/dreamerv3/atari_200M.py
#	rllib/tuned_examples/dreamerv3/dm_control_suite_vision.py
#	rllib/utils/test_utils.py
Signed-off-by: sven1977 <[email protected]>
sven1977 and others added 12 commits June 10, 2024 12:21
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Co-authored-by: angelinalg <[email protected]>
Signed-off-by: Sven Mika <[email protected]>
Co-authored-by: angelinalg <[email protected]>
Signed-off-by: Sven Mika <[email protected]>
Co-authored-by: angelinalg <[email protected]>
Signed-off-by: Sven Mika <[email protected]>
Signed-off-by: sven1977 <[email protected]>
…r_readme' into cleanup_examples_folder_13_folder_readme
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
@sven1977 sven1977 changed the title [RLlib] Cleanup examples folder 13: Add READMEs to folder and all sub-folders. [RLlib] Cleanup examples folder #13. Fix main examples docs page for RLlib. Jun 11, 2024
Signed-off-by: sven1977 <[email protected]>
…nup_examples_folder_13_folder_readme

Signed-off-by: sven1977 <[email protected]>

# Conflicts:
#	rllib/examples/inference/policy_inference_after_training.py
Signed-off-by: sven1977 <[email protected]>
@sven1977 sven1977 enabled auto-merge (squash) June 11, 2024 13:52
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Jun 11, 2024
Copy link
Collaborator

@simonsays1980 simonsays1980 left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -114,7 +114,7 @@ The following figure shows *synchronous sampling*, the simplest of `these patter

RLlib uses `Ray actors <actors.html>`__ to scale training from a single core to many thousands of cores in a cluster.
You can `configure the parallelism <rllib-training.html#specifying-resources>`__ used for training by changing the ``num_env_runners`` parameter.
Check out our `scaling guide <rllib-training.html#scaling-guide>`__ for more details here.
See this `scaling guide <rllib-training.html#scaling-guide>`__ for more details here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The scaling guide also needs to be overhauled.

CUDA devices. For example if `os.environ["CUDA_VISIBLE_DEVICES"] = "1"`
then a `local_gpu_idx` of 0 will use the GPU with ID=1 on the node.
and `local_gpu_idx=0`, RLlib uses the GPU with ID=1 on the node.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels counterintuitive. The GPU index 0 does not equal the environment variable 1 and we have two or more GPUs for a single learner. A user would expect a single GPU for a single learner when multiple GPUs are available on a node to be indicated with an ID or index. Do I misunderstand sth here?

`num_learners` x `train_batch_size_per_learner` and can
be accessed via the property `AlgorithmConfig.total_train_batch_size`.
`num_learners` x `train_batch_size_per_learner` and you can
access it with the property `AlgorithmConfig.total_train_batch_size`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should refer hereto in the scaling guide ~ if not done yet.

@github-actions github-actions bot disabled auto-merge June 12, 2024 10:03
Signed-off-by: sven1977 <[email protected]>
@sven1977 sven1977 enabled auto-merge (squash) June 12, 2024 11:21
@sven1977 sven1977 merged commit c84bf37 into ray-project:master Jun 12, 2024
7 checks passed
@sven1977 sven1977 deleted the cleanup_examples_folder_13_folder_readme branch June 12, 2024 12:59
richardsliu pushed a commit to richardsliu/ray that referenced this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests rllib RLlib related issues rllib-docs-or-examples Issues related to RLlib documentation or rllib/examples rllib-newstack rllib-oldstack-cleanup Issues related to cleaning up classes, utilities on the old API stack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants