-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
refactor(config): Move device & amp args to PreTrainedConfig #812
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 nice! I left a few questions
…ntrol_loop + ensure proper use of device str or torch.device fix(configs/policies): Ensure try_device is a str refactor(ci): update config changes to makefile fix(scripts/control_robot): Take arg from config instead of policy refactor(robot_devices/control_utils): Remove device and use_amps args from the record_episode and control_loop functions fix(config): ensure consistency when using device as str or as a torch.device
5bd03d3
to
d2046d4
Compare
for more information, see https://pre-commit.ci
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 LGTM (modulo my comment over device from pretrained) but I realize there's currently one big caveat for this:
Using --policy.path
with some --policy
overrides currently doesn't work. We should probably do something about it before merging. WDYT?
What this does
Related to: #683
This PR moves the
device
anduse_amp
configuration arguments fromTrainPipelineConfig
class to thePreTrainedConfig
class. This change affects some other classes and functions in the code path, including:__post_init__
method of thePreTrainedConfig
classmake_policy
: No longer needsdevice
input arg, as this is now incfg
from_pretrained
: No longer needsdevice
input arg, as this is now inconfig
PreTrainedConfig
object or inheriting from its class no longer need thedevice
anduse_amp
attributesPreTrainedConfig
class.In order to fully integrate and test this PR (in the CI), we will need to merge these sisters PRs in our HF Hub repository for the public models:
diffusion_pusht
: https://huggingface.co/lerobot/diffusion_pusht/discussions/3vqbet_pusht
: https://huggingface.co/lerobot/vqbet_pusht/discussions/3pi0
: https://huggingface.co/lerobot/pi0/discussions/11act_aloha_sim_transfer_cube_human
: https://huggingface.co/lerobot/act_aloha_sim_transfer_cube_human/discussions/4act_aloha_sim_insertion_human
: https://huggingface.co/lerobot/act_aloha_sim_insertion_human/discussions/2/filesNote: This PR is meant to be squash merged
How it was tested
uv run pytest
didn't have any failing testHow to checkout & try? (for the reviewer)
uv run pytest