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] MARWIL RLModule #44970

Merged
merged 38 commits into from
Aug 1, 2024
Merged

[RLlib] MARWIL RLModule #44970

merged 38 commits into from
Aug 1, 2024
+1,538 −673

Conversation

simonsays1980
Copy link
Collaborator

@simonsays1980 simonsays1980 commented Apr 25, 2024 •
edited
Loading

Why are these changes needed?

This PR implements MARWIL in the new API stack using Learner API and RLModule API. It relates to the proposal for the new Offline Data API to be used in its training step.

Related issue number

#44969
Closes #37775

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
Initiated MARWIL RL Module and added catalog, learner and tf_learner.
cbfd05f 
Signed-off-by: Simon Zehnder <[email protected]>
@simonsays1980
Added MARWIL RL Module and started to write test.
c488da7 
Signed-off-by: Simon Zehnder <[email protected]>
@simonsays1980
Merge branch 'master' into marwil-rl-module
9078af8 
Signed-off-by: Simon Zehnder <[email protected]>
@simonsays1980
Implemented Torch version of MARWIL.
b4e1795 
Signed-off-by: Simon Zehnder <[email protected]>
@simonsays1980
Added torch learner.
5eeb2e6 
Signed-off-by: Simon Zehnder <[email protected]>
@simonsays1980
Merged master.
a1928bc 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Moved trainign step logic from BC to MARWIL.
3fcef32 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Setup MARWIL with the new stack using 'OfflineData'. This is an unfor…
e9abc27 
…matted commit merely for securing the work.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Fixed multiple bugs in 'MARWILOfflinePreLearner', 'MARWILTorchLearner…
c930464 
…' and 'MARWILTorchPolicy', fixed imports and tested MARWIL on non-recurrent policies.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
LINTER.
8b575db 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Merged Master
1325beb 
Signed-off-by: simonsays1980 <[email protected]>
@sven1977 sven1977 changed the title [RLlib] - MARWIL RLModule [RLlib] MARWIL RLModule Jul 24, 2024
@sven1977 sven1977 marked this pull request as ready for review July 24, 2024 15:36
sven1977
sven1977 reviewed Jul 25, 2024
View reviewed changes
rllib/algorithms/bc/tests/test_bc.py
@@ -59,7 +63,7 @@ def test_bc_compilation_and_learning_from_offline_file(self):
results = algo.train()
print(results)

eval_results = results.get("evaluation", {})
eval_results = results.get(EVALUATION_RESULTS, {})
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

sven1977
sven1977 reviewed Jul 25, 2024
View reviewed changes
rllib/algorithms/bc/torch/bc_torch_learner.py Outdated
@@ -49,11 +50,13 @@ def possibly_masked_mean(t):
mask = None
possibly_masked_mean = torch.mean

action_dist_class_train = self.module[module_id].get_train_action_dist_cls()
action_dist_class_train = (
self.module[module_id].unwrapped().get_train_action_dist_cls()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but I wonder whether we should make the MARLModule return already the unwrapped RLModule automatically when we access a sub-module through __getitem__(). ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we need anywhere the wrapped module? I guess not even in the DDP case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, hold on, good point. The most user-friendly way is probably to make sure the wrapper exposes:

  • all RLModule base methods.
  • all API methods that the wrapped module implements -> So the wrapper should check, what RLModule APIs (ValueFunctionAPI, TargetNetworkAPI, etc..) its wrapped module implements, then expose all this API's methods as well automatically. ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is how it could work. Its not a very elegant design but I tried around some weeks ago to make it elegant by just defining all methods not yet published from the wrapped module and this is non-trivial. Have to check agaon what the reasons were.

sven1977
sven1977 reviewed Jul 25, 2024
View reviewed changes
rllib/algorithms/marwil/tests/test_marwil.py

config = (
marwil.MARWILConfig()
.env_runners(num_env_runners=1)
.api_stack(
enable_rl_module_and_learner=True,
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 Jul 25, 2024
View reviewed changes
rllib/algorithms/marwil/tests/test_marwil.py

config = (
marwil.MARWILConfig()
.api_stack(
enable_rl_module_and_learner=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!!

@simonsays1980
Moved definition of prelearner class from 'MARWILConfig.offline_data'…
96d1e8e 
… to 'OfflineData'. Set return to reach higher for tuned example.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Fixed path for data in BUILD.
9a29bf1 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Removed a duplicated forward slash from BUILD file that led to errors…
0dc7c8a 
… in linting and building.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Merged master.
061804e 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Added main to MARWIL RLModule test.
ff41d12 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Added tests for old stack MARWIL.
122dc50 
Signed-off-by: simonsays1980 <[email protected]>
@ray-project ray-project deleted a comment from simonsays1980 Jul 29, 2024
sven1977
sven1977 approved these changes Jul 29, 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 great! Let's get this merged :) Thanks @simonsays1980 for this great PR!

@sven1977 sven1977 enabled auto-merge (squash) July 29, 2024 06:59
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Jul 29, 2024
@github-actions github-actions bot disabled auto-merge July 29, 2024 08:57
@simonsays1980
Added 'finalize' to 'OfflinePreLearner._map_to_episodes' b/c some con…
e0a0351 
…nectors request finalized episodes.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Set 'OfflinePreLearner' as BC's default prelearner class in the confi…
5632061 
…g as this was giving an error when 'MARWILOfflinePreLearner' tried to call a value function unneeded by BC. Deprecated hybrid stack.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Fixed a small bug in MARWIL when writing to the metrics logger and se…
9409a1e 
…tting 'beta=0.0'.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Deprecated TensorFlow support for BC and removed bc-specific learners…
8222454 
…. BC depends now fully on MARWIL.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Fixed old stack test by removing hybrid stack.
5afc3ab 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Fixed file path for MARWIL learning test in BUILD.
b027890 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Merged Master
94dd369 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Fixed pretrain BC example as it was using the hybrid stack which is d…
0e73c14 
…epcrecated. Moved to old stack as it uses policies.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Removed training step for hybrid stack and renamed test files.
c781c58 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Fixed small typo.
b216d6a 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Merge branch 'master' into marwil-rl-module
84ea8ff
@simonsays1980
Removed override of 'get_default_learner_class' in BC. BC takes now t…
e57045f 
…he learner from MARWIL.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Fixed path in BUILD test for MARWIL Learning test.
09f10db 
Signed-off-by: simonsays1980 <[email protected]>
@sven1977 sven1977 merged commit 9df091a into ray-project:master Aug 1, 2024
4 of 5 checks passed
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

@ArturNiederfahrenhorst ArturNiederfahrenhorst Awaiting requested review from ArturNiederfahrenhorst

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.

[RLlib] Enable RLModule by default on MARWIL
2 participants
@simonsays1980 @sven1977