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] Deflake replay buffer demo - Up test size to leave a little more time for ReplayBufferDemo #31429

Merged
merged 3 commits into from
Jan 5, 2023
Merged

[RLlib] Deflake replay buffer demo - Up test size to leave a little more time for ReplayBufferDemo #31429

merged 3 commits into from
Jan 5, 2023

Conversation

ArturNiederfahrenhorst
Copy link
Contributor

@ArturNiederfahrenhorst ArturNiederfahrenhorst commented Jan 4, 2023 •
edited
Loading

Signed-off-by: Artur Niederfahrenhorst artur@anyscale.com

Why are these changes needed?

The replay buffer demo likes to take approx 10 iterations to reach the nice reward of 50 but often gets cancelled earlier (for example after 7 like in the picture) because it is marked as a test of size medium.

Screenshot 2023-01-04 at 12 26 33

Screenshot 2023-01-04 at 12 27 51

@ArturNiederfahrenhorst
up test size to leave a little more time
14d4722 
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
gjoliver
gjoliver reviewed Jan 4, 2023
View reviewed changes
Copy link
Member

@gjoliver gjoliver left a comment

Choose a reason for hiding this comment

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

need 15 mins to run 10 iterations??
what is taking all this time? should we just make an iteration shorter by setting min_time_s_per_iteration?

@ArturNiederfahrenhorst
make medium again and add workers
32c8af3 
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
@ArturNiederfahrenhorst
Copy link
Contributor Author

@gjoliver min_time_s_per_iteration is None by default, but we could lower minimum samples per iteration from 1000 to something lower. However then the script probably would never learn, also not on user's machines.
I've given it three rolloutworkers now and I'm guessing its running much quicker locally. Should we try that?

@gjoliver
Copy link
Member

gjoliver commented Jan 4, 2023

@gjoliver min_time_s_per_iteration is None by default, but we could lower minimum samples per iteration from 1000 to something lower. However then the script probably would never learn, also not on user's machines. I've given it three rolloutworkers now and I'm guessing its running much quicker locally. Should we try that?

how long does it take to reach 50 now that we have 3 workers? does it ever stop because of 10-iter limit?

@ArturNiederfahrenhorst
Copy link
Contributor Author

@gjoliver min_time_s_per_iteration is None by default, but we could lower minimum samples per iteration from 1000 to something lower. However then the script probably would never learn, also not on user's machines. I've given it three rolloutworkers now and I'm guessing its running much quicker locally. Should we try that?

how long does it take to reach 50 now that we have 3 workers? does it ever stop because of 10-iter limit?

It usually stops bc of the 10-iter limit.
On my own machine, execution time is halved:

Screenshot 2023-01-05 at 01 01 58

Screenshot 2023-01-05 at 00 55 40

@ArturNiederfahrenhorst
Copy link
Contributor Author

Should be more than enough to make this green I guess

@gjoliver
Copy link
Member

gjoliver commented Jan 5, 2023

this is very cool!

@gjoliver gjoliver merged commit a96d5de into ray-project:master Jan 5, 2023
@ArturNiederfahrenhorst
Copy link
Contributor Author

Yeah let's hope it has a super similar effect on CI! 🥳

@ArturNiederfahrenhorst ArturNiederfahrenhorst deleted the replaybufferflakeyness branch January 5, 2023 15:39
AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023
@ArturNiederfahrenhorst @AmeerHajAli
[RLlib] Deflake replay buffer demo - Use more rollout workers for Rep…
daa1329 
…layBufferDemo (#31429)

make medium again and add workers

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
tamohannes pushed a commit to ju2ez/ray that referenced this pull request Jan 25, 2023
@ArturNiederfahrenhorst @tamohannes
[RLlib] Deflake replay buffer demo - Use more rollout workers for Rep…
79206e3 
…layBufferDemo (ray-project#31429)

make medium again and add workers

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: tmynn <hovhannes.tamoyan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gjoliver gjoliver gjoliver left review comments

@krfricke krfricke krfricke approved these changes

@sven1977 sven1977 Awaiting requested review from sven1977 sven1977 is a code owner

@avnishn avnishn Awaiting requested review from avnishn

@smorad smorad Awaiting requested review from smorad

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@kouroshHakha kouroshHakha Awaiting requested review from kouroshHakha

Assignees

@sven1977 sven1977

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ArturNiederfahrenhorst @gjoliver @krfricke @sven1977