Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix downloading files in windows #330

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

lbugnon
Copy link
Contributor

@lbugnon lbugnon commented Oct 1, 2021

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.

@bemoody
Copy link
Collaborator

bemoody commented Oct 1, 2021 via email

@cx1111
Copy link
Member

cx1111 commented Mar 31, 2022

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.

1 similar comment
@cx1111
Copy link
Member

cx1111 commented Mar 31, 2022

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.

@bemoody
Copy link
Collaborator

bemoody commented Mar 31, 2022

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.

@cx1111
Copy link
Member

cx1111 commented Apr 1, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants