[feature request] Allow passing in image reading/loading function to old-style datasets constructors

[vadimkantorov]

🚀 The feature

This makes more sense now that torchvision is compiled with its own image reading functions (read_image), so easier way to test pipelines without PIL would be nice

In addition, this would allow easy stubbing out image loading when wanted and solving #4975

Motivation, pitch

N/A

Alternatives

No response

Additional context

No response

cc @pmeier

[datumbox]

@vadimkantorov Thanks for the proposal.

It's unlikely that we will add many new features on the old-style datasets but this is certainly good feedback for those in prototype. As far as I understand the new API has a mechanism (decoder) for using different readers, but it seems that now only PIL is supported.

@pmeier Is supporting TorchVision's io within your current plans?

[pmeier]

Yes, the new datasets will use the decoder parameter to do just that. You can pass any callable to datasets.load(..., decoder=) that takes an open file handle and returns a tensor. torchvision.io.read_image currently takes a path, so we are using PIL by default now.

There are still some design choices to be made what a decoder is and what it should return. For example, how do we handle the case if more than one type of file need to be decoded? Furthermore, how do we handle the case of multiple return values for example the audio and video tensors after decoding a video.

[vadimkantorov]

The proposed change is simple, low risk and is immediately useful to hundreds of people working with existing dataset APIs, it will take some time until the new ones are adopted by the community.

[pmeier]

Unfortunately, I don't think the change is simple. The functionality you propose is implemented in the new-style datasets, but we already ran into troubles and I see more to come in the future. I've opened #5075 for a discussion of how we want to handle decoding in the future. While we will find a solution for them for the new-style datasets I'm not eager to also maintain the same functionality in the old-style ones.

From your request I get that you only want to disable it and not use custom decoding. If don't care about the actual data and only use it for testing, can't you simply patch it out?

from unittest import mock

from torchvision import datasets

dataset = datasets.VOCDetection(...)

print(dataset[0][0])

with mock.patch("torchvision.datasets.voc.Image.open"):
    print(dataset[0][0])

<PIL.Image.Image image mode=RGB size=500x442 at 0x7F705C1E8A90>
<MagicMock name='open().convert()' id='140119809177920'>

With the patch, iterating over the complete dataset takes ~1.5 seconds on my machine.

[vadimkantorov]

Yes, I have ways around it of course as mentioned above, but it's cumbersome.

What kind of troubles could it be if one just accepts a lambda that reads an image by path? or at least makes it semi-standard private methods that are elementary to patch?

[pmeier]

As detailed in #5075:

1. The current signature assumes that there is only one type of payload to decode in a dataset, i.e. images or videos. Other types, for example annotation files stored as .mat, .xml, or .flo, will always be decoded. Thus, the user can't completely deactivate the decoding after all. Furthermore, they can also not use any custom decoding for these types if need be.
2. The current signature assumes that all payloads of a single type can be decoded by the same decoder. Counter examples to this are the HD1K optical flow datasets that uses 16bit .png images as annotations which have sub-par support by Pillow.

[vadimkantorov]

About (1), in context of old-style dataset, one can start with accepting image_loader in constructor or using _image_loader function, this already speeds up a lot the scenario of iterating/indexing a dataset without image loading/decoding. If wanted, more loading functions could be moved to private functions, such as _load_xml or _load_flo - this is relatively simple to patch out as well if not wanting to accept them to constructor (although this is also possible just fine)

Aobut (2): old-style dataset may have a suitable default loading function if None is passed or if _image_loader isn't redefined, there's no assumption that the same decoder would be used for all datasets. If it works now, it would work with moving the image loading logic to _load_image function just fine

[pmeier]

this already speeds up a lot the scenario of iterating/indexing a dataset without image loading/decoding.

Our datasets are map-style datasets. Thus, they are fully indexed as soon as their are instantiated. What kind of information do you want to extract that is not available at that stage?

old-style dataset may have a suitable default loading function if None is passed or if _image_loader isn't redefined, there's no assumption that the same decoder would be used for all datasets. If it works now, it would work with moving the image loading logic to _load_image function just fine

My point is that this solution will not work for all datasets as detailed in the other issue. I don't want one-off solutions for a problem that already has a workaround will be solved in a more general way in the future.

I'm ok with a solution like you proposed in #4975, because it doesn't increase the API surface. Still, this new "feature" won't be available immediately. The next release is planned for early March. This is also the time we plan on moving the new-style datasets out of a prototype state.

[vadimkantorov]

I don't want one-off solutions for a problem that already has a workaround will be solved in a more general way in the future.

The vast majority of code and packages and projects in development use old-style datasets. It's your call of course, but adding such small features, even if not "solving the data loading" is useful and helpful for a lot of people.

[pmeier]

What exactly is the use case? As stated above, the datasets are already indexed after they are instantiated. What kind of data do you want to extract?