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 redirection in download tests #3568

Merged
merged 3 commits into from
Mar 17, 2021
Merged

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Mar 15, 2021

Before we never updated the initial request to incorporate the redirection

for _ in range(max_redirects + 1):
response = fn(request, *args, **kwargs)
if response.url == url or response.url is None:
if url != initial_url:
warnings.warn(f"The URL {initial_url} ultimately redirects to {url}.")
return response
url = response.url
else:

Furthermore, after #3560 has landed, we can now use datasets.utils._get_redirect_url instead of duplicating the functionality without requests.

@codecov
Copy link

codecov bot commented Mar 15, 2021

Codecov Report

Merging #3568 (78e3639) into master (8ee6339) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3568   +/-   ##
=======================================
  Coverage   79.07%   79.07%           
=======================================
  Files         105      105           
  Lines        9787     9787           
  Branches     1572     1572           
=======================================
  Hits         7739     7739           
  Misses       1567     1567           
  Partials      481      481           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ee6339...78e3639. Read the comment docs.

@pmeier pmeier requested a review from fmassa March 15, 2021 17:17
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!

@@ -150,7 +162,7 @@ def assert_server_response_ok():


def assert_url_is_accessible(url, timeout=5.0):
request = Request(url, headers={"method": "HEAD", "User-Agent": USER_AGENT})
request = Request(url, headers={"User-Agent": USER_AGENT}, method="HEAD")
Copy link
Member

Choose a reason for hiding this comment

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

for my understanding: is this a cosmetic change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't mean to include this. It was just for testing. But it shouldn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, if you want to revert this change in a follow-up PR I'm ok with it

@fmassa fmassa merged commit 00c460a into pytorch:master Mar 17, 2021
@pmeier pmeier deleted the fix-download-tests branch March 17, 2021 14:31
facebook-github-bot pushed a commit that referenced this pull request Mar 19, 2021
Summary: Co-authored-by: Francisco Massa <fvsmassa@gmail.com>

Reviewed By: fmassa

Differential Revision: D27128005

fbshipit-source-id: fdafdf85a8286a2bbd7e379229cccfdd84b47513
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.

4 participants