-
Notifications
You must be signed in to change notification settings - Fork 746
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
Reward assignment during recording #518
base: user/michel-aractingi/2024-11-27-port-hil-serl
Are you sure you want to change the base?
Reward assignment during recording #518
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.
Really cool! I like this design ;)
We are in the process of switching to dataset v2. We removed populate_dataset.py and updated scripts/control_robot.py.
By any chance could you rebase on top of user/aliberts/2024_09_25_reshape_dataset
? Sorry for that!!!
You might need to solve some conflicts. Happy to jump on a call if needed or message me on Discord ;)
git fetch origin user/aliberts/2024_09_25_reshape_dataset
git rebase origin/user/aliberts/2024_09_25_reshape_dataset
lerobot/common/datasets/push_dataset_to_hub/aloha_hdf5_format.py
Outdated
Show resolved
Hide resolved
lerobot/common/datasets/push_dataset_to_hub/aloha_hdf5_format.py
Outdated
Show resolved
Hide resolved
Also style check failing was failing. Could you please run our pre-commit? :D
https://github.com/huggingface/lerobot/blob/main/CONTRIBUTING.md#submitting-a-pull-request-pr |
766ed5a
to
2001f16
Compare
nargs="*", | ||
help="Add tags to your dataset on the hub.", | ||
) | ||
# parser_record.add_argument( |
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 commented those because they were raising "unexpected keyword argument" errors. Probably shouldn't be done in this PR. Lmk if you want me to bring them back
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.
We will bring them back after merging Simon's PR ;) thanks for flagging ; same for force-override
cc @aliberts
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 ;) Let's wait for Simon's to merge the PR, then we can merge
nargs="*", | ||
help="Add tags to your dataset on the hub.", | ||
) | ||
# parser_record.add_argument( |
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.
We will bring them back after merging Simon's PR ;) thanks for flagging ; same for force-override
cc @aliberts
lerobot/scripts/control_robot.py
Outdated
dataset.push_to_hub(private=True) | ||
dataset.push_to_hub() |
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.
cc @aliberts
Co-authored-by: Remi <[email protected]>
Co-authored-by: Remi <[email protected]>
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.
Nice work! Left few comments but in general it's ready to merge.
features = {} if features is None else features | ||
features.update(get_features_from_robot(robot, use_videos)) |
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.
What do you think of writing it like this?
features = {} if features is None else features | |
features.update(get_features_from_robot(robot, use_videos)) | |
features = {**(features or {}), **get_features_from_robot(robot)} |
# Allow to exit early while recording an episode or resetting the environment, | ||
# by tapping the right arrow key '->'. This might require a sudo permission | ||
# to allow your terminal to monitor keyboard events. |
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.
Should we add some comments here to explain assign_rewards
?
# Allow to exit early while recording an episode or resetting the environment, | |
# by tapping the right arrow key '->'. This might require a sudo permission | |
# to allow your terminal to monitor keyboard events. | |
""" | |
Initializes a keyboard listener to enable early termination of an episode | |
or environment reset by pressing the right arrow key ('->'). This may require | |
sudo permissions to allow the terminal to monitor keyboard events. | |
Args: | |
assign_rewards (bool): If True, allows annotating the collected trajectory | |
with a binary reward at the end of the episode to indicate success. | |
""" |
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.
Also the same in the comments header of lerobot/scripts/control_robot.py
What this does
Examples:
Those PR is meant to add the possibility for the teleoperator to add rewards when performing the tasks.
The current approach simply consists in having frames labeled 0 until the experimenter presses the space bar. Then the reward for each frame becomes 1. The experimenter can press the bar again (eg. in case of subsequent failure) and the frame labeling will return to 0. Each time the space bar is pressed, the labeling switches
This is one way to go about it, but lmk if you have a better idea!
How it was tested
I made datasets with Moss with and without rewards. Checking that I got the expected behavior.
How to checkout & try? (for the reviewer)