resolve redirection for HTTP resources #5151
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
💊 CI failures summary and remediationsAs of commit d573acc (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
| Job | Step | Action |
|---|---|---|
 cmake_macos_cpu |
curl -o conda.sh https://repo.anaconda.com/miniconda/Miniconda3-latest-MacOSX-x86_64.sh | |
| sh conda.sh -b | ||
| source $HOME/miniconda3/bin/activate | ||
| conda install -yq conda-build cmake | ||
| packaging/build_cmake.sh | ||
| 🔁 rerun |
This comment was automatically generated by Dr. CI (expand for details).
Please report bugs/suggestions to the (internal) Dr. CI Users group.
NicolasHug
approved these changes
Jan 14, 2022
There was a problem hiding this comment.
Thanks @pmeier ,
I mostly have questions but I'll approve to unlock. For my own understanding, did we face cases in the past where not handling redirections was a problem?
Comment on lines
+168
to
+170
| gdrive_id = _get_google_drive_file_id(redirect_url) | ||
| if gdrive_id: | ||
| return GDriveResource(gdrive_id, **meta) |
There was a problem hiding this comment.
Do we have current cases where an HTTP resource redirects to a Google drive?
There was a problem hiding this comment.
Yes. It is one of the datasets not yet ported to prototype. I've also hit this while doing #5154.
|
|
||
| def _download(self, root: pathlib.Path) -> None: | ||
| if not self.resolved: | ||
| return cast(OnlineResource, self.resolve())._download(root) | ||
|
|
||
| for url in itertools.chain((self.url,), self.mirrors or ()): | ||
| try: | ||
| download_url(url, str(root), filename=self.file_name, md5=None) |
There was a problem hiding this comment.
In your first comment you wrote
This is far from perfect, since the mirrors do not get resolved
Could we instead do the redirection here and handle the mirrors?
There was a problem hiding this comment.
We could, but that would make stuff more complicated. Given that the plan is to refactor this anyway with iopath and we currently only have a single datasets that has official mirrors, I wouldn't sweat it at the moment.
facebook-github-bot
pushed a commit
that referenced
this pull request
Feb 3, 2022
Summary: * resolve redirection for HTTP resources * appease mypy * address review Reviewed By: kazhang Differential Revision: D33927505 fbshipit-source-id: a6b39b2809fd63419620523f6b84a60e91147818
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is far from perfect, since the
mirrorsdo not get resolved. So far we never hit an issue where this would be a problem, but still.In the future, the plan is to switch the resources to
iopath. If that is available, resolving will no longer generate a new resource, but rather change the underlying path in place.cc @pmeier @bjuncek