improve robustness of cached_path when extracting archives #4622
Conversation
epwalsh
changed the title
|
|
||
| # We extract first to a temporary directory in case something goes wrong | ||
| # during the extraction process so we don't end up with a corrupted cache. | ||
| tmp_extraction_dir = tempfile.mkdtemp(dir=os.path.split(extraction_path)[0]) |
There was a problem hiding this comment.
This is fine, but why didn't you use with tempfile.TemporaryDirectory(...) as d:?
There was a problem hiding this comment.
When used as a context manager, it will try to remove the temp directory when we exit the context (there is no option to disable that). But at that point - if the extraction was successful - we've already renamed the temp directory to it's final location, so the context manager will raise an exception when it tries to remove the temp directory that no longer exists.
allennlp/common/file_utils.py
Outdated
| os.makedirs(extraction_path) | ||
| os.replace(tmp_extraction_dir, extraction_path) |
There was a problem hiding this comment.
One more comment: Doesn't the first line create all the directories, including the target one? If we fail between these two lines, I think it'll conclude that the archive has been extracted and won't do it again?
In summary, I think that first one has to be os.makedirs(os.path.split(extraction_path)[0]).
There was a problem hiding this comment.
Just fixed. After taking another look, I realized we can just remove that first line altogether because at that point the base directory should already exist.
Closes #4501.