feat: Add RLlib BC and MARWIL - #38
noamonti748 wants to merge 10 commits into
Conversation
Keep BC/MARWIL offline training on top of main's typing cleanup.
…ecutable for data collection
…in in one command
|
|
||
| """ | ||
| Cyclopts template for generating Schola subcommands (multi-simulator and algorithm dispatch). | ||
| """ |
There was a problem hiding this comment.
Why are you modifying command_template.py in this PR? This code has proven difficult for AI to interpret, so there is a good chance these changes were unnecessary, or extremely complicated for what should be a small change.
What issue does the changes to command_template solve?
There was a problem hiding this comment.
It's for sure overkill and will cut back, but the one thing I think it solves is that it allows us to run without binding to a simulator, because the default template binds to external by default whenever a simulator command is not included. Would it be ok to just isolate this part and kill the rest (should be < 50 lines I think)
There was a problem hiding this comment.
The command template should support this without changes (0-N algorithms, 0-N simulators). This case has tests as well so should be working. If you want to change the default sim, just override the function in the child class (it should be ignored when there is no supplied simulators).
If you really want to change it, making it str or None to indicate No Default should be fine. In general, the actual structure is rather delicate and bolting on features rather than building them in properly is error prone.
| @@ -1,29 +1,43 @@ | |||
| # Copyright (c) 2024-2025 Advanced Micro Devices, Inc. All Rights Reserved. | |||
| # Copyright (c) 2024-2026 Advanced Micro Devices, Inc. All Rights Reserved. | |||
There was a problem hiding this comment.
This is a bit of a mess, seems like a separate offline-train command is the way to go, alongside a collect command with functionality matching minari.
There was a problem hiding this comment.
Split commands into these.
| ) | ||
|
|
||
|
|
||
| def load_rl_module_from_algorithm_checkpoint( |
There was a problem hiding this comment.
Does another version of this code exist elsewhere already? I believe this functionality is required by the export command (to load the policy before converting it to a Schola Model API).
There was a problem hiding this comment.
Yep, that's true, but I think the issue is that RLModule load, where the rllib export goes through load_rl_module_from_algorithm_checkpoint or MultiRLModule.from_checkpoint is not shared between them. And of course, the load_rl... functionality is different.
There was a problem hiding this comment.
What about for eval? The comment here says it shares layout handling with eval.
It may also be possible to make eval depend on this, ideally, we keep the number of distinct handlers for loading RLLib objects down lol. Between eval and rllib we already have a lot.
Feel free to make more drastic changes here if you see a way to make everything fit nicely. (e.g a generic Load function that everything else relies on in different places)
| @@ -0,0 +1,480 @@ | |||
| # Copyright (c) 2026 Advanced Micro Devices, Inc. All Rights Reserved. | |||
There was a problem hiding this comment.
How much of this can be outsourced to RLLIB?
As an example, see the JSON Writer for writing the dataset to a file.
There was a problem hiding this comment.
JSONWriter/DatasetWriter seems to use the old-stack data format.
We can use the Parquet + msgpack over write_parquet though.
Also the SingleAgentEpisode
There was a problem hiding this comment.
Sounds good to me.
My philosophy here is that it will be easier in the long run to use the rllib tools, rather than reimplement, where possible since any re-implementations will be coupled to the output of the functions. If the output format changes, we will need to adjust our tools to match.
- Update imitation_learning guide for generic BC/IL with Minari - `command_template` kept to original standard - Shared checkpoint loading path - Use RLlib functions for dataset writing
Summary
The PR enables the use of RLlib's BC and MARWIL with Schola.
Related issues
None.
Type of change
Changes
schola rllib bcandschola rllib marwil--input[offline]pip extra for RLlib offline dependenciesTesting
Python (
Resources/python,Test)pip install --group test -e "./Resources/python[all]"python -m pytest Test --import-mode=importlib -n 0Other Tests
Checklist