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

Add header for MNIST download due to cloudflare browser check #1939

Closed
wants to merge 2 commits into from

Conversation

ptrblck
Copy link
Contributor

@ptrblck ptrblck commented Mar 4, 2020

Should fix #1938, but would need verification from other machines, locations etc.
Works at least locally for me now.

CC @fmassa

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.

Nicely done, thanks @ptrblck !

I have a comment, let me know what you think

if header is not None:
opener = urllib.request.build_opener()
opener.addheaders = header
urllib.request.install_opener(opener)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should revert back to the previous opener after the function has finished? Otherwise this globally affects the rest of the code using urllib. Not sure if it's possible to do it with the current API though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense and thanks for pointing this out!
It seems to work, if I create an empty opener and just install it again, as I get the same forbidden error then.

@ptrblck
Copy link
Contributor Author

ptrblck commented Mar 4, 2020

Added the resetting of the header.

@fmassa Please feel free to ignore/close this PR, if we find another (and better) way of disabling the browser check, but this might be seen as a workaround for the next nightly/master builds.

@seemethere
Copy link
Member

This is also handled, in a different way, here: #1940

@ptrblck
Copy link
Contributor Author

ptrblck commented Mar 4, 2020

#1940 seems to be less intrusive, so closing this one.

@ptrblck ptrblck closed this Mar 4, 2020
@fmassa fmassa reopened this Mar 4, 2020
@fmassa
Copy link
Member

fmassa commented Mar 4, 2020

Re-opening this as I'm not sure we can be hosting our own version of the dataset

@ptrblck
Copy link
Contributor Author

ptrblck commented Mar 5, 2020

Not sure, if failing tests are real, but please ping me if you think so and need a fix.

@fmassa
Copy link
Member

fmassa commented Mar 5, 2020

We have fixed this issue in the server hosting the dataset (thanks to @soumith), so there is no need for a change anymore.

Thanks again for the PR!

@fmassa fmassa closed this Mar 5, 2020
@ptrblck ptrblck deleted the url_header branch March 5, 2020 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Downloading MNIST dataset with torchvision gives HTTP Error 403
3 participants