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] Remove execution plan #25624

Merged
merged 12 commits into from
Jun 14, 2022
Merged

[RLlib] Remove execution plan #25624

merged 12 commits into from
Jun 14, 2022

Conversation

avnishn
Copy link
Member

@avnishn avnishn commented Jun 9, 2022

Remove the final unused execution plans and now unused execution plan utilities

Why are these changes needed?

Related issue number

Checks

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

gjoliver
gjoliver reviewed Jun 9, 2022
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.

awesome! my favorite kind of PR.
thanks a ton man.

rllib/algorithms/ddppo/ddppo.py
max_remote_requests_in_flight_per_worker=1,
ray_wait_timeout_s=0.03,
)
self._curr_learner_info = {}
Copy link
Member

Choose a reason for hiding this comment

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

can you help comment here that ddppo only has torch implementation, so it's ok to do torch specific work here?

rllib/execution/replay_ops.py Outdated
@@ -1,136 +1,10 @@
from typing import List, Any, Optional
from typing import List, Optional
Copy link
Member

Choose a reason for hiding this comment

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

we should be able to delete this whole file?
we should be using Arthur's RBs now right?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're still using the MixInMultiAgentReplayBuffer for IMPALA, until Artur refactors that out

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me get rid of it..

rllib/execution/train_ops.py Outdated
@@ -511,21 +511,10 @@ def __call__(
class UpdateTargetNetwork:
Copy link
Member

Choose a reason for hiding this comment

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

writing the comment here, but it's not related to UpdateTargetNetwork.
can we delete the TrainOneStep and MultiGPUTrainOneStep classes now??

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I hadn't fully deleted these yet at the time that I had uploaded the PR, but they should be deleted now :)

@gjoliver
Copy link
Member

gjoliver commented Jun 9, 2022

lint and 1 unit test failure.
otherwise, this is great!

@avnishn
Copy link
Member Author

avnishn commented Jun 9, 2022

ah forgot to remove a test from BUILD that I deleted

@avnishn avnishn changed the title [RLlib] [WIP] Remove execution plan [RLlib] Remove execution plan Jun 9, 2022
@avnishn
Copy link
Member Author

avnishn commented Jun 9, 2022

ready to merge pending tests passing

@gjoliver
Copy link
Member

lint / doc error doesn't seem related.

@sven1977
Copy link
Contributor

Looks great. Thanks for cleaning this up @avnishn!
There is one problem with the LINTer, it seems:

File "../../python/ray/rllib/execution/parallel_requests.py", line ?, in default
--
  | Failed example:
  | manager.call(lambda worker, a, b: worker.task(a, b), fn_args=[1, 2])
  | Exception raised:
  | Traceback (most recent call last):
  | File "/opt/miniconda/lib/python3.6/doctest.py", line 1330, in __run
  | compileflags, 1), test.globs)
  | File "<doctest default[6]>", line 1, in <module>
  | manager.call(lambda worker, a, b: worker.task(a, b), fn_args=[1, 2])
  | NameError: name 'manager' is not defined

Maybe we need to add these # doctest: +SKIP to the Examples docstring lines?

@suquark
Copy link
Member

suquark commented Jun 10, 2022

@jiaodong it seems this PR might help your rllib prototyping, since it removes a lot of complexity in the code.

@jiaodong
Copy link
Member

@suquark thanks for ccing me here :) 2000 less lines of code sounds great indeed

ArturNiederfahrenhorst
ArturNiederfahrenhorst approved these changes Jun 11, 2022
View reviewed changes
Copy link
Contributor

@ArturNiederfahrenhorst ArturNiederfahrenhorst left a comment

Choose a reason for hiding this comment

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

Great stuff. I checked out the PR and found 1-2 minor occurences of "execution_plan" and changed them myself. When are we going to do MBMPO and MAML? Anything keeping us from writing training iteration functions on these?

@sven1977
Copy link
Contributor

Yeah, we still need to complete MB-MPO and MAML. We left them out for no specific reasons.

@ArturNiederfahrenhorst
Copy link
Contributor

Let's put them on my next sprint, please.

@sven1977 sven1977 merged commit 8047198 into ray-project:master Jun 14, 2022
@mattip
Copy link
Contributor

mattip commented Jun 14, 2022

This PR removed rllib/execution/concurrency_ops.py and rllib/execution/multi_gpu_impl.py which are referred to in the documentation, so the book-lint CI run is failing. here is the documentation source with the file names.

@mattip
Copy link
Contributor

mattip commented Jun 14, 2022

here is the exact reference to concurrency_ops.py and here is the reference to multi_gpu_impl.py

fishbone added a commit that referenced this pull request Jun 14, 2022
@fishbone
Revert "[RLlib] Remove execution plan code no longer used by RLlib. (#…
e590d95 
…25624)"

This reverts commit 8047198.
@fishbone fishbone mentioned this pull request Jun 14, 2022
Merged
fishbone added a commit that referenced this pull request Jun 14, 2022
@fishbone
Revert "[RLlib] Remove execution plan code no longer used by RLlib. (#…
7b8b0f8 
…25624)" (#25776)

This reverts commit 8047198.
@mattip mattip mentioned this pull request Jun 16, 2022
Merged
6 tasks
@ArturNiederfahrenhorst ArturNiederfahrenhorst mentioned this pull request Jul 18, 2022
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArturNiederfahrenhorst ArturNiederfahrenhorst ArturNiederfahrenhorst approved these changes

@gjoliver gjoliver gjoliver approved these changes

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

@smorad smorad Awaiting requested review from smorad

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@kouroshHakha kouroshHakha Awaiting requested review from kouroshHakha

@krfricke krfricke Awaiting requested review from krfricke

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@avnishn @gjoliver @sven1977 @suquark @jiaodong @ArturNiederfahrenhorst @mattip