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.

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 Franka Simulation package to SERL #1

Merged
merged 9 commits into from
Dec 13, 2023
Merged

Add Franka Simulation package to SERL #1

merged 9 commits into from
Dec 13, 2023

Conversation

Leo428
Copy link
Collaborator

@Leo428 Leo428 commented Dec 8, 2023

This PR includes a simple Franka lift cube Gym environment and its assets. Installation and testing instructions are in the README.md.

Thank you for reviewing!

@Leo428 Leo428 requested a review from youliangtan December 8, 2023 23:36
@Leo428 Leo428 requested a review from jianlanluo December 8, 2023 23:49
Copy link
Member

@youliangtan youliangtan left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, i briefly tried to run it. Manage to run the frank_sim env.

high level feedback, it would be great to install all packages directly from the level-0 serl directory, instead needing to go in the individual readme.

Maybe something like this:

find . -name 'requirements.txt' -execdir pip3 install -r {} \;
find . -name 'setup.py' -execdir pip3 install -e . \;

# run the env
python franka_sim/franka_sim/test/test_gym_env_human.py

README.md Outdated

## Installation
- Conda Environment:
- create an environment with `conda create -n serl_dev python=3.10`
Copy link
Member

Choose a reason for hiding this comment

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

use serl as conda env name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, just fixed this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the requirements.txt, are you thinking about putting all the package dependencies into one requirements.txt? Originally, I was thinking about having a requirement_sim.txt so users can optionally choose if they want to try sim.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I mean that the "coverpage" readme should cover the instructions on how to run and install the code, without needing the user to find it. So just a short code snippet of how to install it will do. The above code snippet is just a cli to install all underlying pkgs in the repo.

In this case that the sim is optional, we can add it soon.

Copy link
Collaborator

@jianlanluo jianlanluo left a comment

Choose a reason for hiding this comment

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

this generally looks good to me

@youliangtan youliangtan merged commit 9529a1e into main Dec 13, 2023
1 check passed
@youliangtan youliangtan deleted the franka_sim branch December 13, 2023 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants