-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix double sharding in ShuffleBuffer #9020
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: dev
Are you sure you want to change the base?
Changes from all commits
4000d9a
e525693
cb3a713
2d59f56
3a4e975
bdb9d30
ef27a72
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -25,6 +25,23 @@ | |||||||||||||||||||
| pd, _ = optional_import("pandas") | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def _source_shards_by_worker(data: Iterable[Any]) -> bool: | ||||||||||||||||||||
| """Return whether the source declares that its iterator partitions by worker. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Args: | ||||||||||||||||||||
| data: iterable source to inspect through its type's method resolution order. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Returns: | ||||||||||||||||||||
| ``True`` if the first class defining ``__iter__`` declares a truthy | ||||||||||||||||||||
| ``_shards_by_worker`` value on itself, or ``False`` if the declaration | ||||||||||||||||||||
| is missing or false. | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| for source_type in type(data).__mro__: | ||||||||||||||||||||
| if "__iter__" in source_type.__dict__: | ||||||||||||||||||||
| return bool(source_type.__dict__.get("_shards_by_worker", False)) | ||||||||||||||||||||
| return False | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| class IterableDataset(_TorchIterableDataset): | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| A generic dataset for iterable data source and an optional callable data transform | ||||||||||||||||||||
|
|
@@ -40,6 +57,8 @@ class IterableDataset(_TorchIterableDataset): | |||||||||||||||||||
|
|
||||||||||||||||||||
| """ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| _shards_by_worker = True | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def __init__(self, data: Iterable[Any], transform: Callable | None = None) -> None: | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| Args: | ||||||||||||||||||||
|
|
@@ -75,6 +94,13 @@ class ShuffleBuffer(Randomizable, IterableDataset): | |||||||||||||||||||
| every iter() call, refer to the PyTorch idea: | ||||||||||||||||||||
| https://github.com/pytorch/pytorch/blob/v1.10.0/torch/utils/data/distributed.py#L98. | ||||||||||||||||||||
| epochs: number of epochs to iterate over the dataset, default to 1, -1 means infinite epochs. | ||||||||||||||||||||
| source_shards_by_worker: whether ``data`` already partitions its stream | ||||||||||||||||||||
| using ``torch.utils.data.get_worker_info``. ``None`` automatically | ||||||||||||||||||||
| recognizes built-in MONAI sources that declare worker partitioning. | ||||||||||||||||||||
| A subclass that overrides iteration without declaring that capability | ||||||||||||||||||||
| is treated as unsharded. ``True`` avoids a second worker partition | ||||||||||||||||||||
| for any worker-aware source, and ``False`` preserves the outer | ||||||||||||||||||||
| partition for unsharded iterable datasets. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Note: | ||||||||||||||||||||
| Both ``monai.data.DataLoader`` and ``torch.utils.data.DataLoader`` do not seed this class (as a subclass of | ||||||||||||||||||||
|
|
@@ -97,11 +123,37 @@ def run(): | |||||||||||||||||||
|
|
||||||||||||||||||||
| """ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def __init__(self, data, transform=None, buffer_size: int = 512, seed: int = 0, epochs: int = 1) -> None: | ||||||||||||||||||||
| _shards_by_worker = True | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def __init__( | ||||||||||||||||||||
| self, | ||||||||||||||||||||
| data, | ||||||||||||||||||||
| transform=None, | ||||||||||||||||||||
| buffer_size: int = 512, | ||||||||||||||||||||
| seed: int = 0, | ||||||||||||||||||||
| epochs: int = 1, | ||||||||||||||||||||
| source_shards_by_worker: bool | None = None, | ||||||||||||||||||||
| ) -> None: | ||||||||||||||||||||
| """Initialize the shuffle buffer. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Args: | ||||||||||||||||||||
| data: input data source to load, shuffle, and optionally transform. | ||||||||||||||||||||
| transform: a callable data transform applied to each yielded item. | ||||||||||||||||||||
| buffer_size: maximum number of items stored before random popping. | ||||||||||||||||||||
| seed: random seed used to initialize the worker random states. | ||||||||||||||||||||
| epochs: number of source iterations, where ``-1`` means infinite. | ||||||||||||||||||||
| source_shards_by_worker: whether ``data`` already partitions its | ||||||||||||||||||||
| stream using ``torch.utils.data.get_worker_info``. ``None`` | ||||||||||||||||||||
| automatically recognizes built-in MONAI sources that declare | ||||||||||||||||||||
| worker partitioning. | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| super().__init__(data=data, transform=transform) | ||||||||||||||||||||
| self.size = buffer_size | ||||||||||||||||||||
| self.seed = seed | ||||||||||||||||||||
| self.epochs = epochs | ||||||||||||||||||||
| self.source_shards_by_worker = ( | ||||||||||||||||||||
| _source_shards_by_worker(data) if source_shards_by_worker is None else source_shards_by_worker | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Comment on lines
+154
to
+156
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This would be enough to detect that |
||||||||||||||||||||
| self._idx = 0 | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def randomized_pop(self, buffer): | ||||||||||||||||||||
|
|
@@ -122,14 +174,24 @@ def generate_item(self): | |||||||||||||||||||
| yield self.randomized_pop(buffer) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def __iter__(self): | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| Randomly pop buffered items from `self.data`. | ||||||||||||||||||||
| Multiple dataloader workers sharing this dataset will generate identical item sequences. | ||||||||||||||||||||
| """Randomly pop buffered items from ``self.data``. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Yields: | ||||||||||||||||||||
| Items from the shuffled source after applying the optional transform. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Raises: | ||||||||||||||||||||
| RuntimeError: When the optional transform raises an exception. | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| self.seed += 1 | ||||||||||||||||||||
| super().set_random_state(seed=self.seed) # make all workers in sync | ||||||||||||||||||||
| for _ in range(self.epochs) if self.epochs >= 0 else iter(int, 1): | ||||||||||||||||||||
| yield from IterableDataset(self.generate_item(), transform=self.transform) | ||||||||||||||||||||
| if self.source_shards_by_worker: | ||||||||||||||||||||
| for item in self.generate_item(): | ||||||||||||||||||||
| if self.transform is not None: | ||||||||||||||||||||
| item = apply_transform(self.transform, item) | ||||||||||||||||||||
| yield item | ||||||||||||||||||||
| else: | ||||||||||||||||||||
| yield from IterableDataset(self.generate_item(), transform=self.transform) | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
Comment on lines
+188
to
195
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Instead of this you could add a new argument to |
||||||||||||||||||||
| def randomize(self, size: int) -> None: | ||||||||||||||||||||
| self._idx = self.R.randint(size) | ||||||||||||||||||||
|
|
@@ -197,6 +259,8 @@ class CSVIterableDataset(IterableDataset): | |||||||||||||||||||
|
|
||||||||||||||||||||
| """ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| _shards_by_worker = True | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||||||||||||||||
| def __init__( | ||||||||||||||||||||
| self, | ||||||||||||||||||||
| src: str | Sequence[str] | Iterable | Sequence[Iterable], | ||||||||||||||||||||
|
|
@@ -278,4 +342,5 @@ def __iter__(self): | |||||||||||||||||||
| data=self._flattened(), transform=self.transform, buffer_size=self.buffer_size, seed=self.seed | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| yield from buffer | ||||||||||||||||||||
| yield from IterableDataset(data=self._flattened(), transform=self.transform) | ||||||||||||||||||||
| else: | ||||||||||||||||||||
| yield from IterableDataset(data=self._flattened(), transform=self.transform) | ||||||||||||||||||||
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 think this introspection approach to determining this property isn't the correct way forward for MONAI types. What we use with transforms is trait types such as
RandomizableTraitwhich are empty but can be used withisinstanceto detect if an object is meant to have a property. I would suggest defining aShardsByWorkerTraitclass whichGridPatchDatasetand others would inherit from as the second parent class, then instead of this function just useisinstance.