Separate extraction and decompression logic in datasets.utils.extract_archive #3443
Conversation
This file contains 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
This reverts commit 7fafebb.
pmeier
force-pushed
the
generalize-decompression
branch
from
e0ed436
to
a22abbc
Compare
|
Blocked by #3542 |
fmassa
approved these changes
Mar 12, 2021
Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 19, 2021
…s.extract_archive (#3443) Summary: * generalize extract_archive * [test] re-enable extraction tests on windows * add tests for detect_file_type * add error messages to detect_file_type * Revert "[test] re-enable extraction tests on windows" This reverts commit 7fafebb. * add utility functions for better mock call checking * add tests for decompress * simplify logic by using pathlib * lint * Apply suggestions from code review * make decompress private * remove unnecessary checks * add error message * fix mocking * add remaining tests * lint Reviewed By: fmassa Differential Revision: D27128004 fbshipit-source-id: 73f7d8a43eca5dbc9c7e63d8b1ff6e0859915d92 Co-authored-by: Francisco Massa <fvsmassa@gmail.com> Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
Comment on lines
+293
to
+296
| elif len(suffixes) > 2: | ||
| raise RuntimeError( | ||
| "Archive type and compression detection only works for 1 or 2 suffixes. " f"Got {len(suffixes)} instead." | ||
| ) |
There was a problem hiding this comment.
This introduces a bug for downloads with a period in the file name. For example, https://landcover.ai/download/landcover.ai.v1.zip.
There was a problem hiding this comment.
Working on a PR to fix this, will ping you when it's done.
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.
Currently the implementation of
datasets.utils.extract_archivehas two downsides:It is not extendible. If a new archive type or compression needs to be added (see Remove caching from MNIST and variants #3420 (comment)) one needs to write a new
_is_whatevermethod and implement the processing in the largeif/elifstatement.This can get confusing quite fast especially since the archive type and compression are orthogonal concepts. This leads to structures like
vision/torchvision/datasets/utils.py
Lines 250 to 251 in 67b2528
Imagine we had another archive type that is also compressed with
gzip.It mixes the extraction and decompression concept. This leads to constructs like this
vision/torchvision/datasets/utils.py
Lines 271 to 274 in 67b2528
Note that
to_pathwould need to be fixed for every case where we only decompress the file.This PR overcomes these by separating the extraction and decompression logic:
_extract_whateverfunction and add it to_ARCHIVE_EXTRACTORSwith the corresponding extension.open_whateverfunction and add it to_COMPRESSED_FILE_OPENERSwith the corresponding extension.This PR is fully BC, but makes it a lot easier to move forward in the future.