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

CLI overhaul (+more) #895

Closed
wants to merge 155 commits into from
Closed

Conversation

phcreery
Copy link
Contributor

@phcreery phcreery commented Oct 15, 2020

CLI input

  • argparse library for command-line argument handling.
    • Why: Automated help messages, simpler usage and ease of scalability.
    • image
  • User can now set spotify ID & secret with --spotify-client-id and --spotify-client-secret
  • Developers and Users can now debug with --debug which can be set to local(default)/global/file. local only logs messages from root process. global logs all logs from all scripts & libraries. file outputs the (global) log to a file only.

CLI feedback

New Progress bars!

image

  • willmcgugan/rich python library is now used for messages, progress bars, logging display, and more.
    • Theming is available.
    • Has support for "dumb" consoles (i.e. no special characters, colors, etc.)
    • Why rich and not other libraries?
      • rich is a very supported library (13.3k stars, 45 devs), is updated regularly, and is very versatile.
      • tqdm, although also very supported, is not as "pretty"

Multiprocessing Enhancements

  • Pool.starmap is now Pool.starmap_async to keep the main process running while the other (4) subprocesses are doing their thing. This opens the possibility for the main process to "keep an eye" on the other processes.
    • Mainly needed to so that the Display Manager does not get pickled.
  • queue for process progress communication
  • download_song now has try: ... Except: ... Exception handling with traceback to ensure exceptions get passed back to the main process.
  • multiprocess library switch. (NOTE: not multiprocessING)
    • Why: Difficult time pickling function/classes (on Windows at least). This library uses dill instead of pickle & supports lambda functions
    • I would like to eventually go back to multiprocessing because of support.

What else is included:

  • logging
    • Much easier & more standard way to debug the script
  • with wrappings
    • This ensures download & display closing functions get called upon Exception or KeyboardInterrupt
    with DisplayManager() as disp:
        with DownloadManager() as downloader:
  • requirements.txt for pip(3) packages.
    • Developers/Contributors/Users can now use pip3 install requirements.txt to install needed requirements (without installing pip package.)
    • When developing inside a venv, this allows for separated dependencies usage & tracking.
  • A couple of examples for developers to either use spotDL as a library or for Contributors to use for testing.

There is a lot to review. I anything should be changed or does not follow contributing guidelines, feel free to let me know.
I will try to keep the pull request smaller from now on to prevent pulling overlapping interests/contributions. (Ex. C-String usage)

phcreery and others added 30 commits February 5, 2020 14:35
Flesihing out the ideas behing restructuring/recoding of spot-dl
The basic Ideas behind the restructuring attempt have been put down and will hopefully be updated as and when required. Now we can get straight to the code  buisness.
Starting to work of the restructuring proper. You won't find any new code here but then a little design before coding can go a long way. I do my design via markdown, you should find a lot of that here.  😁
Some messups with the last commit and me trying to revert to a previous commit. Damn it should have been named 'Problems & Solutions III' well, here is the fix.

The interface definitions have been finished. If I just figure out the code class dubbed soulOfSpotDl and the 'tools' proposed I can get down to code. Cheerio. 😁
Fixed up some inconspicious typos, ran some hacky tests, minor changes to interface definitons and some (in a way) working code.
Fixed some typo's, updated working docs, did some tests on logging in ./Hacks and got a working and configured heirarchal logger set up.
Nothing fancy here, just some typo fixes, guideline updates, design notes and recycled code.
Restructured Temp floder to resemble a python package. Minor name changes, a few edits, lots of questions and untested code. I might even have a hacky implementaion of the Metadata Search Interface.
Not much code this time around, was (re)figuring out the interfaces/object ideas, still have to change the docs to mirror the changes in code.
added the gener look up. did the doc strings...
Updated logging guidelines for more clarity of the resulting logs, updated the logging messages interspersed throughout the code to match the new guidelines, updated workingDocs. Chucked in a package diagram for good measure, it forces your to take stalk and stop roming around in circles.

Put up the new stuff since 'Problems & Solutions I' under README updates.
Got in fresh embedding code that should work in theory, tests not yet done.

Slight changes to the package diag.  Some more minor folder renamings and stuff. One step closer to completion (4 more steps to go, assuming someone else will write the search provider which I'm hoping will be YTmusic)

Thinking of ways to highlight the not so great function input variable 'v2_version=' used in mutagen, it tells you absolutely nothing about what the variable does.
Some typos and a lott of code...  ༼ つ ◕_◕ ༽つ (Yayy!!!)
Was adding fresh functionality to spotifyHelpers.py
and added a few more spotify-api responses to the REFS folder for reference
So, my ideas is to do the downloads and conversions in parrlell to speeden up things. Threading is not prrlell processing - multiprocess is. Fiddled around with multiprocessing.

Tried calculating the SHA512 checksum for 33,326 files. single process ~ 25mins, 16 process (since I use a 8core Hyper-V processor) ~ just 3 mins. Far better than I expected.
Looking into various lobraries that can be used to download audio from YouTube and also into the speedy format conversion issue
Copied from the code I wrote for the original library. Includes "test" 
code.
Also updated objects.md with additional  metadata suggestions.
Also updated objects.md with additional  metadata suggestions.
Also includes (commented out) code to get metadata.
YouTube Music's search response is a sprawling, over-nested JS Object. Code to filter out unnecessary data from those musked responses capable of handling all of the common response structures.
Wrote up a partial YouTube Music Based search Provider based off @roketinventors original code. I'm sure his version will be better
Got the search provider complete based of work done by @rocketinventor. He's the one who cracked YTM querys.

Little tweaks to interface definitions.
The Metadata Search interface is no more. All completed classes have Doc Strings now.
@ghost
Copy link

ghost commented Nov 13, 2020

The code is good, it looks awesome. There are a few functional errors, i'll try fixing them up but i'm not very thourough with your code so I'll probably ping you once in a while. Also, it would be good to shift back to multiprocessing as multiprocess doesn't really fix anything as of now. I'd rather keep spotify leas as possible - more dependencies, more likely something will go wrong.

But,

PS D:\Projects\GitHub\spotify-downloader> spotdl '.\She''s Got a Gun.spotdlTrackingFile'
Traceback (most recent call last):
  File "D:\Software\Python39\Scripts\spotdl-script.py", line 33, in <module>
    sys.exit(load_entry_point('spotdl==3.1.4', 'console_scripts', 'spotdl')())
  File "D:\Software\Python39\Scripts\spotdl-script.py", line 25, in importlib_load_entry_point
    return next(matches).load()
  File "D:\Software\Python39\lib\importlib\metadata.py", line 77, in load
    module = import_module(match.group('module'))
  File "D:\Software\Python39\lib\importlib\__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
  File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 664, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 627, in _load_backward_compatible
  File "<frozen zipimport>", line 259, in load_module
  File "D:\Software\Python39\lib\site-packages\spotdl-3.1.4-py3.9.egg\spotdl\__main__.py", line 4, in <module>
ModuleNotFoundError: No module named 'spotdl.cli'
  1. we added support for artist URL's, they don't work now.

  2. More weird errors,

spotdl '.\She''s Got a Gun.spotdlTrackingFile'
Traceback (most recent call last):
  File "D:\Software\Python39\Scripts\spotdl-script.py", line 33, in <module>
    sys.exit(load_entry_point('spotdl==3.1.4', 'console_scripts', 'spotdl')())
Fetching Playlist... 
Process ID: 1480         Error: get_ytplayer_config: could not find match for config_patterns    While downloading: Elddansurin - Heilung
Traceback (most recent call last):
  File "D:\Projects\GitHub\spotify-downloader\spotdl\download\downloader.py", line 104, in download_song
    youtubeHandler = YouTube(
  File "D:\Software\Python39\lib\site-packages\a_pytube_fork_for_spotdl_users-9.6.4-py3.9.egg\pytube\__main__.py", line 103, in __init__
    self.descramble()
  File "D:\Software\Python39\lib\site-packages\a_pytube_fork_for_spotdl_users-9.6.4-py3.9.egg\pytube\__main__.py", line 123, in descramble
    self.player_config_args = get_ytplayer_config(self.watch_html)[
  File "D:\Software\Python39\lib\site-packages\a_pytube_fork_for_spotdl_users-9.6.4-py3.9.egg\pytube\extract.py", line 205, in get_ytplayer_config  
    raise RegexMatchError(
pytube.exceptions.RegexMatchError: get_ytplayer_config: could not find match for config_patterns
 
Process ID: 8560         Error: get_ytplayer_config: could not find match for config_patterns    While downloading: C'est La Vie - Weathers
Traceback (most recent call last):
  File "D:\Projects\GitHub\spotify-downloader\spotdl\download\downloader.py", line 104, in download_song
    youtubeHandler = YouTube(
  File "D:\Software\Python39\lib\site-packages\a_pytube_fork_for_spotdl_users-9.6.4-py3.9.egg\pytube\__main__.py", line 103, in __init__  
    self.descramble()
  File "D:\Software\Python39\lib\site-packages\a_pytube_fork_for_spotdl_users-9.6.4-py3.9.egg\pytube\__main__.py", line 123, in descramble
    self.player_config_args = get_ytplayer_config(self.watch_html)[
  File "D:\Software\Python39\lib\site-packages\a_pytube_fork_for_spotdl_users-9.6.4-py3.9.egg\pytube\extract.py", line 205, in get_ytplayer_config  
    raise RegexMatchError(
pytube.exceptions.RegexMatchError: get_ytplayer_config: could not find match for config_patterns
 
Total                                             4/6 complete       ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╺━━━━━━━━━━━━━━━━━━━━━  67% 0:00:01
She's Got a Gun - KUURO, McCall                   Skipping           ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00
Aequilibrium - Andrey Vinogradov                  Skipping           ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00
C'est La Vie - Weathers                           Error              ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━   0% -:--:--
Gotta Go - Monoplay                               Skipping           ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00
Elddansurin - Heilung                             Error              ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━   0% -:--:--
Nord Mead - Miracle Of Sound                      Skipping           ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00
  1. The setup.py has excess requirements, and lack mutiprocess and rich.

@ghost
Copy link

ghost commented Nov 13, 2020

Well, going through the code in detail to make required changes, reveals some minor design changes to be made.

__main__.py Outdated Show resolved Hide resolved
__main__.py Outdated Show resolved Hide resolved
__main__.py Outdated Show resolved Hide resolved
@phcreery
Copy link
Contributor Author

@Mikhail-Zex Thank you for looking. I will get around to resolving, commenting, etc hopefully later today. And don't worry about critiquing! Keep em coming! This is my first PR after all.

@phcreery phcreery requested a review from a user November 20, 2020 02:04
@ghost
Copy link

ghost commented Nov 21, 2020

Will review today

@ghost
Copy link

ghost commented Nov 22, 2020

Code is good. Your setup.py is broken. and i keep getting this error

Fetching Song...
Process ID: 12516        Error: 'assets'         While downloading: Trøllabundin - Eivør
Traceback (most recent call last):
  File "D:\Software\Python39\lib\site-packages\spotdl-3.1.2-py3.9.egg\spotdl\download\downloader.py", line 104, in  
download_song
    youtubeHandler = YouTube(
  File "D:\Software\Python39\lib\site-packages\pytube3-9.6.4-py3.9.egg\pytube\__main__.py", line 91, in __init__    
    self.prefetch()
  File "D:\Software\Python39\lib\site-packages\pytube3-9.6.4-py3.9.egg\pytube\__main__.py", line 183, in prefetch   
    self.js_url = extract.js_url(self.watch_html)
  File "D:\Software\Python39\lib\site-packages\pytube3-9.6.4-py3.9.egg\pytube\extract.py", line 143, in js_url      
    base_js = get_ytplayer_config(html)["assets"]["js"]
KeyError: 'assets'

Trøllabundin - Eivør                   Error              ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━   0% -:--:--

@phcreery
Copy link
Contributor Author

phcreery commented Nov 29, 2020

@Mikhail-Zex, I am going to close this in favor of asyncio PR #994 . The asyncio implementation drastically changes (and potentially improves) the implementation of most of this code.

I look forward to recreating a PR for prettier progress bars with updated code after the decision of an async library. Probably going to separate the argparse into a separate PR.

@phcreery phcreery closed this Nov 29, 2020
@ghost
Copy link

ghost commented Nov 30, 2020

Ok.

@aklajnert aklajnert mentioned this pull request Dec 11, 2020
@phcreery phcreery mentioned this pull request Dec 15, 2020
@phcreery phcreery mentioned this pull request Jan 9, 2021
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.

4 participants