-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Conversation
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
This is also handled, in a different way, here: #1940 |
#1940 seems to be less intrusive, so closing this one. |
Re-opening this as I'm not sure we can be hosting our own version of the dataset |
Not sure, if failing tests are real, but please ping me if you think so and need a fix. |
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! |
Should fix #1938, but would need verification from other machines, locations etc.
Works at least locally for me now.
CC @fmassa