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] Fix small memory leak in OfflineSingleAgentEnvRunner. #48309

Merged
merged 16 commits into from
Dec 2, 2024
Merged

[RLlib; Offline RL] Fix small memory leak in OfflineSingleAgentEnvRunner. #48309

merged 16 commits into from
Dec 2, 2024

Conversation

simonsays1980
Copy link
Collaborator

@simonsays1980 simonsays1980 commented Oct 29, 2024 •
edited
Loading

Why are these changes needed?

There was a small memory leak that became apparent when recording for a longer time. This leak was mainly due to output_max_rows_per_file too small for the sample size. This PR

  • fixes this small memory leak,
  • fixes a file-naming typo in the last written records when stopping a run
  • adds a new configuration option output_write_remaining_data that defines if the recording buffers in an OfflineSingleAgentEnvRunner should be cleared to disk when stopping recording.
  • changes the recording example to a more realistic use case.

Related issue number

Closes #48025

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
Removed references to fix a small memory leak in the 'OfflineSingleAg…
00040c9 
…entEnvRunner'.

Signed-off-by: simonsays1980 <simon.zehnder@gmail.com>
@simonsays1980 simonsays1980 added rllib RLlib related issues rllib-system system issues, runtime env, oom, etc rllib-offline-rl Offline RL problems labels Oct 29, 2024
@simonsays1980 simonsays1980 marked this pull request as ready for review October 29, 2024 10:58
@sven1977 sven1977 changed the title [RLlib; Offline RL] - Fix small memory leak in OfflineSingleAgentEnvRunner. [RLlib; Offline RL] Fix small memory leak in OfflineSingleAgentEnvRunner. Oct 29, 2024
sven1977
sven1977 reviewed Oct 29, 2024
View reviewed changes
rllib/offline/offline_env_runner.py Outdated
@@ -164,6 +164,8 @@ def sample(
# TODO (simon): Find a better way to write these data.
self._samples = self._samples[self.output_max_rows_per_file :]
samples_ds = ray.data.from_items(samples_to_write)
# Delete reference to free memory.
del samples_to_write
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to do this? Python's GC should take care of it when the variable goes out of scope, no?
It also keeps all the data anyways b/c it's just a slice from the self._samples ndarray.

@simonsays1980
Merge branch 'master' into fix-small-memory-leak-and-stopping-in-offl…
d03138d 
…ine-env-runner
@simonsays1980
Added multiple iterations over recording buffer and implemented a met…
607ff26 
…ric. Furthermore, added another user configuration 'output_write_remaining_data' to decide, if remaining data from the recording buffers of EnvRunners should be written to disk. Finally, updated the recording example to show a more realistic recording use case.

Signed-off-by: simonsays1980 <simon.zehnder@gmail.com>
sven1977
sven1977 approved these changes Nov 22, 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.

Very nice PR!!

While we are at this, can we convert this into an actual example and add it to the CI? This will bolsert our offline RL documentation efforts.

@simonsays1980
Added docstring for new attribute 'output_write_remaining_data'.
c7c7b62 
Signed-off-by: simonsays1980 <simon.zehnder@gmail.com>
@simonsays1980
Moved recording script to examples/offline_rl/ and added docstrings. …
71a6c80 
…Implemented some changes suggested in @sven1977's review.

Signed-off-by: simonsays1980 <simon.zehnder@gmail.com>
@simonsays1980
Merge branch 'master' into fix-small-memory-leak-and-stopping-in-offl…
ba66164 
…ine-env-runner
@simonsays1980
Switched test of example 'cartpole_recording' to 'large' as it was ti…
6108f89 
…ming out in CI tests.

Signed-off-by: simonsays1980 <simon.zehnder@gmail.com>
@simonsays1980
Merge branch 'master' into fix-small-memory-leak-and-stopping-in-offl…
f8541e9 
…ine-env-runner
@simonsays1980
Added longer timeouts for different stages of recording example as au…
f8e6d75 
…toscaler tells in CI test errors that resources can not be fulfilled.

Signed-off-by: simonsays1980 <simon.zehnder@gmail.com>
@simonsays1980
Limited resources for each data context in recording to the resources…
eea6358 
… of the corresponding 'EnvRunner'.

Signed-off-by: simonsays1980 <simon.zehnder@gmail.com>
@simonsays1980
Added more CPUs to CI test for recording because it was failing const…
411f7f6 
…antly due to resource requests.

Signed-off-by: simonsays1980 <simon.zehnder@gmail.com>
@simonsays1980
Merge branch 'master' into fix-small-memory-leak-and-stopping-in-offl…
95b4f2b 
…ine-env-runner
@simonsays1980
Removed recording test from CI tests as it was failing with timeouts …
c8692d7 
…but still runs on local and cloud tests.

Signed-off-by: simonsays1980 <simon.zehnder@gmail.com>
@simonsays1980
Merge branch 'master' into fix-small-memory-leak-and-stopping-in-offl…
b789135 
…ine-env-runner
@simonsays1980
Removed double ray.shutdown from recording example.
e71df86 
Signed-off-by: simonsays1980 <simon.zehnder@gmail.com>
@sven1977 sven1977 enabled auto-merge (squash) December 2, 2024 08:54
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Dec 2, 2024
@github-actions github-actions bot disabled auto-merge December 2, 2024 12:11
@sven1977 sven1977 merged commit 0e9f3d1 into ray-project:master Dec 2, 2024
5 checks passed
jecsand838 pushed a commit to jecsand838/ray that referenced this pull request Dec 4, 2024
@simonsays1980 @jecsand838
[RLlib; Offline RL] Fix small memory leak in `OfflineSingleAgentEnvRu…
0319d34 
…nner`. (ray-project#48309)

Signed-off-by: Connor Sanders <connor@elastiflow.com>
dentiny pushed a commit to dentiny/ray that referenced this pull request Dec 7, 2024
@simonsays1980 @dentiny
[RLlib; Offline RL] Fix small memory leak in `OfflineSingleAgentEnvRu…
d35ebb9 
…nner`. (ray-project#48309)

Signed-off-by: hjiang <dentinyhao@gmail.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Dec 17, 2024
@simonsays1980 @ujjawal-khare
[RLlib; Offline RL] Fix small memory leak in `OfflineSingleAgentEnvRu…
e53085e 
…nner`. (ray-project#48309)

Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
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
go add ONLY when ready to merge, run all tests rllib RLlib related issues rllib-offline-rl Offline RL problems rllib-system system issues, runtime env, oom, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RLlib; Offline RL] - SingleAgentOfflineEnvRunner runs into OOM
2 participants
@simonsays1980 @sven1977