Fixes issue #1868 regarding TypeError: ZipperIterDataPipe #1954
Conversation
…ance doesn't have a valid length
|
Hi @otioss! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
✅ Deploy Preview for pytorch-tutorials-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
| @@ -303,7 +303,7 @@ def train_epoch(model, optimizer): | |||
| optimizer.step() | |||
| losses += loss.item() | |||
|
|
|||
| return losses / len(train_dataloader) | |||
| return losses / len(list(train_dataloader)) | |||
There was a problem hiding this comment.
It might be better to fix the __len__ of ZipperIterDataPipe in torchdata: https://github.com/pytorch/pytorch/blob/ea72a0991c3422d8f314acdf8b911de42a6b4c1e/torch/utils/data/datapipes/iter/combining.py#L554
cc: @ejguan
There was a problem hiding this comment.
It's better to append a LenthSetterDataPipe at the end of each Dataset within TorchText with a known length. For Zipper in torchdata, it's for general usage that requires the prior DataPipe provides valid length.
There was a problem hiding this comment.
@ejguan if my understanding is correct, calling list(train_dataloader) would cause all data in the dataset to materialize right? I thought this is something we'd want to avoid given the fact that this can lead to OOMs for very large datasets.
It's better to append a LenthSetterDataPipe at the end of each Dataset within TorchText with a known length.
If we do this, can we avoid the data materialization issue I mentioned above?
There was a problem hiding this comment.
Correct, we shouldn't do materialization here. If this Dataset has a known length, we can add a DataPipe:
class LenSetter(IterDataPipe):
def __init__(self, datapipe, length):
self.datapipe = datapipe
self.length = length
def __iter__(self):
yield from self.datapipe
def __len__(self):
self.length = lenthThis would prevent any pre-materialization before data iterated. IterDataPipe itself doesn't guarantee to provide a __len__ function because there might DataPipe operations introduce the dynamic length (filter)
There was a problem hiding this comment.
Gotcha, just created an issue to track this pytorch/text#1943. Will merge this PR for now
This fixes issue #1868: