Make spotDl faster and more resilient #999

MHausBermel · 2020-11-28T00:04:47Z

I was trying to process a large playlist (~560 songs), which brought these flaws to light, so I decided to take a quick, hacky jab at improving it.

In this work:

Get info for songs in parallel
Set larger chunk size for parallel processing of playlist songs
Wrap error prone code to enable the app to keep running after errors
Restructured to make the entrypoint more testable
- Eliminated the spotifyClient singleton pattern in favor of dependency injection
- Created a SpotifyDlApp class into which our configured dependencies may be injected for testability
- Note: although this doesn't perfectly adhere to SOLID principals, it was a quick way to get some testing going without, in my opinion, compromising on any of the important details
Reworked argument parsing to use argparse -- I find the API easy to use, and really easy to add on to without deep-diving into the documentation
Added basic tests for argument parsing and early exits
Changed the 'skip song' behavior -- it seemed to hang up with the prior implementation. This seems to work, but this could potentially mess with the progress output.
Provided the ability for users to specify their own spotify dev information
- when I first encountered issues, I assumed it was due to these creds being potentially revoked as I'm not used to seeing them committed to the repo (well, certainly not the secret). Figured I'd make them configurable in case those creds are changed/revoked for some reason.

Other notes:

I saw a few different code styles -- I tried to match what was most prevalent in any given file
I haven't written Python in some time, please feel free to recommend more pythonic approaches when appropriate

…cessing can continue on error.

MHausBermel · 2020-11-28T00:05:07Z

Whoops.

ghost · 2020-11-29T05:27:26Z

What happened ?

…g parsing and early exit tests. Allow user to provide spotifyId and spotifySecret via CLI.

MHausBermel · 2020-11-29T19:46:06Z

I got pulled away and realized I wanted to address some things for fun. I've updated the PR description 👍

aklajnert · 2020-11-30T05:36:27Z

spotdl/test/test_arg_behavior.py

+        with self.assertRaises(SystemExit) as captor:
+            with patch('spotdl.search.spotifyClient.SpotifyClient') as mock:
+                with patch('spotdl.download.downloader.DownloadManager') as dlMock:
+                    app = SpotDlApp()


Great that you've created some tests, just a small hint - you don't need to have separate with statement to use multiple context managers. You can chain them by using comma, that would eliminate the right-way drift:

with self.assertRaises(SystemExit) as captor, \ patch('spotdl.search.spotifyClient.SpotifyClient') as mock, \ patch('spotdl.download.downloader.DownloadManager') as dlMock: app = SpotDlApp()

aklajnert · 2020-11-30T05:37:56Z

spotdl/spotDlApp.py

+    default_client_id = '4fe3fecfe5334023a1472516cc99d805'
+    default_client_secret = '0f02b7c483c04257984695007a4a8d5c'
+
+    def __parseArgs(self, cmdLine) -> Namespace:


this function should start with a single underscore (https://dbader.org/blog/meaning-of-underscores-in-python)

I had forgotten about that entirely since the last time I really used python -- got me feeling all nostalgic haha. Thanks for the reminder!

aklajnert · 2020-11-30T05:43:17Z

spotdl/search/spotifyClient.py

@@ -6,67 +6,63 @@
 from spotipy.oauth2 import SpotifyClientCredentials


+class SpotifyClient():


The parenthesis is not needed here.
In general, it's a good idea for replacing the ugly global with a class. I wanted to do that myself in #992, but my focus was different there.

aklajnert · 2020-11-30T05:46:27Z

spotdl/search/songObj.py

@@ -27,7 +26,7 @@ def __init__(self, rawTrackMeta, rawAlbumMeta, rawArtistMeta, youtubeLink):
    #! Note, since the following are class methods, an instance of songObj is initialized
    #! and passed to them
    @classmethod
-    def from_url(cls, spotifyURL: str):
+    def from_url(cls, spotifyURL: str, spotifyClient):


You don't need to pass spotifyClient, as you can get its instance from anywhere.

You're right, we could new up an instance if we wanted to.
While passing the spotifyClient in via params creates a bit of noise and, perhaps, a bit of inconvenience, I prefer this approach for a couple reasons.

This allows DI to take place at the platform boundary/application entrypoint. Each instance of the class needs credential information, which is now made available via CLI args (with the existing as a fallback).

For testability, I wanted to method params or constructor injection

I also went back and created a SpotifyClientFactory which allows the SpotDlApp instance to init our instance in a more testable way. Being sure to inject dependencies into SpotDlApp means that, as development continues, the project can move towards boundary to boundary 'unit' testing, which covers the expected functionality of both individual classes as well as the way they're all collectively configured to function.

ghost · 2020-11-30T06:11:03Z

Cool. @MHausBermel I'll have a look. We probably won't be taking all changes from this PR as we are currently shifting to using Asyncio instead of multiprocessing. The other ideas seem interesting. Give me till the weekend.

ghost · 2020-11-30T06:12:52Z

And @aklajnert nice to see you looking around.

…stantiation.

…tion of SpotifyClientFactory.

MHausBermel · 2020-12-06T17:36:50Z

Thanks for the feedback, I appreciate it!

We probably won't be taking all changes from this PR as we are currently shifting to using Asyncio instead of multiprocessing.
No problem, I understand! If I had a bit more time, I might take a stab at introducing Asyncio here too, but I don't think I have the time just now.

I introduced the SpotifyClientFactory so we can share configuration of that class in both test and prod code. In tests, we could mock the factory to return whatever instance of a SpotifyClient that we'd like, thus providing a means of testing our interactions with the SpotifyClient instance.

You are both much better versed in python than myself, but on the off chance this is actually helpful, this is the command I use to run the tests from the root of the repo: python -m unittest discover

aqilc · 2020-12-22T00:29:31Z

When will this be merged? It seems pretty helpful and would solve a few issues for me.

ghost · 2020-12-22T08:11:52Z

These a few other things were working on. So things are a little stuck up.

Silverarmor · 2021-01-06T23:24:07Z

Hi all. Joining in a bit late on this.

@MHausBermel could you

rebase spotDL:master into your branch to resolve conflicts?
change this PR to target spotDL:dev. We do not allow PRs to go into master directly when making major changes.

Get song info in parallel. Wrap error-prone calls in try/catch so pro…

1dc4bae

…cessing can continue on error.

MHausBermel closed this Nov 28, 2020

Do some basic restructuring to make entrypoint testable. Add basic ar…

29750ef

…g parsing and early exit tests. Allow user to provide spotifyId and spotifySecret via CLI.

MHausBermel reopened this Nov 29, 2020

aklajnert reviewed Nov 30, 2020

View reviewed changes

Marcus Bermel added 4 commits December 6, 2020 09:11

Clean up with clauses per PR feedback

c97eb56

Clean up inappropriate double-underscore-prefixed method.

d0f0ff5

Make a SpotifyClientFactory to enable easier testing of our client in…

2889d5f

…stantiation.

Update documentation to reflect changes to SpotifyClient and the addi…

1646281

…tion of SpotifyClientFactory.

Fix whitespace in comment.

b474579

Silverarmor added the Stale Issue with no activity for the last 30 days label Jan 25, 2021

stale bot closed this Feb 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make spotDl faster and more resilient #999

Make spotDl faster and more resilient #999

MHausBermel commented Nov 28, 2020 •

edited

MHausBermel commented Nov 28, 2020

ghost commented Nov 29, 2020

MHausBermel commented Nov 29, 2020

aklajnert Nov 30, 2020

aklajnert Nov 30, 2020

MHausBermel Dec 6, 2020

aklajnert Nov 30, 2020

aklajnert Nov 30, 2020

MHausBermel Dec 6, 2020

ghost commented Nov 30, 2020

ghost commented Nov 30, 2020

MHausBermel commented Dec 6, 2020 •

edited

aqilc commented Dec 22, 2020 •

edited

ghost commented Dec 22, 2020

Silverarmor commented Jan 6, 2021

		@@ -6,67 +6,63 @@
		from spotipy.oauth2 import SpotifyClientCredentials


		class SpotifyClient():

Make spotDl faster and more resilient #999

Make spotDl faster and more resilient #999

Conversation

MHausBermel commented Nov 28, 2020 • edited

MHausBermel commented Nov 28, 2020

ghost commented Nov 29, 2020

MHausBermel commented Nov 29, 2020

aklajnert Nov 30, 2020

Choose a reason for hiding this comment

aklajnert Nov 30, 2020

Choose a reason for hiding this comment

MHausBermel Dec 6, 2020

Choose a reason for hiding this comment

aklajnert Nov 30, 2020

Choose a reason for hiding this comment

aklajnert Nov 30, 2020

Choose a reason for hiding this comment

MHausBermel Dec 6, 2020

Choose a reason for hiding this comment

ghost commented Nov 30, 2020

ghost commented Nov 30, 2020

MHausBermel commented Dec 6, 2020 • edited

aqilc commented Dec 22, 2020 • edited

ghost commented Dec 22, 2020

Silverarmor commented Jan 6, 2021

MHausBermel commented Nov 28, 2020 •

edited

MHausBermel commented Dec 6, 2020 •

edited

aqilc commented Dec 22, 2020 •

edited