Legacy interface for prototype datasets?

[pmeier]

The prototype datasets change the interface in two ways:

1. The input parameters are a little different in some cases. For example in the current API the MNIST dataset would be instantiated with datasets.MNIST(..., train=True) whereas now it looks like datasets.load("mnist", split="train").
2. The output is completely different. Before we returned a tuple (sometimes of varying length) whereas now we always return a dictionary. Furthermore, before we used PIL images and numpy arrays as return types, whereas now we always use tensor subclasses.

To lower the burden to move to the new style datasets a little, we could have a legacy: bool = False keyword argument on datasets.load(). For all datasets that have a legacy variant, we could simply implement two functions that map the input and output. If a dataset doesn't support a legacy variant, we could simply error out.

cc @pmeier @bjuncek

[datumbox]

I assume you refer to the torchvision.prototype.datasets.load() method. Is this where you propose to add the legacy parameter?

Can you talk more about how supporting both legacy and non-legacy datasets via the new registration API will help/simplify adoption? It's unclear to me what the user would have to do if we don't implement this proposal versus implementing it. This is not feedback over whether we should do this or not. It's just unclear the motivation and how it fits in the picture. thanks!

[pmeier]

I assume you refer to the torchvision.prototype.datasets.load() method. Is this where you propose to add the legacy parameter?

Correct. For the legacy example datasets comes from from torchvision import datasets, whereas for the prototype example it comes from from torchvision.prototype import datasets.

Can you talk more about how supporting both legacy and non-legacy datasets via the new registration API will help/simplify adoption? It's unclear to me what the user would have to do if we don't implement this proposal versus implementing it.

There is a difference between supporting legacy datasets and supporting the interface of the legacy datasets. The former will not be possible, while the second is the subject of this discussion.

Let's say the current test loop looks somewhat like this:

from torchvision import datasets

dataset = datasets.MNIST(..., train=False)

for input, target in dataset:
    ....

With the prototype datasets the same loop looks like this:

from torchvision.prototype import datasets

dataset = datasets.load("mnist", split="test")

for sample in dataset:
    input, target = sample["image], sample["label"]

There are three differences here:

1. Instead of instantiating a dataset class, the user now has to call datasets.load().
2. Instead of using train=False, the user now has to pass split="test".
3. Instead of directly getting a tuple of the input and target, the user now has to grab them from the sample dictionary.

Point 1. is a deliberate choice and there is no way around that. If the user wants to use the new datasets, they need to change the instantiation.

Point 2. and 3. however are only changes we made to unify the datasets. Meaning, if the user wants the exact same behavior as before, they need to put in some extra work with no benefit.

This issue is about handling this for the user so they can make the change from the legacy to the datasets in small steps. With the POC implementation in #5041, the loop could look like this:

from torchvision.prototype import datasets

dataset = datasets.load("mnist", legacy=True, train=False)

for input, target in dataset:
    ...

The only change here compared to the current implementation is the instantiation from datasets.MNIST(...) to datasets.load("mnist", legacy=True, ...).

[datumbox]

Sounds reasonable. Are there any other hidden differences that we need to highlight here before assessing the proposal? For example how about the return types of input and target. Are they going to be the same?

[pmeier]

Are there any other hidden differences that we need to highlight here before assessing the proposal? For example how about the return types of input and target. Are they going to be the same?

From #5041 (comment)

This will only handle the interface, but not the return types. Meaning with legacy=True we would still return Tuple[torchvision.prototype.features.Image, torchvision.prototype.features.Label] instead of Tuple[PIL.Image.Image, int].

I think this is a reasonable tradeoff. To use the data with on of our models, the data has to be converted to a tensor at some point any way. This usually happened with transform=ToTensor() or the like.

So yes, if the user needs the return types to also be exact, the proposed approach is not sufficient.

[datumbox]

Yeah I think this is a critical detail that needs to be highlighted here. If what we get from the dataset is different, the user still needs to make a bunch of changes on their code to handle this. It's not just the X part of the training data pair; it might be necessary handling the Y as well.

Given the above, I'm not sure if writing extra code to make things look different is worth it, given that the APIs need to be handled differently (due to their return types). Instead, one could argue that we should be putting enough good features in the new API, solving common user issues to make the migration worth it.

I would love to hear what others think on this. @NicolasHug @prabhat00155 @fmassa?

[pmeier]

If what we get from the dataset is different, the user still needs to make a bunch of changes on their code to handle this.

It depends. In most cases everything gets converted into a tensor anyway. From our references:

vision/references/classification/presets.py

Line 58 in 47bd962

transforms.PILToTensor(),

vision/references/classification/presets.py

Line 30 in 47bd962

transforms.PILToTensor(),

vision/references/detection/presets.py

Line 11 in 47bd962

T.PILToTensor(),

vision/references/detection/presets.py

Line 44 in 47bd962

self.transforms = T.ToTensor()

So, unless the user does some stuff on the PIL image before it gets passed to the transform, nothing changes.

[NicolasHug]

Overall I share @datumbox points.

I don't think it's worth the hassle and extra complexity of maintaining 2 sets of API. If anything, I feel like this would actually hurt the migration, because some users would just do half of it instead of doing it all. and if/when we remove support for the legacy, they would have to apply a second set of change.

Instead, one could argue that we should be putting enough good features in the new API, solving common user issues to make the migration worth it

I was going to comment something along these lines before reading it