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.

Sign up for GitHub

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

Jump to bottom

[RLlib; Offline RL] 2. Multiple optimizations for streaming data. #49195

Merged
merged 16 commits into from
Dec 13, 2024
Merged

[RLlib; Offline RL] 2. Multiple optimizations for streaming data. #49195

merged 16 commits into from
Dec 13, 2024

Conversation

simonsays1980
Copy link
Collaborator

@simonsays1980 simonsays1980 commented Dec 10, 2024 •
edited
Loading

Why are these changes needed?

Streaming data directly from cloud storage showed still low performance. This PR comes with multiple optimizations to train Offline RL agents in a truly streaming way, i.e. starting training after the first chunks of data are read and preprocessed. The following changes were made:

  • Removing schema calls.
  • Removing customizations of data block numbers.
  • Removing default filesystems.
  • Removing locality_hints from OfflinePreLearner as this is not used anymore.
  • Removing local shuffling of data.
  • Removing corresponding customizations from all our tuned_examples and tests.
  • Changing the RLUnplugged example to
    • Read converted Parquet data to demonstrate the powerful streaming capabilities of Ray Data in our Offline RL API
    • Providing a SPREAD scheduling strategy when using multiple learners on a multi-node cluster.
    • Removing all customizations that slow down data processing.
    • Fixing multiple bugs and improving performance in the custom ConnectorV2 to read stacked and encoded Atari frames

Related issue number

Related to #49194

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 :(

@simonsays1980
Optimized RLUnplugged benchmark example and changed name. Furthermore…
2f526cc 
…, removed default read arguments in 'OfflineData' to always use Ray Data's optimized reads (specifically on the product). Moved call to schema to debug logging in 'OfflineData' to avoid any further overhead when loading a dataset.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Adapted all tuned examples to the optimizations made in this branch.
df8afbe 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Removed call for schema completely to not hurt perforamance anyhow. F…
5a179ae 
…urthermore, removed learner and locality hints from multi-learner setups b/c we do not need them for the time being and actor handles cannot get serialized.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Added a scheduling strategy for multi-node and multi-learner setups. …
a407064 
…Furthermore, removed 'num_cpus' from 'map_batches' as it was blocking execution with multi-learner multi-node setups. In addition modified all #args.num_learner' usages.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Changed dataset iterations per learner to a higher value to improve r…
af757e8 
…educe overhead costs.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Removed local shuffle buffer from 'OfflineData''s defaults b/c it slo…
716fa69 
…ws iterating over batches down as long as it is not fixed in Ray Data.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Merged master and resolved conflicts.
db07517 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980 simonsays1980 added enhancement Request for new feature and/or capability rllib RLlib related issues rllib-offline-rl Offline RL problems labels Dec 10, 2024
@simonsays1980 simonsays1980 marked this pull request as ready for review December 10, 2024 18:18
sven1977
sven1977 reviewed Dec 10, 2024
View reviewed changes
rllib/offline/offline_data.py
@@ -146,8 +147,7 @@ def sample(
# Add constructor `kwargs` when using remote learners.
fn_constructor_kwargs.update(
{
"learner": self.learner_handles,
"locality_hints": self.locality_hints,
"learner": None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, what changed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Locality hints are gone. The learner will go in a third PR blocked by the Ray Data w/ Ray Tune issue

sven1977
sven1977 reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@sven1977 sven1977 left a comment

Choose a reason for hiding this comment

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

Looks good. Just a handful of questions ...

@sven1977 sven1977 enabled auto-merge (squash) December 10, 2024 19:45
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Dec 10, 2024
@simonsays1980 simonsays1980 changed the title [RLlib; Offline RL] Multiple optimizations for streaming data. [RLlib; Offline RL] 2. Multiple optimizations for streaming data. Dec 11, 2024
@sven1977 sven1977 added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. rllib-newstack labels Dec 11, 2024
@github-actions github-actions bot disabled auto-merge December 11, 2024 11:11
sven1977
sven1977 reviewed Dec 13, 2024
View reviewed changes
rllib/offline/tests/test_offline_prelearner.py
@@ -193,7 +192,7 @@ def test_offline_prelearner_sample_from_old_sample_batch_data(self):
algo = self.config.build()
# Build the `OfflinePreLearner` and add the learner.
oplr = OfflinePreLearner(
self.config,
config=self.config,
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

sven1977
sven1977 reviewed Dec 13, 2024
View reviewed changes
rllib/offline/tests/test_offline_prelearner.py
@@ -74,7 +74,7 @@ def test_offline_prelearner_buffer_class(self):
algo = self.config.build()
# Build the `OfflinePreLearner` and add the learner.
oplr = OfflinePreLearner(
self.config,
config=self.config,
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

sven1977
sven1977 reviewed Dec 13, 2024
View reviewed changes
rllib/offline/offline_prelearner.py
@@ -84,9 +84,9 @@ class OfflinePreLearner:
@OverrideToImplementCustomLogic_CallToSuperRecommended
def __init__(
self,
*,
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

sven1977
sven1977 approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@sven1977 sven1977 left a comment

Choose a reason for hiding this comment

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

LGTM now. Thanks @simonsays1980 !

@sven1977 sven1977 merged commit 797dc77 into ray-project:master Dec 13, 2024
5 checks passed
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Dec 17, 2024
@simonsays1980 @ujjawal-khare
[RLlib; Offline RL] 2. Multiple optimizations for streaming data. (ra…
9fa46df 
…y-project#49195)

Signed-off-by: ujjawal-khare <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sven1977 sven1977 sven1977 approved these changes

Assignees
No one assigned
Labels
enhancement Request for new feature and/or capability go add ONLY when ready to merge, run all tests rllib RLlib related issues rllib-newstack rllib-offline-rl Offline RL problems tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@simonsays1980 @sven1977