Skip to content

add prototype implementation for MNIST and CIFAR#4414

Merged
pmeier merged 4 commits intopytorch:prototypefrom
pmeier:prototype-datasets
Sep 16, 2021
Merged

add prototype implementation for MNIST and CIFAR#4414
pmeier merged 4 commits intopytorch:prototypefrom
pmeier:prototype-datasets

Conversation

@pmeier
Copy link
Contributor

@pmeier pmeier commented Sep 15, 2021

No description provided.

@datumbox
Copy link
Contributor

I know it's extremely early to comment but I'm very interested in the prototype namespace for unrelated work.

So couple of questions:

  1. Have we finalized the name of this namespace? Many have been proposed in the past, is it aligned across domains?
  2. I assume that prototype will be python only and no C++ features are going to be included, correct?
  3. Will this PR land infrastructure that will exclude the prototype namespace from being included in releases? I assume prototype features will be included in nightlies, right?

@pmeier
Copy link
Contributor Author

pmeier commented Sep 15, 2021

  1. Have we finalized the name of this namespace? Many have been proposed in the past, is it aligned across domains?

I think so. torchaudio also uses it: https://github.com/pytorch/audio/tree/main/torchaudio/prototype cc @fmassa

  1. I assume that prototype will be python only and no C++ features are going to be included, correct?

For me, yes. I'm going for the same tree as the default torchvision, e.g.

torchvision/
  datasets/
  ...
  prototype/
    datasets/
    transforms/
    ...
  1. Will this PR land infrastructure that will exclude the prototype namespace from being included in releases? I assume prototype features will be included in nightlies, right?

For now this PR will not be merged into main, but rather into the prototype branch. The reason is that I want to port stuff from my prototype repo and discuss this with everyone. Afterwards this can be merged into main and only then will your question become relevant. IIRC, @fmassa wants to have the datasets prototypes in our next release, so I'm guessing we will include this namespace into the releases.

@pmeier pmeier merged commit d94da14 into pytorch:prototype Sep 16, 2021
@pmeier pmeier deleted the prototype-datasets branch September 16, 2021 06:26
@fmassa
Copy link
Member

fmassa commented Sep 16, 2021

Given that this is landing in the prototype folder, I would say just merge it right away into the main branch.

Also, one thing which is currently missing in the current version is the ability to query the number of classes (or class names) from the dataset builder. This is something we need to get in our new design in a consistent way

@mthrok
Copy link
Contributor

mthrok commented Sep 16, 2021

Have we finalized the name of this namespace? Many have been proposed in the past, is it aligned across domains?

This looks consistent with the way torchaudio is doing. (note that Torchtext's experimental directory predates the convention of prototype, so that is an exception.)

AFAIK only the top-level name prototype is agreed convention, so how to organize the content of prototype module is up to vision team. You should feel free to do something different than audio does.

One reminder is to make sure that it's user's responsibility to explicitly import prototype module. meaning, do not import prototype submodule from top-level __init__.py. (This PR looks good in that sense)

I assume that prototype will be python only and no C++ features are going to be included, correct?

Speaking this for general cases, it depends. Torchaudio had C++ code in prototype, but the code itself lived in torchaudio/csrc for convenience and it was strange.

Will this PR land infrastructure that will exclude the prototype namespace from being included in releases? I assume prototype features will be included in nightlies, right?

It will be ideal to have a permanent mechanism in setup.py to exclude prototype submodule. (which I imagine something like if build is happening in git tag, then define some environment var, which is picked by setup.py and exclude prototype from packaging)

I once thought of working on it, but I did not have time, and meanwhile prototype features in torchaudio changed a lot so I did not do. It was not too bad to manually remove them in release branch.

@mthrok
Copy link
Contributor

mthrok commented Sep 16, 2021

Given that this is landing in the prototype folder, I would say just merge it right away into the main branch.

Yeah, prototype branch is not the convention we use, and it looks like a rigid branch, which might be confusing for some people. Once the technical discussion is done, I recommend to merge it to main and remove the prototype branch (unless you folks use it for some purpose I am not aware.)

@mthrok
Copy link
Contributor

mthrok commented Sep 16, 2021

Also one recommendation is to have the documentation ready for prototype feature. That really eases the communication.

@pmeier
Copy link
Contributor Author

pmeier commented Sep 16, 2021

The intention to not have this in main right away is to reduce the CI load. While porting stuff from the prototype repo, which will be done by only me, there is no need to run the whole CI suite on every PR. After this is done, I'm going to send a PR to merge prototype into main and afterwards everyone can work on the prototype features. I'm guessing this will happening either tomorrow or early next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants