Skip to content
This repository has been archived by the owner on Dec 26, 2023. It is now read-only.

Add concurrent downloading #1

Merged
merged 12 commits into from
May 12, 2019
Merged

Add concurrent downloading #1

merged 12 commits into from
May 12, 2019

Conversation

csnewman
Copy link
Contributor

Download speed seems to be limited per request, not per IP or user.
This enables much faster downloading.

Copy link
Owner

@ed-cooper ed-cooper left a comment

Choose a reason for hiding this comment

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

You have modified settings-template.py.

Please update README.md to match the changes

run.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
run.py Show resolved Hide resolved
@ed-cooper ed-cooper added good first issue Good for newcomers and removed good first issue Good for newcomers labels May 11, 2019
@ed-cooper
Copy link
Owner

Also, did you install any modules for the concurrent.futures?

If so, please run

pip freeze > requirements.txt

You'll also need to pull the latest changes from the repo first if this is the case

@csnewman csnewman changed the title Add concurrent downloading [WIP] Add concurrent downloading May 12, 2019
@csnewman
Copy link
Contributor Author

Also, did you install any modules for the concurrent.futures?

If so, please run

pip freeze > requirements.txt

You'll also need to pull the latest changes from the repo first if this is the case

pip freeze is considered bad practice as it exposes dependencies of your dependencies. It also lists all packages installed in your virtual environment. I have instead used:
pipreqs . --force

@ed-cooper
Copy link
Owner

That's fine except for flake8.
Obviously this is required on the Travis CI and can be added to the .travis.yml file, but I also think it's useful to have it in the repo for local testing?

@ed-cooper
Copy link
Owner

What command did you use to include flake8?
Or did you just add it manually to the file?

@csnewman csnewman changed the title [WIP] Add concurrent downloading Add concurrent downloading May 12, 2019
@csnewman
Copy link
Contributor Author

Ready for second round of review.

@ed-cooper
Copy link
Owner

I think set the default concurrent streams to 4 for people on WiFi (both in the template and README)

README.md Outdated
@@ -14,6 +14,7 @@ Then, open it in your editor of your choice, and set the following 3 variables:
username = "Your username" # The username you would usually use for My Manchester
password = "Your password" # The accompanying password
base_dir = "~/Documents/Lectures" # Where to download files to
concurrent_downloads = 8 # How many podcasts to download concurrently (Increases speed)
Copy link
Owner

Choose a reason for hiding this comment

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

Use simultaneously instead of concurrently for the description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Concurrent is the correct term here, as the downloads are overlapping over a period of time, whereas simultaneously refers to a occurring at the same instant.

@csnewman
Copy link
Contributor Author

I think set the default concurrent streams to 4 for people on WiFi (both in the template and README)

Default lowered.

@ed-cooper
Copy link
Owner

ed-cooper commented May 12, 2019 via email

@csnewman
Copy link
Contributor Author

Yes, but most people don't know what concurrent means, whereas simultaneous makes more sense for normal people

In that case, they will not mess with the setting. We can link to a dictionary or StackOverflow post explaining the meaning if it becomes an issue.

@ed-cooper
Copy link
Owner

ed-cooper commented May 12, 2019 via email

@csnewman
Copy link
Contributor Author

I held a democratic poll at brunch and the results were: Simultaneous - 5 Concurrent - 0

I sadly do not accept democracy and ran a second referendum on this issue:
Simultaneous = -5
Concurrent = +10000

@ed-cooper
Copy link
Owner

ed-cooper commented May 12, 2019 via email

ed-cooper
ed-cooper previously approved these changes May 12, 2019
@ed-cooper ed-cooper changed the title Add concurrent downloading [WIP] Add concurrent downloading May 12, 2019
@csnewman
Copy link
Contributor Author

Where would you suggest the link being placed. As it is not possible to have a link inside a code fragment

@ed-cooper
Copy link
Owner

How about after the code fragment?

I was thinking having a whole paragraph dedicated to explaining the meaning of concurrency and why they are so much more than simultaneous downloads.

@csnewman
Copy link
Contributor Author

A peer reviewed paper might be the only solution to that problem.

However in the meantime, the wording has been changed.

@csnewman csnewman changed the title [WIP] Add concurrent downloading Add concurrent downloading May 12, 2019
Copy link
Owner

@ed-cooper ed-cooper left a comment

Choose a reason for hiding this comment

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

👍

@ed-cooper ed-cooper merged commit aa5c2cb into ed-cooper:master May 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants