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

[feat]: Add support to external downloader aria2p #31500

Merged
merged 23 commits into from
Feb 9, 2023
Merged

[feat]: Add support to external downloader aria2p #31500

merged 23 commits into from
Feb 9, 2023

Conversation

teddy171
Copy link
Contributor

@teddy171 teddy171 commented Jan 27, 2023

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly
  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])
  • Use Preview tab to see how your pull request will actually look like

Before submitting a pull request make sure you have:

In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

  • Bug fix
  • Improvement
  • New extractor
  • New feature

Description of your pull request and other information

This pull request support a external downloader aria2p(a command-line tool and Python library to interact with an aria2c daemon process through JSON-RPC). It can make download faster and get the rate of progress and other information such as download speed, eta time more easily(compared with use aria2c only). When use aria2p, install aria2c and aria2p(by pip) first. Run aria2c and enable with the --enable-rpc option in the background. Then all data will download with the aria2c daemon process.

For example:

python -m youtube_dl --external-downloader aria2p "http://www.youtube.com/watch?&v=BaW_jenozKc"

Related: #31188

Copy link
Contributor

@dirkf dirkf left a comment

Choose a reason for hiding this comment

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

Thanks for this interesting proposal.

Although (currently) it would only be useful with Python versions that are supported for aria2fp, I'd be happy to incorporate it as long as there are no unwanted side-effects when it's not available or supported.

Supposing someone has yt-dl installed with Python 2.7 but also has aria2fp installed with a supported Python3, there could be an option to invoke aria2fp by its CLI. I guess that would best be addressed with two FD classes, perhaps with some inheritance.

I've made some detailed suggestions to consider. Let me know what you think.

youtube_dl/downloader/common.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
@teddy171 teddy171 requested a review from dirkf January 28, 2023 00:24
dirkf

This comment was marked as resolved.

@dirkf dirkf requested review from dirkf and removed request for dirkf January 28, 2023 00:37
Co-authored-by: dirkf <fieldhouse@gmx.net>
@teddy171
Copy link
Contributor Author

I think there is some problems. It raise AttributeError: 'Aria2pFD' object has no attribute '_filename' in line 256 external.py. Should I use the function undo_temp_name which in FileDownloader?

@dirkf
Copy link
Contributor

dirkf commented Jan 28, 2023

If you apply all the suggestions, and now that I've fixed the bad one, the CI tests might pass. Install flake8 with pip and run flake8 on your code to identify code issues that will fail the Linter step (unused variables, indentation, etc).

You could make up a test harness like youtube-dl/test/test_downloader_http.py to check that your code actually works. The CI tests will only show that it doesn't break other things.

@rautamiekka
Copy link
Contributor

Surely using PyCharm, being a very powerful dedicated Python IDE, would help a lot.

@dirkf
Copy link
Contributor

dirkf commented Jan 30, 2023

A test harness can be committed into the repository and used to test regressions (though in this case it would have to be disabled from the automatic CI tests).

@teddy171
Copy link
Contributor Author

teddy171 commented Feb 3, 2023

Could you tell me how to write my test? Seem the test environment doesn’t install aira2c and aria2p.

@dirkf
Copy link
Contributor

dirkf commented Feb 6, 2023

Make your test succeed trivially if the aria2p package isn't available, but run fully if it is. Post the successful test log from your environment.

@teddy171
Copy link
Contributor Author

teddy171 commented Feb 7, 2023

My test log

$ python -m unittest test/test_external_downloader.py 
..
----------------------------------------------------------------------
Ran 2 tests in 0.667s

OK

Copy link
Contributor

@dirkf dirkf left a comment

Choose a reason for hiding this comment

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

Almost ready to go, I hope.

test/test_external_downloader.py Outdated Show resolved Hide resolved
test/test_external_downloader.py Outdated Show resolved Hide resolved
test/test_external_downloader.py Outdated Show resolved Hide resolved
test/test_external_downloader.py Outdated Show resolved Hide resolved
test/test_external_downloader.py Outdated Show resolved Hide resolved
test/test_external_downloader.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
teddy171 and others added 6 commits February 8, 2023 07:05
Co-authored-by: dirkf <fieldhouse@gmx.net>
Co-authored-by: dirkf <fieldhouse@gmx.net>
Co-authored-by: dirkf <fieldhouse@gmx.net>
Co-authored-by: dirkf <fieldhouse@gmx.net>
Co-authored-by: dirkf <fieldhouse@gmx.net>
@teddy171
Copy link
Contributor Author

teddy171 commented Feb 8, 2023

Thank you for your suggestions @dirkf. I have applied your suggestions. Is it able to merge now?

Copy link
Contributor

@dirkf dirkf left a comment

Choose a reason for hiding this comment

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

I'll just apply some Lint, style and simplification fixes. If the CI tests work, please post your test output too. If all good we can merge.

test/helper.py Show resolved Hide resolved
test/helper.py Show resolved Hide resolved
test/test_downloader_http.py Outdated Show resolved Hide resolved
test/test_external_downloader.py Outdated Show resolved Hide resolved
test/test_http.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
youtube_dl/downloader/external.py Outdated Show resolved Hide resolved
test/test_external_downloader.py Outdated Show resolved Hide resolved
@teddy171
Copy link
Contributor Author

teddy171 commented Feb 9, 2023

The new code is OK. Thank you @dirkf. Test Log

$ python -m unittest test/test_external_downloader.py 
..
----------------------------------------------------------------------
Ran 2 tests in 0.586s

OK

@dirkf
Copy link
Contributor

dirkf commented Feb 9, 2023

And in the CI tests, as expected:

test_chunked (test.test_external_downloader.TestAria2pFD) ... SKIP: aria2p module not found
test_regular (test.test_external_downloader.TestAria2pFD) ... SKIP: aria2p module not found

On reflection the test file ought to be named like the one for HTTP, so I did that.

@dirkf dirkf merged commit 9256b02 into ytdl-org:master Feb 9, 2023
@teddy171 teddy171 deleted the Aria2pFD branch February 9, 2023 22:43
github-actions bot added a commit to hellopony/youtube-dl that referenced this pull request Feb 10, 2023
dirkf added a commit that referenced this pull request Feb 13, 2023
* feat: add class Aria2pFD

* feat: create call_downloader function

* feat: a colorful download interface to aria2pFD

* feat: change value name

* Apply suggestions from code review

Co-authored-by: dirkf <fieldhouse@gmx.net>

* Typo in suggestion

* fix: remove unused value

* fix: add not function to return value(0 is normal); add total_seconds to download.eta(timedelta object); add waiting status when hook progress

* fix: remove unuse method ..utils.format_bytes

* fix: be up to flake8

* fix: be up to flake8

* Apply suggestions from code review

* [feat] test external downloader aria2p

* [feat] test external downloader aria2p

* [fix] test_external_downloader.py

* Apply suggestions from code review

Co-authored-by: dirkf <fieldhouse@gmx.net>

* Apply suggestions from code review

Co-authored-by: dirkf <fieldhouse@gmx.net>

* Update test/test_external_downloader.py

Co-authored-by: dirkf <fieldhouse@gmx.net>

* Update test/test_external_downloader.py

Co-authored-by: dirkf <fieldhouse@gmx.net>

* Update youtube_dl/downloader/external.py

Co-authored-by: dirkf <fieldhouse@gmx.net>

* refactoring code and fix bugs

* Apply suggestions from code review

* Rename test_external_downloader.py to test_downloader_external.py

---------

Co-authored-by: dirkf <fieldhouse@gmx.net>
github-actions bot added a commit to hellopony/youtube-dl that referenced this pull request Feb 13, 2023
* https://github.com/ytdl-org/youtube-dl:
  [StreamsbIE] Add extractor for streamsb.com (viewsb.com) (ytdl-org#31517)
  [KommunetvIE] Add extractor for kommunetv.no (ytdl-org#31516)
  [FileMoonIE] Add extractor for filemoon.sx (ytdl-org#31515)
  [feat]: Add support to external downloader aria2p (ytdl-org#31500)
  [rbgtum] Add new extractor (ytdl-org#31305)
  [YouTube] Fix tests
  [YouTube] Refresh compat/utils usage * import parse_qs() * import parse_qs in lazy_extractors (clears old TODO) * clean up old compiled lazy_extractors for Py2 * use update_url()
  [compat] Update test_compat
  [utils] Add parse_qs, update_url
  [compat] Systematise compat_ naming
  [test] Fix TestAgeRestriction
  [YouTube] Add `signatureTimestamp` for age-gate bypass
  [YouTube] Bypass age-gating for certain restricted videos
  [Vimeo] Support /user{video_id}/{slug} URL format
  [Vimeo] Fix `Unable to extract info section` redux * as reported in yt-dlp/yt-dlp#6149 * also allow newline in target JSON object
  [jsinterp] Nits
  [IGN] Overhaul extractor to avoid URL redirection loop
alxlive pushed a commit to alxlive/youtube-dl that referenced this pull request Feb 27, 2023
* feat: add class Aria2pFD

* feat: create call_downloader function

* feat: a colorful download interface to aria2pFD

* feat: change value name

* Apply suggestions from code review

Co-authored-by: dirkf <fieldhouse@gmx.net>

* Typo in suggestion

* fix: remove unused value

* fix: add not function to return value(0 is normal); add total_seconds to download.eta(timedelta object); add waiting status when hook progress

* fix: remove unuse method ..utils.format_bytes

* fix: be up to flake8

* fix: be up to flake8

* Apply suggestions from code review

* [feat] test external downloader aria2p

* [feat] test external downloader aria2p

* [fix] test_external_downloader.py

* Apply suggestions from code review

Co-authored-by: dirkf <fieldhouse@gmx.net>

* Apply suggestions from code review

Co-authored-by: dirkf <fieldhouse@gmx.net>

* Update test/test_external_downloader.py

Co-authored-by: dirkf <fieldhouse@gmx.net>

* Update test/test_external_downloader.py

Co-authored-by: dirkf <fieldhouse@gmx.net>

* Update youtube_dl/downloader/external.py

Co-authored-by: dirkf <fieldhouse@gmx.net>

* refactoring code and fix bugs

* Apply suggestions from code review

* Rename test_external_downloader.py to test_downloader_external.py

---------

Co-authored-by: dirkf <fieldhouse@gmx.net>
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.

None yet

4 participants