Add MNIST dataset & drop torchvision dep. from tests #986

awaelchli · 2020-02-29T17:40:27Z

Before submitting

Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
Did you read the contributor guideline?
Did you make sure to update the docs?
Did you write any new necessary tests?
If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #942 and follow up to #990.

Added a custom MNIST class that does not rely on torchvision.
Added download links to MNIST on S3
Removed torchvision dependency from tests
PL examples still use the real torchvision

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

pep8speaks · 2020-02-29T17:40:32Z

Hello @awaelchli! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-03-27 22:46:44 UTC

awaelchli · 2020-02-29T19:31:08Z

why is it trying to run the imagenet example? It's not part of the tests, or is it?

Borda

just curious, do we need all these functions/methods, I would rather use the minimal version so we do not maintain it in future...
Personally I would make simple MNIST datasets and include the to_tensor in it (__getitem__) so we do not need any transformers in tests (anyway we do just a few fist epochs)
Also, we can still use torchvision in examples just adjust tests (split tests and examples, I will do :])

awaelchli · 2020-02-29T21:49:05Z

just curious, do we need all these functions/methods, I would rather use the minimal version so we do not maintain it in future...

ok, I will try to compress everything. Most of the code is for downloading and extracting the dataset files.

I didn't want to remove any tests, that's why I tried to override the imports. Maybe you go ahead and do the splitting of examples and tests first, then I will adjust my changes to that.

Borda · 2020-02-29T22:04:06Z

ok, I will try to compress everything. Most of the code is for downloading and extracting the dataset files.

Well I didn't check it in detail...

I didn't want to remove any tests, that's why I tried to override the imports. Maybe you go ahead and do the splitting of examples and tests first, then I will adjust my changes to that.

You are right, we shall not change any test, just swap dataset source lok

PS: sorry for miss-click (accidentally closing, working on phone)

awaelchli · 2020-02-29T23:46:00Z

@Borda I made it a lot simpler. It is not necessary to do all the data extraction that torchvision does. The MNIST class is now very simple as you wished. And I got rid of the transforms, so we can even drop the PIL requirement now. No PIL to Tensor conversions needed.

Now we just need to decouple the tests from examples :)

Borda · 2020-03-01T21:05:03Z

@awaelchli I have prepared the split, check #990

awaelchli · 2020-03-01T21:54:44Z

@Borda Great! I think yours should be merged first, because I can't make the tests pass otherwise.

Borda

GREAT job! Get this done as soon as possible...

.gitignore

tests/datasets/mnist.py

CHANGELOG.md

tests/base/datasets.py

Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com>

awaelchli · 2020-03-26T00:42:25Z

The test failed because of this?

Expected nothing
Got:
    TRAINS new version available: upgrade to v0.14.1 is recommended!

awaelchli · 2020-03-26T00:44:45Z

@bmartinn is there a way we can suppress such messages during test?

Borda · 2020-03-26T06:47:16Z

@bmartinn is there a way we can suppress such messages during test?

We know about it... allegroai/clearml#119

bmartinn · 2020-03-26T16:49:10Z

Hi @awaelchli , as @Borda mentioned the issue is known, and a fix was already merged into the master branch.
If CI tests are failing, the easiest temporarily fix (just for this PR) is to upgrade the trains package in the requirements-extra.txt to the latest stable release.
There is already a pending PR on that as well ...

awaelchli · 2020-03-27T23:19:02Z

@Borda I see KeyBoardInterrupt in CI. Did you have that before?

Borda · 2020-03-27T23:24:19Z

I see KeyBoardInterrupt in CI. Did you have that before?

not really...

Borda · 2020-03-30T16:58:27Z

@williamFalcon @PyTorchLightning/core-contributors lets get this done...

* added custom mnist without torchvision dep * move files so it does not conflict with mnist gitignore * mock torchvision for tests * fix line too long * fix line too long * fix "module level import not at top of file" warning * move mock imports to __init__.py * simplify MNIST a lot and download directly the .pt files * further simplify and clean up mnist * revert import overrides * make as before * drop PIL requirement * move mnist.py to datasets subfolder * use logging instead of print * choose same name as in torchvision * remove torchvision and pillow also from yml file * refactor if train Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com> * capitalized class attr * moved mnist to models * re-added datsets ignore * better name for file variable * Update mnist.py * move dataset classes to datasets.py * new line * update * update * fix automerge * move to base folder * adapt testingmnist to new mnist base class * remove temporal fix * fix datatype * remove old testingmnist * readable * fix import * fix whitespace * docstring Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com> * Update tests/base/datasets.py Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com> * changelog * added types * Update CHANGELOG.md Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com> * exist->isfile Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com> * index -> idx * temporary fix for trains error * better changelog message Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

awaelchli changed the title ~~Drop torchvision dep. from tests~~ Drop torchvision dep. from tests [WIP] Feb 29, 2020

Borda reviewed Feb 29, 2020

View reviewed changes

Borda closed this Feb 29, 2020

Borda reopened this Feb 29, 2020

Borda changed the title ~~Drop torchvision dep. from tests [WIP]~~ Add MNIST dataset & drop torchvision dep. from tests [WIP] Feb 29, 2020

Borda added ci Continuous Integration feature Is an improvement or enhancement labels Feb 29, 2020

This was referenced Mar 1, 2020

CI: split tests-examples #990

Merged

add python 3.8 testing #915

Merged

awaelchli changed the title ~~Add MNIST dataset & drop torchvision dep. from tests [WIP]~~ [blocked by #990] Add MNIST dataset & drop torchvision dep. from tests Mar 3, 2020

awaelchli mentioned this pull request Mar 3, 2020

hparams as dict #1029

Merged

Borda approved these changes Mar 4, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

tests/datasets/mnist.py Outdated Show resolved Hide resolved

tests/datasets/mnist.py Outdated Show resolved Hide resolved

Adrian Wälchli and others added 11 commits March 4, 2020 13:42

added custom mnist without torchvision dep

d9a6c63

move files so it does not conflict with mnist gitignore

72ea8a4

mock torchvision for tests

6dedc60

fix line too long

2a5d0f8

fix line too long

5ef53a7

fix "module level import not at top of file" warning

6cbc638

move mock imports to __init__.py

2dbbfcf

simplify MNIST a lot and download directly the .pt files

59000f0

further simplify and clean up mnist

bffacaf

revert import overrides

5cf4c27

make as before

423d76c

added types

be425e3

Borda added the ready PRs ready to be merged label Mar 25, 2020

Borda reviewed Mar 25, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Borda reviewed Mar 26, 2020

View reviewed changes

tests/base/datasets.py Outdated Show resolved Hide resolved

Borda reviewed Mar 26, 2020

View reviewed changes

tests/base/datasets.py Outdated Show resolved Hide resolved

Adrian Wälchli and others added 3 commits March 26, 2020 01:03

Update CHANGELOG.md

0ae8ac4

Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com>

exist->isfile

3204684

Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com>

index -> idx

ec6e6b2

Adrian Wälchli added 5 commits March 26, 2020 18:07

Merge branch 'master' into mnistcopy

5d1b2c6

temporary fix for trains error

85c9a4a

Merge remote-tracking branch 'PyTorchLightning/master' into mnistcopy

b360d1a

Merge branch 'master' into mnistcopy

dfa5f6e

better changelog message

aca2cb9

Borda requested review from jeffling, jeremyjordan, justusschock, tullie and a team March 30, 2020 16:57

williamFalcon merged commit b7de42f into Lightning-AI:master Mar 30, 2020

awaelchli deleted the mnistcopy branch March 30, 2020 22:43

Borda modified the milestones: v0.7., v0.7.x Apr 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MNIST dataset & drop torchvision dep. from tests #986

Add MNIST dataset & drop torchvision dep. from tests #986

awaelchli commented Feb 29, 2020 •

edited

Loading

pep8speaks commented Feb 29, 2020 •

edited

Loading

awaelchli commented Feb 29, 2020

Borda left a comment

awaelchli commented Feb 29, 2020

Borda commented Feb 29, 2020 •

edited

Loading

awaelchli commented Feb 29, 2020

Borda commented Mar 1, 2020

awaelchli commented Mar 1, 2020

Borda left a comment

awaelchli commented Mar 26, 2020 •

edited

Loading

awaelchli commented Mar 26, 2020

Borda commented Mar 26, 2020

bmartinn commented Mar 26, 2020

awaelchli commented Mar 27, 2020

Borda commented Mar 27, 2020

Borda commented Mar 30, 2020

Add MNIST dataset & drop torchvision dep. from tests #986

Add MNIST dataset & drop torchvision dep. from tests #986

Conversation

awaelchli commented Feb 29, 2020 • edited Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

pep8speaks commented Feb 29, 2020 • edited Loading

Comment last updated at 2020-03-27 22:46:44 UTC

awaelchli commented Feb 29, 2020

Borda left a comment

Choose a reason for hiding this comment

awaelchli commented Feb 29, 2020

Borda commented Feb 29, 2020 • edited Loading

awaelchli commented Feb 29, 2020

Borda commented Mar 1, 2020

awaelchli commented Mar 1, 2020

Borda left a comment

Choose a reason for hiding this comment

awaelchli commented Mar 26, 2020 • edited Loading

awaelchli commented Mar 26, 2020

Borda commented Mar 26, 2020

bmartinn commented Mar 26, 2020

awaelchli commented Mar 27, 2020

Borda commented Mar 27, 2020

Borda commented Mar 30, 2020

awaelchli commented Feb 29, 2020 •

edited

Loading

pep8speaks commented Feb 29, 2020 •

edited

Loading

Borda commented Feb 29, 2020 •

edited

Loading

awaelchli commented Mar 26, 2020 •

edited

Loading