-
Notifications
You must be signed in to change notification settings - Fork 37
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.
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
Add SERL launcher with a Franka Arm from state example #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR. Some high-level thoughs.
Install edgeml
from the original repo. This can easily be done by including the git path and commit hash in setup.py
. This will automatically pull and install the code during pip3 install -e .
in serl_launcher
. The main idea here is that edgeml
will get maintained overtime, and we try to prevent duplication of code when possible.
After these changes, i can try run the simulation code.
|
||
setup( | ||
name="edgeml", | ||
version="0.1.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serl_launcher
package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I can change that.
"typing_extensions", | ||
"opencv-python", | ||
"lz4", | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Iam maintaining edgeml
, so it is better not to duplicate code
install_requires=[
....... TODO DEPENDENCIES
"'edgeml @ git+https://github.com/youliangtan/edgeml.git@COMMIT_HASH',
],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this still work if I added additional code for an efficient online RL replay buffer in the /data folder?
Also, for SERL's use cases, some of the features in edgeml are not used. Shall we meet some times tomorrow? I'd love to hear your suggestions in person.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you will just add it to serl_launcher/data
and the custom code will import edgeml
as library.
It is okay that features are not used. Think edgeml
as a numpy
library, you only use classes that you required. If you make custom changes to edgeml, either open a PR, or just have the code in serl_launcher
.
README.md
Outdated
|
||
2. Install RL library | ||
- the examples here use jaxrl-minimal as the RL library. | ||
- To install jaxrl-minimal, `git clone https://github.com/rail-berkeley/jaxrl_minimal/tree/serl_dev`, the `serl_dev` branch is based off the latest `main` branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no diff in serl_dev
and main
in jaxrl_minimal
, is that what it suppose to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch is created for future use, we are going to add in encoder stuff, DrQ, and VICE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventually, we should merge changes back to the main branch though. This is for our convenience when testing and running code for now.
@@ -0,0 +1,195 @@ | |||
from functools import partial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be similar to the one in jaxrl_m
. If there's diff, place it in serl_dev
, or keep it here for now (both options are good)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it can be in either places. I'm not placing it in jaxrl_minimal for now because I'm not sure if anyone else's code depends on the specifics of the dataset and replay_buffer implementation in jaxrl_minimal. Although I highly recommend this version of the memory efficient replay buffer as it is fast and compact when doing image-based online RL training.
from flax.core import frozen_dict | ||
from gymnasium.spaces import Box | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this package is now call serl_launcher
, the path should be:
serl_launcher/serl_launcher/data/memory_efficient_replay_buffer.py
Hi @youliangtan, I made the changes per your requests. Some side notes: When running jaxrl-minimal, you might run into some bugs related to chex and tensorflow versions. You can run |
Signed-off-by: youliang <[email protected]>
Signed-off-by: youliang <[email protected]>
I managed to test-run the environment and run the actor and learner bash scripts. Did minor code cleanups, and also updated the Readme doc. srry for the nitpicking, but is a good practice to use code block rather than code line in markdown (easier for users to copy) Note: need to update chex to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this works well. I set it up and ran it with a clean env. Havnt try to run long enough since i only tried it on a CPU setup. Can do it for the next iteration
Nice! with CPU it might take a bit longer to converge. Thank you for the code improvements! |
Hi all,
This PR includes:
Thank you so much for reviewing!
Notes:
chex
andtensorflow
versions to run training correctly. Maybe jaxrl-minimal needs a pip requirement update at some point?https://github.com/rail-berkeley/jaxrl_minimal/tree/serl_dev
. This branch is based on and identical to themain
branch. I will add DrQ and encoder-related changes later this week once this PR is through.