@smessmer, you've been thinking about this recently. Could you please review this patch?

@ezyang I was thinking about this in the context of kernel APIs. I think data readers are orthogonal to that. The changes in this diff are doing what the description claims, but we'd have to discuss if we want these changes.

@thiagocrepaldi is right with the claim that ChunkDataReader uses some std::vector APIs, so even before this PR, it would only have worked with container types that implement .reserve(), .emplace(), ..., but it would have worked with custom types implementing that API. After this PR, custom container types would not work anymore. I don't think I have enough context to make this call.

@smessmer Yes, you are right. However, in any of the use cases we see so far, we have not seen a case for chunk reader to produce non vector types. For example, the "BatchType" type used in the C++ API is a vector of "Tensor"s or "Examples". This PR makes the pybinding the dataloaders much easier in the future as well. Since it is a minimum change, just wondering if we can take this to pytorch before 1.1.

@apaszke @goldsborough Do you see any potential issues with the change?

A gentle reminder...

@apaszke I see your concern, but the whole point of the 'ChunkDataReader' is to read a large chunk. Also this PR makes the pybind of the 'ChunkDataSet' bit more easier. So while, I would like to keep the API flexible, I don't think the concern on a different vector is a valid one to hold this PR.

Ok, how about this. Instead of using a single template parameter for the chunk type we keep two - one for the data type, and another as a template parameter for a container, which defaults to std::vector? That's the approach which is often taken in STL.

Ok, how about this. Instead of using a single template parameter for the chunk type we keep two - one for the data type, and another as a template parameter for a container, which defaults to std::vector? That's the approach which is often taken in STL.

Hi Adam, we have a dual goal for this PR. The first is to fix the API issue that arises when we use a non-vector complaint API as template argument for the ChunkDataReader batch type. At the same type, this PR also targets to ease pybinding ChunkDataReader and ChunkDataset classes.

Your suggestion would fix the first concern beautifully in C++, but would also make Python binding very difficult because Python doesn't support templates and we would have to either 'hide' all templates from Python by either instantiating ChunkDataReader with all combinations of 'vector' / 'batch type' or create a generic batch type class that mimics STL vector API.
The former approach wouldnt be 100% bullet proof and from time to time we may have to pybind new custom classes. The second approach is less natural and more verbose for future implementation.

Although keeping the API generic is usually a good thing, I was wondering if we could keep the implementation as simple as possible until we have an use case that supports the additional complexity

How would it make pybinding any different? I don't see how e.g. exposing a pybind class for ChunkDataReader<int, std::vector> in my proposal (note that the second argument is also optional, so you don't have to specify that) would be different than exposing ChunkDataReader<int> in yours. As long as someone actually doesn't change the container type, there's nothing else they need to pybind.

How would it make pybinding any different? I don't see how e.g. exposing a pybind class for ChunkDataReader<int, std::vector> in my proposal (note that the second argument is also optional, so you don't have to specify that) would be different than exposing ChunkDataReader<int> in yours. As long as someone actually doesn't change the container type, there's nothing else they need to pybind.

Leveraging default arguments for the template during binding generations is a good idea! Although ChunkDataReader<int, std::vector> cant be instantiated because std::vector is incomplete without its arguments. ChunkDataReader<int, std::vector<int>> does the trick. All we would need to do in the current PR is changing ChunkDataReader API to something like:

template <typename DataType, typename ContainerType = std::vector<DataType>>
class ChunkDataReader { ... }

How would it make pybinding any different? I don't see how e.g. exposing a pybind class for ChunkDataReader<int, std::vector> in my proposal (note that the second argument is also optional, so you don't have to specify that) would be different than exposing ChunkDataReader<int> in yours. As long as someone actually doesn't change the container type, there's nothing else they need to pybind.

Leveraging default arguments for the template during binding generations is a good idea! Although ChunkDataReader<int, std::vector> cant be instantiated because std::vector is incomplete without its arguments. ChunkDataReader<int, std::vector<int>> does the trick. All we would need to do in the current PR is changing ChunkDataReader API to something like:

template <typename DataType, typename ContainerType = std::vector<DataType>>
class ChunkDataReader { ... }

I have pushed a new update with the proposed change. What do you think?

A gentle reminder...

@pytorchbot rebase this please

@pytorchbot merge this please

@pytorchbot rebase this please

@soumith merged this pull request in 3d4d7b9.