Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix (Fashion|K)MNIST download and MNIST download test #3557

Merged
merged 2 commits into from
Mar 12, 2021

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Mar 12, 2021

Fixes #3556.

Comment on lines -252 to +253
return collect_download_configs(lambda: datasets.MNIST(ROOT, download=True), name="MNIST")
with unittest.mock.patch.object(datasets.MNIST, "mirrors", datasets.MNIST.mirrors[-1:]):
return collect_download_configs(lambda: datasets.MNIST(ROOT, download=True), name="MNIST")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a hacky fix, but the test infrastructure is currently not able to handle mirrors. During test collection, our tests mock out download_url and record all calls to for each dataset. Afterwards we do head requests against the logged URLs to check if they are accessible. Since we can't know if a request returns an error during collection, the mirror URLs are never collected.

Maybe we should add a mirror functionality to download_url that handles this internally. If we had this, we could add a functionality like this to our tests that warns in case a mirror is down.

Comment on lines +209 to +211
mirrors = [
"http://fashion-mnist.s3-website.eu-central-1.amazonaws.com/"
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Fashion|K)MNIST use the same download logic as MNIST. Thus, we need to also provide the URLs in the same fashion (no pun intended 😉) although we only have a single mirror.

@pmeier
Copy link
Collaborator Author

pmeier commented Mar 12, 2021

Only EMNIST now fails during download tests, which is a recurring failure first reported in #3515.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Let's leave the discussion for mirrors in download_url for the future.

@fmassa fmassa merged commit 9a08e47 into pytorch:master Mar 12, 2021
@pmeier pmeier deleted the fix-mnist-download branch March 12, 2021 09:57
pmeier added a commit to pmeier/vision that referenced this pull request Mar 12, 2021
* add mirrors to (Fashion|K)MNIST

* fix download tests for MNIST
fmassa added a commit that referenced this pull request Mar 12, 2021
* datasets: Fallback to our own mirrors for mnist (#3544)

We are experiencing 403s when trying to download from the main mnist
site so lets fallback to our own mirror on failure.

Signed-off-by: Eli Uriegas <eliuriegas@fb.com>

Co-authored-by: Francisco Massa <fvsmassa@gmail.com>

* Fix (Fashion|K)MNIST download and MNIST download test (#3557)

* add mirrors to (Fashion|K)MNIST

* fix download tests for MNIST

Co-authored-by: Eli Uriegas <1700823+seemethere@users.noreply.github.com>
Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
facebook-github-bot pushed a commit that referenced this pull request Mar 18, 2021
Summary:
* add mirrors to (Fashion|K)MNIST

* fix download tests for MNIST

Reviewed By: fmassa

Differential Revision: D27128007

fbshipit-source-id: 2ca4bba7d8e823cdca3174f408f14e9e6e2e8346
@datumbox datumbox added bug and removed fix labels Apr 27, 2021
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.

Scheduled workflow failed
5 participants