Repository navigation
Additional fixes - #754
Additional fixes#754aliberts merged 11 commits into
Conversation
Cadene
left a comment
Cadene
left a comment
There was a problem hiding this comment.
Looks good!! thanks :)
I left a few comments that might need to be addressed before merging.
Looking forward to removing import torch from lerobot_dataset.py
| check_timestamps_sync(self.hf_dataset, self.episode_data_index, self.fps, self.tolerance_s) | ||
| timestamps = torch.stack(self.hf_dataset["timestamp"]).numpy() | ||
| episode_indices = torch.stack(self.hf_dataset["episode_index"]).numpy() | ||
| ep_data_index_np = {k: t.numpy() for k, t in self.episode_data_index.items()} | ||
| check_timestamps_sync(timestamps, episode_indices, ep_data_index_np, self.fps, self.tolerance_s) |
There was a problem hiding this comment.
I am a bit worried that this check will take a lot of time for BIG datasets with a few GB of hf_dataset.
Droid is 2GB of hf_dataset, so it's ok-ish.
Should we add an option to disable it int LeRobotDataset init ?
Should we do the check in save_episode only? Why do we need it in init ?
There was a problem hiding this comment.
I think it's okay to just keep it for now as it's only run in the init and relatively fast still.
| def _save_episode_table(self, episode_buffer: dict, episode_index: int) -> None: | ||
| episode_dict = {key: episode_buffer[key] for key in self.hf_features} | ||
| ep_dataset = datasets.Dataset.from_dict(episode_dict, features=self.hf_features, split="train") | ||
| ep_dataset = embed_images(ep_dataset) | ||
| self.hf_dataset = concatenate_datasets([self.hf_dataset, ep_dataset]) | ||
| self.hf_dataset.set_transform(hf_transform_to_torch) | ||
| ep_data_path = self.root / self.meta.get_data_file_path(ep_index=episode_index) | ||
| ep_data_path.parent.mkdir(parents=True, exist_ok=True) | ||
| write_parquet(ep_dataset, ep_data_path) | ||
| ep_dataset.to_parquet(ep_data_path) |
There was a problem hiding this comment.
I am not sure to understand what this function is doing. Comments could help
concatenate_datasets([self.hf_dataset, ep_dataset]) seems expensive.
Why do we embed images? ep_dataset is not supposed to contain images no?
There was a problem hiding this comment.
Images are always embedded for the parquet to be portable. This is doing the same operations as before but in a slightly different order.
We used to call _save_episode_table (which embedded the images) then load_hf_dataset in save_episode, which is very slow and expensive. Now I'm just appending the new episode table to the existing hf_dataset. Except that in order to do that I first need to embed the images because we delete episode images in save_episodes and so it would keep the paths to these deleted files, which raises an error when trying to read them.
There was a problem hiding this comment.
But we dont have images, we have videos. I am not sure to understand.
There was a problem hiding this comment.
For when we do have images (e.g. pusht_image)
| upper_versions = [v for v in hub_versions if v > target_version] | ||
| assert len(upper_versions) > 0 | ||
| raise ForwardCompatibilityError(repo_id, min(upper_versions)) |
What this does
"language_instruction"feature which was left in some openX datasets'sinfo.jsonload_hf_datasetcall fromsave_episodeand use np.ndarrays incheck_timestamps_syncHow it was tested
Updated the tests accordingly