wfdb-python icon indicating copy to clipboard operation
wfdb-python copied to clipboard

Fix downloading files in windows

Open lbugnon opened this issue 3 years ago • 5 comments

Adding a small change to make optional the use of multiple threads for download files. We couldn't make it work in a conda environment in windows.

With this change, default is the same, but multithread download can be disabled by optional parameter.

lbugnon avatar Oct 01 '21 05:10 lbugnon

Thanks!

As I said before, did you try simply changing "multiprocessing.Pool" to "multiprocessing.dummy.Pool"? I know that multiprocessing is messy and best avoided here, but multiprocessing.dummy should not have the same drawbacks (it has other drawbacks instead.)

If multiprocessing.dummy.Pool doesn't work for you either, that's a bigger problem and something I'd like to try to understand.

bemoody avatar Oct 01 '21 14:10 bemoody

I think we can merge in this change after the requested changes are made. It seems useful in general.

Let's figure out #306 separately.

cx1111 avatar Mar 31 '22 04:03 cx1111

I think we can merge in this change after the requested changes are made. It seems useful in general.

Let's figure out #306 separately.

cx1111 avatar Mar 31 '22 04:03 cx1111

If we do want to make parallel downloads optional, I'd say the name of the argument should be parallel or something like that. If we change the implementation to use threads instead of processes then use_multiprocess becomes misleading.

I'm not opposed to adding that option, I just don't want to add the option solely to avoid a known bug in the package, instead of just fixing the bug.

I also don't understand this change, which seems unrelated:

@@ -4525,7 +4527,8 @@ def dl_database(db_dir, dl_dir, records='all', annotators='all',
         db_dir = posixpath.join(dir_list[0], get_version(dir_list[0]), *dir_list[1:])
     else:
         db_dir = posixpath.join(db_dir, get_version(db_dir))
-    db_url = posixpath.join(download.PN_CONTENT_URL, db_dir) + '/'
+
+    db_url = posixpath.join(download.PN_CONTENT_URL, db_dir)
     # Check if the database is valid
     _url.openurl(db_url, check_access=True)
 

I think this is probably a mistake, we do want to request a path ending in slash (the path with the slash is the "real" URL, without the slash is a redirection.) I don't remember off the top of my head if check_access=True will treat a redirected URL as valid or not.

bemoody avatar Mar 31 '22 22:03 bemoody

Yes, let's set the param name toparallel.

And I agree that this change isn't meant as a way to avoid solving the actual issue.

cx1111 avatar Apr 01 '22 17:04 cx1111