[C++ Frontend] Remove size() from BatchDataset and templatize IndexType #12960

goldsborough · 2018-10-23T00:27:34Z

This PR brings to changes to the recently landed C++ Frontend dataloader:

Removes the size() method from BatchDataset. This makes it cleaner to implement unsized ("infinite stream") datasets. The method was not used much beyond initial configuration.
Makes the index type of a dataset a template parameter of BatchDataset and Sampler. This essentially allows custom index types instead of only vector<size_t>. This greatly improves flexibility.

See the InfiniteStreamDataset and TestIndex datasets in the tests for what this enables.

Some additional minor updates and code movements too.

torch/csrc/api/include/torch/data/dataloader.h

  /// to configure the `DataLoader` with, and a `sampler` that specifies the
  /// sampling strategy.
  DataLoader(Dataset dataset, DataLoaderOptions options, Sampler sampler)
      : options_(std::move(options)),


torch/csrc/api/include/torch/data/samplers/random.h

-    index_ += index_batch.size();
-    return index_batch;
-  }
+  optional<std::vector<size_t>> next(size_t batch_size) override;


apaszke · 2018-10-23T12:41:38Z

I didn't have time to review this yet, but I'm concerned that our C++ dataset API starts diverging from the Python one significantly. We should stop this and bring them together, or we should stop calling this a C++ interface to PyTorch.

goldsborough · 2018-10-23T16:37:27Z

@apaszke that sounds a bit dramatic :) Let me give some more context on these two changes. @ssnl, @soumith and I actually sat together before meeting with some partners to discuss changes to the C++ dataloader (which MSFT wants to use), and how we can reconcile them with the Python dataloader. We want both to have the same features/functionality and @ssnl is working on making the size() optional in Python too.

We want to support the use case of infinite stream datasets cleanly. Sure, you can always have size() be INT_MAX, but (1) this is not a clean API and (2) if you ever accidentally used the (default) RandomSampler, you'd have a bit of a memory problem. By making the concept of unsized datasets more "first class", we can verify, for example, that you don't try to create a RandomSampler from an unsized dataset.
This change actually makes the C++ frontend dataloader more like Python, because you can now have indices other than size_t. In Python this all works through duck typing of course. So in C++ we allow this now too, which yields much greater flexibility.

I hope this clarifies things and let me know if you have more thoughts.

goldsborough · 2018-10-26T16:53:51Z

Requesting review please: @apaszke @ezyang

apaszke

Haven't checked the tests. I think the API for streams feels a bit like a hack and could be improved to make it much nicer. There are also some rough edges around making the dataset size optional.

If we're going to merge this change, I'd really like us to consider how would we integrate those things into our Python API. Those two interfaces already have different semantics, and I don't want them to diverge any further.

torch/csrc/api/include/torch/data/dataloader.h

torch/csrc/api/include/torch/data/datasets/base.h

torch/csrc/api/include/torch/data/samplers/base.h

torch/csrc/api/include/torch/data/samplers/custom_index.h

torch/csrc/api/include/torch/data/samplers/sequential.h

torch/csrc/api/include/torch/data/samplers/stream.h

goldsborough · 2018-10-31T17:35:39Z

@apaszke I've renamed Index to BatchIndex and documented StreamSampler. I added a comment about why StreamSampler was written in the "just give me batch_size values" way. Let me know what you think.

facebook-github-bot

@goldsborough has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

goldsborough · 2018-11-02T16:47:47Z

@apaszke what do you think about the StreamSampler issue? I don't want to have this PR linger too long, there are people at microsoft waiting for it

facebook-github-bot

@goldsborough is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

goldsborough requested a review from ebetica as a code owner October 23, 2018 00:27

goldsborough force-pushed the cpp-data-loader-improved branch from 416e901 to 63f50db Compare October 23, 2018 00:29

ssnl reviewed Oct 23, 2018

View reviewed changes

torch/csrc/api/include/torch/data/samplers/random.h

index_ += index_batch.size();

return index_batch;

}

optional<std::vector<size_t>> next(size_t batch_size) override;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

goldsborough force-pushed the cpp-data-loader-improved branch 2 times, most recently from 20b6ed3 to 9bb319a Compare October 23, 2018 20:15

ssnl mentioned this pull request Oct 23, 2018

Python dataloader Improvements #13023

Open

7 tasks

goldsborough force-pushed the cpp-data-loader-improved branch 3 times, most recently from 8a97ccf to 862f8c6 Compare October 26, 2018 16:53

goldsborough force-pushed the cpp-data-loader-improved branch 2 times, most recently from 7b554bd to bcf8a94 Compare October 29, 2018 20:58

apaszke suggested changes Oct 30, 2018

View reviewed changes

goldsborough force-pushed the cpp-data-loader-improved branch from bcf8a94 to b3b2ae0 Compare October 31, 2018 17:32

facebook-github-bot reviewed Nov 1, 2018

View reviewed changes

apaszke approved these changes Nov 5, 2018

View reviewed changes

goldsborough added 5 commits November 5, 2018 11:00

Remove size() from BatchDataset and templatize IndexType

5aafcd5

Fix CanUseCustomTypeAsIndexType

89f3008

Rename Index -> BatchIndex

0aa46a3

Rename index -> examples_retrieved_so_far in StreamSampler

8caf199

IndexBatch -> BatchRequest

ca8bf7c

goldsborough force-pushed the cpp-data-loader-improved branch from b3b2ae0 to ca8bf7c Compare November 5, 2018 19:36

facebook-github-bot reviewed Nov 5, 2018

View reviewed changes

facebook-github-bot closed this in 8fafa7b Nov 6, 2018

goldsborough mentioned this pull request Nov 6, 2018

Modifications to the data loader for enabling chunk support. #13585

Closed

ezyang added the merged label Jun 25, 2019

[C++ Frontend] Remove size() from BatchDataset and templatize IndexType #12960

[C++ Frontend] Remove size() from BatchDataset and templatize IndexType #12960

Uh oh!

Conversation

goldsborough commented Oct 23, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

apaszke commented Oct 23, 2018

Uh oh!

goldsborough commented Oct 23, 2018

Uh oh!

goldsborough commented Oct 26, 2018

Uh oh!

apaszke left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

goldsborough commented Oct 31, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

goldsborough commented Nov 2, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment