-
Notifications
You must be signed in to change notification settings - Fork 129
Feat/sync artists albums #92
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this matching algorithm of just looking at the name would be too error prone to be used reliably. When matching strings like this you generally have to use multiple fields in order to avoid false positives and false negatives, as per the match() function. For albums you should also use the ipc code if it exists
|
I've improved the artist and album syncing. When syncing a users' album, the algorithm preferable uses the UPC code to retrieve it from the tidal API, if none is present it uses the search function with a query string and afterwards verifies the name, the artist and the number of tracks (I decided not to include the check for the release date as this reduced the syncing quality as for some reason some albums don't have the same release date as on Spotify...). The artist syncing matches artists by retrieving a few albums of the artist from the Spotify API. Afterwards it searches these albums on tidal and checks whether the artists are the correct one. I've also added a fallback to match an artist by their top tracks if none albums are available. |
|
If that works for you, I can create a cleanup commit. |
|
Sure that's a reasonable starting point, I'd need to go through and do some testing to find the right balance between speed and accuracy though. In general the PR needs quite a lot of cleaning up so if you could do your best to clean up, simplify, refactor, and match the existing style that would be appreciated, then I can take over the PR. |
src/spotify_to_tidal/sync.py
Outdated
| try: | ||
| albums = spotify_session.artist_albums(artist_id, album_type="album,single", limit=5)['items'] | ||
| return albums | ||
| except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general it's very unlikely that you should need to catch a general Exception in a function like this. If there's a specific exception that needs to be handled here then please catch that specific exception. If not then just remove the try block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect a maximum of ONE instance of catching except Exception as e for the entire project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that right. I should have thrown a more specific exception like spotipy.exceptions.SpotifyException. But I removed this function in my last commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still 6 other instances of catching Exception though ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored the code to remove most of the try catch blocks. The one remaining in search_album_on_tidal is needed as the method get_albums_by_barcode is raising an ObjectNotFound exception when it cannot find an album with the given UPC.
|
Btw have you tested this? |
|
Yes, I have tested this and can confirm that both the artists and the album syncing do work. I've did some clean up, improved the artist syncing and reduced the number of methods. |
julence
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worked for me
|
@timrae does this look good to you now? |
|
@jlindbergh can you look into this and merge the PR? |
Hi! Unfortunately I don't have time to look deeper into this, nor have I the privileges to approve a merge. It does look pretty reasonable to me though (without having really tested it...). Not sure what happens with saved spotify sessions when the |
|
Hi @jlindbergh, |
|
Sorry I haven't had capacity to look into this. Currently I'm the only maintainer, and am going to be pretty occupied with other projects for at least the next few months. I'd say continuing in your fork for now with the goal of merging back at a later point would be pragmatic for now |
|
Thanks @c0ball for the work on this feature! I've run I added a |
|
+1 on the above, getting the same error |
|
Hi @EBendinelli, |
|
@EBendinelli @eignatenkov I was in a hurry, trying to reproduce the error, but was not able to. Could you try the latest commit in which I added ISRC validation and let me know if the error is still thrown and if synchronization works? |
|
it fails differently now |
|
After the last fix it stopped failing. It gave out a gigantic output of the kind like below just for one artist but then it went okay with the next ones (this is just a small part of it) |
|
Give me some time for further debugging, I'll hit you up |
|
Thank @c0ball for your work. I tired just now and I think I got rate limited after 183 artists? Relaunching the command processed the remaining 100 artists without issue. I also got a few errors similar to @eignatenkov : |
|
I have a question about album sync. Re-running it, to be specific. when I re-run artist-sync, it only tries again to add the artists which had errors on the previous run. I got 20 something left, only they are processed again, and they fail again :) you can see that I'm at a point where the tool wants to synchronize only 731 albums out of 1702, it goes through the list, fails sometimes, other entries are processed without any comments, but when I try to re-run it, I'm back at 731 albums it wants to add, and the output is the same. So I'm not sure what is happening there. |
|
Thanks for pointing that out: I'll also have an eye on this |
…led album sync for further debugging
|
@eignatenkov @EBendinelli Sorry that it took a little longer than expected. Therefore, I would be extremely grateful if you could test this new version and provide feedback. 🙏 |
|
Ran album-sync. In the beginning it said that it found 1798 albums on spotify. Then it started trying to add 875. In the end it said I can see 16 new albums added in tidal interface I see one new album Another album added I'd say it's kind of unclear what is happening. Is 876 a maximum number of albums that can be processed in one go? And it still looks like it re-adds again the albums there were already synced |
not syncing casesartist name western / cyrillic
removed from spoti / available on tid track & name upper / lowercase diff
|
No description provided.