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

datasets: Fallback to our own mirrors for mnist #3544

Merged
merged 2 commits into from
Mar 11, 2021

Conversation

seemethere
Copy link
Member

@seemethere seemethere commented Mar 10, 2021

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

Re-do of #1940

We have now gotten permission to mirror the MNIST dataset

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

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>
Copy link
Contributor

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

One minor nitpick below. Still, could you give an example where you are experiencing an 403 error? Our download test pass for MNIST (EMNIST fails but with an unrelated issue) and running

from torchvision import datasets
datasets.MNIST(".", download=True)

also works for me locally.

Comment on lines +165 to +166
finally:
print()
Copy link
Contributor

Choose a reason for hiding this comment

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

These seem like debug statements, no?

Suggested change
finally:
print()

Copy link
Member

Choose a reason for hiding this comment

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

I think this might be here to avoid the download bar stopping at the middle of the screen for some reason. I'd be happy to remove it in a follow-up commit if this is unnecessary, but I'll be moving forward with merging this as this is blocking a few other things

@Zethson
Copy link

Zethson commented Mar 11, 2021

@pmeier
Copy link
Contributor

pmeier commented Mar 11, 2021

@Zethson The requests fail with an 503 instead of 403 error. Meaning the server maybe just undergoes maintenance, but the access is not generally forbidden. @seemethere do you have insights?

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 @seemethere !

Let's get this merged now to unblock, and maybe revisit @pmeier comments in a follow-up PR

Comment on lines +165 to +166
finally:
print()
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be here to avoid the download bar stopping at the middle of the screen for some reason. I'd be happy to remove it in a follow-up commit if this is unnecessary, but I'll be moving forward with merging this as this is blocking a few other things

@pmeier
Copy link
Contributor

pmeier commented Mar 12, 2021

@seemethere @fmassa

This regressed the download of (K|Fashion)MNIST variants. They rely on the download functionality of the original MNIST. Given that hosting MNIST ourselves is not a temporary solution, couldn't we simply remove the original download links? Otherwise we need to refactor this module.

@fmassa
Copy link
Member

fmassa commented Mar 12, 2021

@pmeier let's fix the MNIST variants.

We got permission to mirror if the download to the original server fails, so we shouldn't just remove the original links

pmeier pushed a commit to pmeier/vision that referenced this pull request Mar 12, 2021
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>
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:
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>

Reviewed By: fmassa

Differential Revision: D27127998

fbshipit-source-id: 552a022845eae39e9a7cc3255bf8b6f8eb2c07e7

Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
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