enhance download_and_extract #8216
Conversation
|
Hi @Jerome-Hsieh If you're having issues with putting this PR together we can discuss how to resolve them, but please do not close and then open a new PR. Thanks for the contribution! |
monai/apps/utils.py
Outdated
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| filename = filepath or Path(tmp_dir, _basename(url)).resolve() | ||
| if filepath: |
There was a problem hiding this comment.
Didn't get the idea for this change here. Seems equivalent to me.
There was a problem hiding this comment.
The change I want to make sure if user don't set the filepath, the process can still work by using the default path
monai/apps/utils.py
Outdated
| if filepath: | ||
| FilepathExtenstion = ''.join(Path(".", _basename(filepath)).resolve().suffixes) | ||
| if urlFilenameExtension != FilepathExtenstion: | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
In my opinion, the issue here is that if filepath can directly get the name of the downloaded file, the download_and_extract function would work as expected, rather than only raising an error at this point.
There was a problem hiding this comment.
I see, but my perspective is that an error with the filepath occurs when the program tries to write the downloaded file to an invalid filepath.
E.g.,filepath='./test' is invalid, filepath='./test.tar.gz' is valid
So I would like to validate the filepath at the very beginning to ensure it is valid.
There was a problem hiding this comment.
I would warn instead of forcing extensions, it takes control away from people to force things like this that aren't strictly necessary.
There was a problem hiding this comment.
But I have a question.
Is it necessary for the user to specify the file extension when setting the filepath for the downloaded file?
There was a problem hiding this comment.
If the user gives a path without extension then you'll get a file with no extension, the user would have to account for that and provide an extension. You could add an extension if none is given (and warn that this happened), but if the user has given a different extension than what's expected, this should be a warning and not an error. If you add an extension you should also be careful not to overwrite an existing file. I realise this is getting rather complicated now, sorry!
Signed-off-by: jerome_Hsieh <[email protected]>
Signed-off-by: jerome_Hsieh <[email protected]>
Signed-off-by: jerome_Hsieh <[email protected]>
Jerome-Hsieh
force-pushed
the
issue5463
branch
from
0511db5 to
0441871
Compare
Signed-off-by: jerome_Hsieh <[email protected]>
|
@ericspod @KumoLiu |
Hi @Jerome-Hsieh, thank you for the quick update. I tested your latest changes, but neither "." nor "./test" as the file path seems to work. Is this the expected behavior? In my opinion, both should work. The logic could be adjusted so that if the file path is a directory and not empty, it automatically captures the name and downloads the file into the specified directory. What do you think? |
|
Hi @KumoLiu my test results： >>> url = "https://github.com/Project-MONAI/MONAI-extra-test-data/releases/download/0.8.1/MedNIST.tar.gz"
>>> monai.apps.utils.download_and_extract(url, filepath=".")
2024-12-06 18:06:56,333 - INFO - Expected md5 is None, skip md5 check for file ..
2024-12-06 18:06:56,334 - INFO - File exists: ., skipped downloading.
2024-12-06 18:06:56,334 - INFO - Non-empty folder exists in ., skipped extracting.and >>> monai.apps.utils.download_and_extract(url, filepath="./test")
2024-12-06 18:08:41,246 - WARNING - filepath=./test, which missing file extension. Auto-appending extension to: ./test.tar.gz
test.tar.gz: 59.0MB [00:10, 6.07MB/s]
2024-12-06 18:08:51,443 - INFO - Downloaded: test.tar.gz
2024-12-06 18:08:51,443 - INFO - Expected md5 is None, skip md5 check for file test.tar.gz.
2024-12-06 18:08:51,443 - INFO - Writing into directory: ..If the results like above, that's expected behavior. Your opinion sounds like |
|
Regarding file downloading from URL, sometimes we can infer the name from the content deposition. Here is one example snippet: https://github.com/Project-MONAI/VLM/blob/7be688aa457a1806f908eb758f2f3ee816fea017/m3/demo/experts/utils.py#L135 @KumoLiu |
|
Thank @mingxin-zheng provide information to find file name. |
Fixes #5463
Description
According to issue, the error messages are not very intuitive.
I think maybe we can check if the file name matches the downloaded file’s base name before starting the download.
If it doesn’t match, it will notify user.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.