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.

Sign up for GitHub

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

Jump to bottom

Add distinction between two unallowed cases in name check "eval_" #489

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Add distinction between two unallowed cases in name check "eval_" #489

merged 2 commits into from
Nov 22, 2024

Conversation

J4nn1K
Copy link
Contributor

@J4nn1K J4nn1K commented Oct 27, 2024

What this does

Title Label
Fixes error in sanity_check_dataset_name() 🐛 Bug

There was a bug within the sanity_check_dataset_name() function. When the control_robot.py script was executed for evaluation and the repo-id did not start with "eval_" it was raising a wrong ValueError.

This caused some confusion because it was unclear that the prefix "eval_" is needed.

How it was tested

  • Added distinction between two possible error cases and checked that evaluation works with policy act_so100_real on our own dataset.

How to checkout & try? (for the reviewer)

Examples:
This should raise the right error message:
(Taken from the SO-100 Guide)

python lerobot/scripts/control_robot.py record \
  --robot-path lerobot/configs/robot/so100.yaml \
  --fps 30 \
  --root data \
  --repo-id ${HF_USER}/act_so100_test \
  --tags so100 tutorial eval \
  --warmup-time-s 5 \
  --episode-time-s 40 \
  --reset-time-s 10 \
  --num-episodes 10 \
  -p outputs/train/act_so100_test/checkpoints/last/pretrained_model

@Cadene
Copy link
Collaborator

Cadene commented Nov 22, 2024

Thanks Jannik! Sorry for the late follow up.

Style check failing. Could you please run our pre-commit? :D

pre-commit run --all-files

https://github.com/huggingface/lerobot/blob/main/CONTRIBUTING.md#submitting-a-pull-request-pr

Then we are ready to merge

@Cadene Cadene self-assigned this Nov 22, 2024
@Cadene Cadene self-requested a review November 22, 2024 13:02
Cadene
Cadene approved these changes Nov 22, 2024
View reviewed changes
Copy link
Collaborator

@Cadene Cadene left a comment

Choose a reason for hiding this comment

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

Looks good to me. I didnt test it locally.

@Cadene Cadene merged commit 975c1c2 into huggingface:main Nov 22, 2024
6 checks passed
ChorntonYoel pushed a commit to ChorntonYoel/lerobot that referenced this pull request Nov 22, 2024
@J4nn1K @ChorntonYoel
Add distinction between two unallowed cases in name check "eval_" (hu…
2001f16 
…ggingface#489)
astroyat pushed a commit to astroyat/lerobot that referenced this pull request Nov 23, 2024
@J4nn1K @astroyat
Add distinction between two unallowed cases in name check "eval_" (hu…
962aed8 
…ggingface#489)
astroyat pushed a commit to astroyat/lerobot that referenced this pull request Nov 23, 2024
@J4nn1K @astroyat
Add distinction between two unallowed cases in name check "eval_" (hu…
5dee8f0 
…ggingface#489)
DomThePorcupine pushed a commit to DomThePorcupine/lerobot that referenced this pull request Dec 2, 2024
@J4nn1K @DomThePorcupine
Add distinction between two unallowed cases in name check "eval_" (hu…
1e3dd76 
…ggingface#489)
Cadene pushed a commit that referenced this pull request Dec 28, 2024
@J4nn1K @Cadene
Add distinction between two unallowed cases in name check "eval_" (#489)
cb0d183
villekuosmanen added a commit to villekuosmanen/lerobot that referenced this pull request Dec 30, 2024
@villekuosmanen @HiroIshida
@alexander-soare @ohharsen @ivelin @brainwavecoder9 @resolver101757 @J4nn1K @KasparSLT @aliberts @Cadene @michel-aractingi @ma3oun @Raziel90 @s1lent4gnt @mishig25 @helper2424
chore: sync fork with upstream (#3)
913bcab 
* feat: enable to use multiple rgb encoders per camera in diffusion policy (huggingface#484)

Co-authored-by: Alexander Soare <[email protected]>

* Fix config file (huggingface#495)

* fix: broken images and a few minor typos in README (huggingface#499)

Signed-off-by: ivelin <[email protected]>

* Add support for Windows (huggingface#494)

* bug causes error uploading to huggingface, unicode issue on windows. (huggingface#450)

* Add distinction between two unallowed cases in name check "eval_" (huggingface#489)

* Rename deprecated argument (temporal_ensemble_momentum) (huggingface#490)

* Dataset v2.0 (huggingface#461)

Co-authored-by: Remi <[email protected]>

* Refactor OpenX (huggingface#505)

* Fix missing local_files_only in record/replay (huggingface#540)

Co-authored-by: Simon Alibert <[email protected]>

* Control simulated robot with real leader (huggingface#514)

Co-authored-by: Remi <[email protected]>

* Update 7_get_started_with_real_robot.md (huggingface#559)

* LerobotDataset pushable to HF from any folder (huggingface#563)

* Fix example 6 (huggingface#572)

* fixing typo from 'teloperation' to 'teleoperation' (huggingface#566)

* [vizualizer] for LeRobodDataset V2 (huggingface#576)

* Fix broken `create_lerobot_dataset_card`  (huggingface#590)

* feat(act): support training end of episode token to ACT model

* changes

* feat(arx): add arx arm (#2)

* feat(arx): support arx arm

* changes

* changes

* changes

* changes

* pass pipes explicitly

* changes

* us ndarray over a pipe

* changes

* changes

* replay basically works

* patch arx sdk

* changes

* support cameras in arx5

* rename to arx5

* kind of works

* changes

* changes

* changes

* various changes

* changes

* revert a few changes

* changes

* changes

* changes

* changes

* changes

* changes

* changes

* changes

* changes

* remove TODO

* allow multiple tasks

---------

Signed-off-by: ivelin <[email protected]>
Co-authored-by: Hirokazu Ishida <[email protected]>
Co-authored-by: Alexander Soare <[email protected]>
Co-authored-by: Arsen Ohanyan <[email protected]>
Co-authored-by: Ivelin Ivanov <[email protected]>
Co-authored-by: Daniel Ritchie <[email protected]>
Co-authored-by: resolver101757 <[email protected]>
Co-authored-by: Jannik Grothusen <[email protected]>
Co-authored-by: KasparSLT <[email protected]>
Co-authored-by: Simon Alibert <[email protected]>
Co-authored-by: Remi <[email protected]>
Co-authored-by: Michel Aractingi <[email protected]>
Co-authored-by: Simon Alibert <[email protected]>
Co-authored-by: berjaoui <[email protected]>
Co-authored-by: Claudio Coppola <[email protected]>
Co-authored-by: s1lent4gnt <[email protected]>
Co-authored-by: Mishig <[email protected]>
Co-authored-by: Eugene Mironov <[email protected]>
villekuosmanen added a commit to villekuosmanen/lerobot that referenced this pull request Jan 10, 2025
@villekuosmanen @HiroIshida
@alexander-soare @ohharsen @ivelin @brainwavecoder9 @resolver101757 @J4nn1K @KasparSLT @aliberts @Cadene @michel-aractingi @ma3oun @Raziel90 @s1lent4gnt @mishig25 @helper2424 @CharlesCNorton @philfung
feat: sync upstream changes (#4)
9e38286 
* feat: enable to use multiple rgb encoders per camera in diffusion policy (huggingface#484)

Co-authored-by: Alexander Soare <[email protected]>

* Fix config file (huggingface#495)

* fix: broken images and a few minor typos in README (huggingface#499)

Signed-off-by: ivelin <[email protected]>

* Add support for Windows (huggingface#494)

* bug causes error uploading to huggingface, unicode issue on windows. (huggingface#450)

* Add distinction between two unallowed cases in name check "eval_" (huggingface#489)

* Rename deprecated argument (temporal_ensemble_momentum) (huggingface#490)

* Dataset v2.0 (huggingface#461)

Co-authored-by: Remi <[email protected]>

* Refactor OpenX (huggingface#505)

* Fix missing local_files_only in record/replay (huggingface#540)

Co-authored-by: Simon Alibert <[email protected]>

* Control simulated robot with real leader (huggingface#514)

Co-authored-by: Remi <[email protected]>

* Update 7_get_started_with_real_robot.md (huggingface#559)

* LerobotDataset pushable to HF from any folder (huggingface#563)

* Fix example 6 (huggingface#572)

* fixing typo from 'teloperation' to 'teleoperation' (huggingface#566)

* [vizualizer] for LeRobodDataset V2 (huggingface#576)

* Fix broken `create_lerobot_dataset_card`  (huggingface#590)

* Update README.md (huggingface#612)

* Fix Quality workflow (huggingface#622)

* fix(docs): typos in benchmark readme.md (huggingface#614)

Co-authored-by: Simon Alibert <[email protected]>

* fix(visualise): use correct language description for each episode id (huggingface#604)

Co-authored-by: Simon Alibert <[email protected]>

* typo fix: batch_convert_dataset_v1_to_v2.py (huggingface#615)

Co-authored-by: Simon Alibert <[email protected]>

* [viz] Fixes & updates to html visualizer (huggingface#617)

* fixes to SO-100 readme (huggingface#600)

Co-authored-by: Philip Fung <no@one>
Co-authored-by: Simon Alibert <[email protected]>

---------

Signed-off-by: ivelin <[email protected]>
Co-authored-by: Hirokazu Ishida <[email protected]>
Co-authored-by: Alexander Soare <[email protected]>
Co-authored-by: Arsen Ohanyan <[email protected]>
Co-authored-by: Ivelin Ivanov <[email protected]>
Co-authored-by: Daniel Ritchie <[email protected]>
Co-authored-by: resolver101757 <[email protected]>
Co-authored-by: Jannik Grothusen <[email protected]>
Co-authored-by: KasparSLT <[email protected]>
Co-authored-by: Simon Alibert <[email protected]>
Co-authored-by: Remi <[email protected]>
Co-authored-by: Michel Aractingi <[email protected]>
Co-authored-by: Simon Alibert <[email protected]>
Co-authored-by: berjaoui <[email protected]>
Co-authored-by: Claudio Coppola <[email protected]>
Co-authored-by: s1lent4gnt <[email protected]>
Co-authored-by: Mishig <[email protected]>
Co-authored-by: Eugene Mironov <[email protected]>
Co-authored-by: CharlesCNorton <[email protected]>
Co-authored-by: Philip Fung <[email protected]>
Co-authored-by: Philip Fung <no@one>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cadene Cadene Cadene approved these changes

Assignees

@Cadene Cadene

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@J4nn1K @Cadene