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

Change url download util to use requests #813

Merged
merged 2 commits into from
Jun 6, 2019
Merged

Conversation

AbhiramE
Copy link
Contributor

@AbhiramE AbhiramE commented Jun 5, 2019

Description

The change changes the url download utility from urlretrieve library to requests. With this change I also get rid of the Tqdm utility class and move to using Tqdm inline.

Requests is a more popular library than urlretrieve.

Related Issues

Checklist:

  • [ x ] I have followed the contribution guidelines and code style for this project.
  • I have added tests covering my contributions.
  • I have updated the documentation accordingly.
    There are tests already covering this use case in the repo. I believe this also requires no update to the documentation.

@miguelgfierro
Copy link
Collaborator

Some evidences here: https://stackoverflow.com/questions/2018026/what-are-the-differences-between-the-urllib-urllib2-and-requests-module

This code had been added to the NLP repo

r = requests.get(url, stream=True)
total_size = int(r.headers.get("content-length", 0))
block_size = 1024
num_iterables = math.ceil(total_size // block_size)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to round up you will need to avoid integer division

math.ceil(total_size / block_size)

@AbhiramE
Copy link
Contributor Author

AbhiramE commented Jun 6, 2019 via email

@gramhagen
Copy link
Collaborator

Do you mean doing this without the math package? Like this, (total_size+block_size-1) / block_size

no, using math is fine, but the // operator does integer division which will round down (10 // 3 = 3), so the ceil is not actually doing anything. but if you do (10 / 3 = 3.333) then apply ceil you will get 4 which is what I'm assuming you want.

@gramhagen
Copy link
Collaborator

awesome, thanks!

@gramhagen gramhagen merged commit a229c94 into staging Jun 6, 2019
@gramhagen gramhagen deleted the abhiram-requests-fix branch June 6, 2019 16:55
yueguoguo pushed a commit that referenced this pull request Sep 9, 2019
Change url download util to use requests
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