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] Add support to directly read from episodes. #46865

Merged
merged 45 commits into from
Aug 12, 2024
Merged

[RLlib; Offline RL] Add support to directly read from episodes. #46865

merged 45 commits into from
Aug 12, 2024
+49 −12

Conversation

simonsays1980
Copy link
Collaborator

@simonsays1980 simonsays1980 commented Jul 30, 2024 •
edited
Loading

Why are these changes needed?

The Offline RL API in the new stack will soon support recording capabilities (see #46818) that allow direclty storing RLlib's episodes (i.e. SingleAgentEpisodes). In regard to this upcoming change this PR proposes a direct episode readfing option for the OfflinePreLearner that can skip the _map_to_episodes step in the map_batches dta pipeline and therefore increase training speed.

Related issue number

Relates to #46818

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]>
@simonsays1980
Removed tensorflow and fixed a small bug.
7785bda 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Readded 'input_read_schema' b/c it was accidentally removed.
3e680cc 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Readded further tests for MARWIL on continuous actions and its loss f…
90c8d03 
…unction.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Added default 'prelearner_class' to 'MARWILConfig'.
8a8d5c5 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Added example to 'tuned_examples' for MARWIL.
5f051e0 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Added BC and MARWIL tuned_examples to learning tests.
f247e7c 
Signed-off-by: simonsays1980 <[email protected]>
@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]>
@simonsays1980
Merge branch 'master' into marwil-rl-module
e7b05c7
@simonsays1980
Fixed a circular import in 'test_offline_data'.
f7f9d89 
Signed-off-by: simonsays1980 <[email protected]>
@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]>
sven1977
sven1977 reviewed Jul 30, 2024
View reviewed changes
rllib/BUILD Outdated Show resolved Hide resolved
sven1977
sven1977 reviewed Jul 30, 2024
View reviewed changes
rllib/algorithms/marwil/marwil_offline_prelearner.py Outdated
# TODO (simon): episodes are only needed for logging here.
return {"batch": [batch]}

def _compute_gae_from_episodes(
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance we can share this code with PPO's?

Create a utility function in PPO folder (e.g. algorithms/ppo/utils.py), then import and use this same function for MARWIL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's the question I have: DO we really need the complete code? Or can we get rid of either the batch or episodes in their when considering MARWIL and the offline learning pipeline?

@simonsays1980
Fixed file path for MARWIL learning test in BUILD.
b027890 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Merge branch 'marwil-rl-module' into offline-prelearner-from-episodes
1b589bf
@simonsays1980
Added @sven1977's review.
e86e2dd 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Added new test file to test folder.
429b5f7 
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 'marwil-rl-module' into offline-prelearner-from-episodes
fff9db1
@simonsays1980
Removed override of 'get_default_learner_class' in BC. BC takes now t…
d2e9a08 
…he learner from MARWIL.

Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Merged Master
cf39a2e 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Removed duplicate and wrong MARWIL tuned example learning test.
351c8a1 
Signed-off-by: simonsays1980 <[email protected]>
@simonsays1980
Fixed some linting errors.
fcca1b4 
Signed-off-by: simonsays1980 <[email protected]>
sven1977
sven1977 approved these changes Aug 8, 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.

LGTM now! Thanks for this PR @simonsays1980 !

@sven1977 sven1977 enabled auto-merge (squash) August 8, 2024 10:42
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Aug 8, 2024
@github-actions github-actions bot disabled auto-merge August 9, 2024 09:21
@sven1977 sven1977 merged commit 936427f into ray-project:master Aug 12, 2024
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.
2 participants
@simonsays1980 @sven1977