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] "APPO-accelerate" vol 01: Make AggregatorActors work with IMPALA/APPO. #49284

Merged
merged 64 commits into from
Jan 13, 2025
Merged

[RLlib] "APPO-accelerate" vol 01: Make AggregatorActors work with IMPALA/APPO. #49284

merged 64 commits into from
Jan 13, 2025

Conversation

sven1977
Copy link
Contributor

"APPO-accelerate" vol 01: Make AggregatorActors work with IMPALA/APPO.

  • This PR adds aggregation actor support for APPO and IMPALA on the new API stack.
  • Aggregation actors now own the learner connector offloading the task of translating episodes to batches from the Learner to these workers. They also pre-load the ready batch directly to the correct GPU and pass the ray reference directly to the Learner, thus avoiding any GPU-CPU transfers and allowing the Learner to directly utilize the train batch om the correct GPU.
  • Co-locates them with n Learner actors and makes them configurable as num_aggregation_actors_per_learner(!).
  • Deprecates old AggregationWorkers on old API stack.

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

@sven1977
wip
9664f97 
Signed-off-by: sven1977 <[email protected]>
@sven1977
1194
ad6cc99 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
acea140 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
2b52303 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
27fb72c 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
7380f46 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
0050c8a 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
4465ad2 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
0e8b9a2 
Signed-off-by: sven1977 <[email protected]>
@sven1977
fix
b8397e9 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
409c95b 
Signed-off-by: sven1977 <[email protected]>
@sven1977
1194
123c8b5 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
b8ac459 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
8213928 
Signed-off-by: sven1977 <[email protected]>
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into appo…
07c6b5c 
…_accelerate
@sven1977
up and running (and learning cartpole) w/ 1 aggregation worker on lap…
8a937b3 
…top (local CPU learner).

Signed-off-by: sven1977 <[email protected]>
@sven1977
fix num_gpus=0.01 for aggregator actor
f4510b5 
Signed-off-by: sven1977 <[email protected]>
@sven1977
fix num_gpus=0.01 for aggregator actor
eda58ab 
Signed-off-by: sven1977 <[email protected]>
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into appo…
843be59 
…_accelerate
@sven1977
fixes
e38fcbc 
Signed-off-by: sven1977 <[email protected]>
@sven1977
fix num_gpus=0.01 for aggregator actor
10f2a23 
Signed-off-by: sven1977 <[email protected]>
@sven1977
fix
df18279 
Signed-off-by: sven1977 <[email protected]>
@sven1977
fix
94e02e2 
Signed-off-by: sven1977 <[email protected]>
@sven1977
2x training throughput (2 aggregation workers vs no aggregation worke…
9032031 
…r) on 1 local(!) GPU, 29 EnvRunners:

python cartpole_impala.py --num-env-runners=29 --num-envs-per-env-runner=20 --stop-iters=10 --num-learners=0 --num-gpus-per-learner=0.98

Signed-off-by: sven1977 <[email protected]>
@sven1977
- even with evaluation active (2 env runners), we almost don't have a…
d8edc3c 
…ny slowdowns wrt runs w/o evaluation active; and those might be due to the fact that we have 2 env runners less

Signed-off-by: sven1977 <[email protected]>
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into appo…
086fab1 
…_accelerate
@sven1977
fix adding RLModule to aggregation workers, so all connector pieces c…
e831f89 
…an be reverted to their "normal" master versions (add states and to-numpy).

Signed-off-by: sven1977 <[email protected]>
@sven1977
- add num-agg-actors-per-learner
300e618 
- co-locate each agg-actor with exactly one learner and keep the exact mapping on the algo (to later match, which gpu-batch-ref should go to which learner)
- formalize config option to NOT build learner connector on learner (b/c its already built on agg actors)
- remove garbage code
- deprecate agg workers on old api stack entirely.

Signed-off-by: sven1977 <[email protected]>
@sven1977
fixes
59d300c 
Signed-off-by: sven1977 <[email protected]>
@sven1977
fix
3acc5ba 
Signed-off-by: sven1977 <[email protected]>
@sven1977
fixes
c76f5b1 
Signed-off-by: sven1977 <[email protected]>
@sven1977
fixes
72e30e1 
Signed-off-by: sven1977 <[email protected]>
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into appo…
9420e8f 
…_accelerate
@sven1977
wip
ca0d05b 
Signed-off-by: sven1977 <[email protected]>
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into appo…
4307c6e 
…_accelerate

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

# Conflicts:
#	rllib/algorithms/impala/impala_learner.py
#	rllib/connectors/learner/learner_connector_pipeline.py
@sven1977
wip
b296e8e 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
a888cc4 
Signed-off-by: sven1977 <[email protected]>
@sven1977 sven1977 enabled auto-merge (squash) January 8, 2025 12:13
@github-actions github-actions bot disabled auto-merge January 8, 2025 12:13
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Jan 8, 2025
@sven1977
wip
d968845 
Signed-off-by: sven1977 <[email protected]>
@sven1977
fix
deec8b3 
Signed-off-by: sven1977 <[email protected]>
@sven1977 sven1977 enabled auto-merge (squash) January 8, 2025 16:38
@sven1977
wip
765476b 
Signed-off-by: sven1977 <[email protected]>
@github-actions github-actions bot disabled auto-merge January 9, 2025 11:29
@sven1977
merge
b71fb78 
Signed-off-by: sven1977 <[email protected]>
@sven1977
merge
313f893 
Signed-off-by: sven1977 <[email protected]>
@sven1977
merge
edbcad3 
Signed-off-by: sven1977 <[email protected]>
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into appo…
96c65c6 
…_accelerate
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into appo…
36904c6 
…_accelerate
@sven1977
wip
82cf4da 
Signed-off-by: sven1977 <[email protected]>
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into appo…
69a3b5b 
…_accelerate

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

# Conflicts:
#	rllib/algorithms/algorithm_config.py
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into do_n…
7a86c43 
…ot_finalize_episodes_sent_to_buffer

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

# Conflicts:
#	rllib/utils/replay_buffers/episode_replay_buffer.py
@sven1977
wip
4c66864 
Signed-off-by: sven1977 <[email protected]>
@sven1977 sven1977 enabled auto-merge (squash) January 13, 2025 16:50
simonsays1980
simonsays1980 approved these changes Jan 13, 2025
View reviewed changes
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. Already reviewed for OSS Ray. One small question tothe value of num_aggregate_workers_per_learner which could be None as it looks like in the code.

rllib/algorithms/algorithm.py
0,
(
cf.num_gpus_per_learner
- 0.01 * cf.num_aggregator_actors_per_learner
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this not fail, if cf.num_aggregator_actors_per_learner=None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

B/c it's always some int. By default 0.

@sven1977 sven1977 merged commit 093ed4c into ray-project:master Jan 13, 2025
6 checks passed
@sven1977 sven1977 deleted the appo_accelerate branch January 13, 2025 18:15
srinathk10 pushed a commit that referenced this pull request Feb 2, 2025
@sven1977 @srinathk10
[RLlib] "APPO-accelerate" vol 01: Make AggregatorActors work with IMP…
d99d6c7 
…ALA/APPO. (#49284)
anyadontfly pushed a commit to anyadontfly/ray that referenced this pull request Feb 13, 2025
@sven1977 @anyadontfly
[RLlib] "APPO-accelerate" vol 01: Make AggregatorActors work with IMP…
afb9c40 
…ALA/APPO. (ray-project#49284)

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

@simonsays1980 simonsays1980 simonsays1980 approved these changes

Assignees
No one assigned
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

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