-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add tests for image deletion and fix mixed image-video deletion #2592
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
base: main
Are you sure you want to change the base?
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.
Pull request overview
This PR fixes a critical bug in temporary image cleanup that was breaking batch video encoding for mixed image-video datasets. The fix ensures that only image features have their temporary directories deleted after each episode, while video features retain their temporary images until batch encoding is triggered.
- Corrects
clear_episode_buffer()to only delete temporary images for image features (dtype="image"), not all camera keys - Adds comprehensive test coverage for image deletion behavior across different scenarios (image-only, video with batch_encoding_size=1, video with batch_encoding_size>1, and mixed datasets)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/lerobot/datasets/lerobot_dataset.py | Changes clear_episode_buffer() to use image_keys instead of camera_keys when deleting temporary image directories, preventing premature deletion of video feature temporary images |
| tests/datasets/test_datasets.py | Adds 4 new test functions to verify correct temporary image deletion behavior for different feature types and batch encoding settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bf630c5 to
81a5526
Compare
Co-authored-by: Copilot <[email protected]> Signed-off-by: Alex Tyshka <[email protected]>
|
@CarolinePascal This is an update to #1751 post-dataset-v3 if you have a chance to review |
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What this does
Currently, if you have a dataset with both image and video features and batch_encoding_size > 1, the tmp images are deleted for both image features and video features at the end of every episode. These images are then missing when batch video encoding is triggered. This fix ensures that only image features are deleted at the end of an episode.
How it was tested
Added 4 new tests to test_datasets.py testing image deletion for image features, unbatched video, batched video, and mixed datasets.
PLEASE NOTE: the test for batched video is currently failing due to #2404 and requires the fix in #2462
How to checkout & try? (for the reviewer)