tehkillerbee / mopidy-tidal

Tidal Backend plugin for Mopidy
Apache License 2.0
93 stars 28 forks source link

Better caching to avoid timeouts on large collections #57

Closed blacklight closed 2 years ago

blacklight commented 2 years ago

Initializing large collections (or accounts with many playlists) often results in timeouts, even when the collection has already been opened once and the application has simply been restarted.

The current lru_cache implementation only stores cached items in memory, but it'd be good if we could store them to disk (at least the playlist IDs -> names) in order to speed up the initialization time.

tehkillerbee commented 2 years ago

@BlackLight I agree, better caching of the album art would be nice too. It would definitely improve the overall user experience and reliability of this mopidy-tidal

In a sligthly related note, mopidy-tidal actually works well together with clients such as Iris, apart from the horrendously slow loading time after restarting mopdiy.

blacklight commented 2 years ago

mopidy-tidal actually works well together with clients such as Iris

For basic things like loading and listing playlists, yes. But Iris was mainly developed with Spotify in mind, so adding tracks to playlists isn't supported by Iris. But this is an issue on Iris' side, not mopidy-tidal - however, it may be worth implementing the save/create/delete methods on the playlist provider so Iris doesn't have to implement it on the frontend for all the extensions. Some work was done first by me and then picked up by others on mopidy-spotify, maybe it's worth recycling and have a general-purpose algorithm for saving playlists?

I have prepared a PR for disk caching https://github.com/tehkillerbee/mopidy-tidal/pull/60, it works for playlists for now but I'd like to implement it also for albums, tracks and artists - I'm open to comments.

fmarzocca commented 2 years ago

I have installed the better-cache branch. Indeed it is very fast, but the first time ever you load the playlist it's always a pain waiting for playing. Mopidy's core command core.playlists.get_items(value) takes a long time. I should probably find a way to start playing soon after the first track is loaded, keeping the remain loads on background...

I don't have installed python-tidal's branch yet. Should it improve this?

blacklight commented 2 years ago

the first time ever you load the playlist it's always a pain waiting for playing.

That's because the API lookup is still sequential. I think that we should probably have a pool of lookup workers (5 of them should suffice to get a noticeable speedup without overworking the CPU on a humble RPi), but keep in mind that background workers can speed things up only when lookup requests are done in batches. If the client still does the lookup one item at the time there won't be much of an improvement.

I don't have installed python-tidal's branch yet. Should it improve this?

My fork of python-tidal adds support for pagination - it retrieves all the items in case you have playlists/collections with >999 items. And it exposes the lastUpdated field on the playlist objects - without that field mopidy-tidal doesn't know if a playlist has been updated upstream, so it won't refresh the cache in case of updates. So it doesn't really speed things up, but it's still good to have it if you want a more bug-free experience.

fmarzocca commented 2 years ago

I am wondering how does mopidy-spotify work. As soon as I add a playlist in Spotify, it is shown in mopidy (including playlist artwork and tracks).

blacklight commented 2 years ago

I am wondering how does mopidy-spotify work

Details are in lookup.py and playlists.py. In short:

  1. It uses libspotify whenever it's available to retrieve things (and it's usually faster than the web API)
  2. It's quite granular in retrieving either only the playlist info, or only the URIs of its tracks, or the full content of the tracks. I'm not sure if this level of granularity can be achieved with the Tidal API
fmarzocca commented 2 years ago

Weird thing happened. I removed a playlist from my Tidal account. Refreshed list in Iris: the playlist is no more there. Sent a refresh to mopidy directly: curl -d '{"jsonrpc":"2.0","id":1,"method":"core.playlists.refresh","params":{"uri_scheme":"tidal"}}' -H 'Content-Type: application/json' http://localhost:6680/mopidy/rpc

and then asked for the playlist list: curl -d '{"jsonrpc": "2.0","id": 1,"method": "core.playlists.as_list"}' -H 'Content-Type: application/json' http://localhost:6680/mopidy/rpc

The playlist is still there, and I have no way to remove it unless restarting mopidy. As my application does not use Iris, but it directly calls Mopidy's API, I need to understand why this is happening... (I am using the "better-cache" branch.)

tehkillerbee commented 2 years ago

@fmarzocca I tried to replicate this, using the web player and Iris:

  1. Refresh playlists in iris
  2. Delete playlist in Tidal web player
  3. refresh playlists in iris again. The deleted playlist is still displayed

A possibility could be that the playlists are not properly deleted from the cache?

fmarzocca commented 2 years ago

@fmarzocca I tried to replicate this, using the web player and Iris:

  1. refresh playlists in iris again. The deleted playlist is still displayed

indeed, I tried again and you are right. The playlist is not deleted from Iris too. But if you restart Mopidy, the playlist is no more there.

fmarzocca commented 2 years ago

A possibility could be that the playlists are not properly deleted from the cache?

I guess

fmarzocca commented 2 years ago

@tehkillerbee, Mopidy emits the event playlist_deleted for any deleted playlist. See: https://docs.mopidy.com/en/latest/api/core/#mopidy.core.CoreListener.playlist_deleted

fmarzocca commented 2 years ago

when are you planning to release this branch (better-cache)?

blacklight commented 2 years ago

@fmarzocca I was planning to release the last changes by this week, but parental duties took precedence.

I should be able to wrap up with the last points in https://github.com/tehkillerbee/mopidy-tidal/pull/60 in a couple of days max.

fmarzocca commented 2 years ago

@fmarzocca I was planning to release the last changes by this week, but parental duties took precedence.

I should be able to wrap up with the last points in #60 in a couple of days max.

Thanks!

tehkillerbee commented 2 years ago

@BlackLight Great to hear, no rush - really!

@fmarzocca This is an open source project (i.e. free) driven by volunteer work so unfortunately most of us, including myself, have to find time to maintain the project in addition to everyday work and family. This means that PR's and releases do take some time to get through, especially for larger PR's like #60.

But issues, PR's, testers and maintainers are naturally very welcome - and very much appreciated.

fmarzocca commented 2 years ago

This is an open source project (i.e. free) driven by volunteer work so unfortunately most of us,

@tehkillerbee I am very well aware about open source project, I have been a pioneer for many years and have led large Ubuntu communities here in Italy. I am also an author of several Linux packages. I was not asking to push, but because I am taking part to a discussion in Mopidy's forum, where there is some people not happy about this extension, and I am arguing that with the better-cache improvement it works so well that I don't miss spotify. That said, I am using the better-cache branch for 10 days now and I reported an issue. (https://github.com/tehkillerbee/mopidy-tidal/issues/57#issuecomment-1142399050)

fmarzocca commented 2 years ago

Found this bug/issue after a playlist refresh:

ERROR [Core-13] mopidy.core.library TidalBackend backend returned bad data: Expected a list of Track, not [Playlist(last_modified=0, name='Your Time Capsule', tracks=[Track(album=Album(artists=[Artist(name='Sting', uri='tidal:artist:17356')], name='...Nothing Like The Sun', ...

This is for ALL Tidal's playlists

tehkillerbee commented 2 years ago

@fmarzocca Good to hear. It would be nice to dedicate more time - perhaps if Tidal will contribute ;)

Regarding the bug, I noticed the same issue. Take a look at my comments in #60. There are a few issues with this PR, hence the reason why it cannot be merged yet. But the fixes are pretty straightforward, mainly due to the way Mopidy expects lists of tracks and not playlists. There are also a few issues with image loading.

You can try the changes I suggested in the PR and see if it works for you as well. Then we can update the PR accordingly.

fmarzocca commented 2 years ago

I have another note: I cleared tidal's cache (by deleting /var/cache/mopidy/tidal) and restart mopidy. Playlists are not getting refreshed and I had to browse all playlists one by one in order to populate the cache. Is this the expected behavior? [Not refreshed playlists were showing "0" tracks]

tehkillerbee commented 2 years ago

@fmarzocca It does sound similar to some of the issues I experienced during testing and I do not believe this is expected behaviour. Are you using Iris to browse?

If I recall correctly, most of the issues you mention were resolved with my changes to the PR, but I will have to test again later today to make sure. You can see more details in the PR discussion, where I added many comments during testing.

fmarzocca commented 2 years ago

@tehkillerbee I use also Iris, but I mainly interact directly with Mopidy's core API, through my MQTT plugin. It's the same either way.

blacklight commented 2 years ago

Found this bug/issue after a playlist refresh:

ERROR [Core-13] mopidy.core.library TidalBackend backend returned bad data: Expected a list of Track, not [Playlist(last_modified=0, name='Your Time Capsule', tracks=[Track(album=Album(artists=[Artist(name='Sting', uri='tidal:artist:17356')], name='...Nothing Like The Sun', ...

This is for ALL Tidal's playlists

This should be fixed in the latest version of the better-cache branch.

blacklight commented 2 years ago

A possibility could be that the playlists are not properly deleted from the cache?

@fmarzocca @tehkillerbee

This should now be fixed by https://github.com/tehkillerbee/mopidy-tidal/pull/60/commits/1b7f02ad409b7e4bd143002cbbf9c8b9fe5aef46 and https://github.com/tehkillerbee/mopidy-tidal/pull/60/commits/eacc7248f5d66e90890d24e0a9fbf6a3672bdbf8.

The problem was with the existing logic in PlaylistProvider.as_list():

if not self._playlists:
    self.refresh()

# Just use the cached data

So loading even a single playlist meant that we wouldn't ever refresh the list until the app was restarted.

I have fixed this by adding a call to the upstream API to retrieve the users' playlists metadata every time as_list() is called. So if a playlist is added/removed from the app/web player, the changes should be detected as soon as the playlists are retrieved again. This adds a bit of overhead, but it's a single API call (well, actually two, one for the user's playlists and one for the favourites) and in my tests it didn't result in more than ~1 second when retrieving the playlists.

We now need a similar logic also for PlaylistProvider.get_items(uri). Basically, we need to retrieve the playlist metadata from the upstream API on each call, and invalidate the cache entry if the last modified timestamp is greater than the cached one. As of now, changes to a tracklist are only detected when mopidy is restarted.

I would also argue that we need two different pieces of logic for playlist refresh. When as_list() is called, we don't need to retrieve all the tracks for all the playlists, we just need the metadata. If the client wants the tracks, then the client will call get_items(). I expect this single small improvement (which is similar to what mopidy-spotify does) to boost the performance of the first lookup a lot.

blacklight commented 2 years ago

Playlist synchronization in case of track updates (without having to restart mopidy) has now been implemented in https://github.com/tehkillerbee/mopidy-tidal/pull/60/commits/c5ba96f8921dcc2f9dab97307373cbc223708a4a

fmarzocca commented 2 years ago

The problem was with the existing logic in PlaylistProvider.as_list():

Good catch!

If the client wants the tracks, then the client will call get_items(). I expect this single small improvement (which is similar to what mopidy-spotify does) to boost the performance of the first lookup a lot.

In my (tiny) plugin I call get_items() only when the user loads the playlist in the tracklist, no problem. But for Iris, not calling get_items() after a call to as_list()can result that the GUI shows 0 tracks in the playlist, until the user opens the playlist and clicks on "Refresh":

Schermata 2022-06-11 alle 10 01 50

Playlist synchronization in case of track updates (without having to restart mopidy) has now been implemented

Great, thank you. I am going to install your recent updates and test them.

fmarzocca commented 2 years ago

My report:

The last 2 issues are relevant.

blacklight commented 2 years ago

But for Iris, not calling get_items() after a call to as_list() can result that the GUI shows 0 tracks in the playlist, until the user opens the playlist and clicks on "Refresh"

This is a small unfortunate side effect of the current logic. The API actually exposes a num_tracks attribute on the playlist metadata object, with no need to retrieve the full list. But it will be exposed in python-tidal 0.7.0, and I don't feel like backporting yet one more feature to 0.6.x.

  • deleting a track from a Tidal's playlist, it is not cleared from the cache even after a get_items() and it is still reported in the playlist
  • adding a track to a TIdal's playlist, it is not added in the client's frontend, not even after a server restart

How did you test this? My primary use-case is through ncmpcpp/over mpd, and everything works fine there.

From Iris, the playlists get properly refreshed upon each update if you access them from Browser -> Tidal -> My Playlists.

But I have also noticed that playlists don't get refreshed if you just access them under the main Playlists section.

I'm not sure if this is an issue with Iris' caching, or if Playlists and Browse/Tidal/Playlists are treated as different collections and one more event needs to be fired to sync them.

fmarzocca commented 2 years ago

How did you test this?

This is what I did:

Don't know if I am doing something wrong, but this is what I did.

For the sake of clarity: I am not accessing Playlist on IRIS from Tidal->Playlists, but directly from Playlists

EDIT: Now I went to Browse->Tidal->Playlists and refreshed the changed playlist, but the track is still not there.

fmarzocca commented 2 years ago

Just FYI (I don't think it is important) but accessing the playlists from Browse -> Tidal -> My Playlists , it always reports them as "0 tracks".

Schermata 2022-06-11 alle 11 05 14

I tend not to use Iris, but standard users do.

blacklight commented 2 years ago

@fmarzocca I have tried these exact steps and could not replicate the issue.

Added/removed tracks to a playlist from the Tidal UI, called get_items (you won't even need a refresh, it just takes a couple of seconds for the update to the playlist to be propagated through the API), and everything is up-to-date.

The only issue I have noticed is with Iris, which has its own caching system which probably needs to be synchronized.

Can you also check via the MPD integration, since that one is guaranteed to always hit the right methods? Use the mopidy-mpd extension and a client like ncmpcpp. Load the playlist into mopidy, do a playlist change on Tidal, clear the queue on mopidy and load the playlist again. If you can't see the changes from there either, then there must be some weird problem.

(@tehkillerbee if you could also test this, it'd really help me validate if the logic is correct).

Just FYI (I don't think it is important) but accessing the playlists from Browse -> Tidal -> My Playlists , it always reports them as "0 tracks".

This is also expected. As I mentioned, the version of python-tidal that we use doesn't return num_tracks, so a playlist will have 0 tracks until it's looked up the first time.

fmarzocca commented 2 years ago

Use the mopidy-mpd extension and a client like ncmpcpp.

I will arrange for doing that. But don't you believe that firing direct core API calls to mopidy is equally guaranteeded?

blacklight commented 2 years ago

But don't you believe that firing direct core API calls to mopidy is equally guaranteeded?

They should have the same effect, and at least they do have the same effect on my machine. It's just to help pinpointing the root cause.

fmarzocca commented 2 years ago

Il will do other testing later this weekend. For the time being, I did the same procedure on a server of a friend which I can control remotely. Same behaviour, the track added on Tidal's playlist doesn't get listed on Iris or from an API call.

I will report more.

fmarzocca commented 2 years ago

Something should really have been happened here, as I have again this error:

Jun 11 10:42:02 pab mopidy[1206]: ERROR [Core-17] mopidy.core.library TidalBackend backend returned bad data: Expected a list of Track, not [Playlist(last_modified=0, name='* Toco', tracks=[Track(album=Album(artists=[Artist(name='Toco', uri='tidal:artist:4736718')], name='Outro Lugar', uri='tidal:album:17426895'), artists=[Artist(name='Toco', uri='tidal:artist:4736718')], disc_no=1, length=202000, name='Outro Lugar', track_no=1, uri='tidal:track:4736718:17426895:17426896'),...

I am going to wipe the cache again A trivial question: why all Tidal's playlist name has a prepended * ?

fmarzocca commented 2 years ago

Wiped cache, but that error is still there. Something weird is happening

blacklight commented 2 years ago

A trivial question: why all Tidal's playlist name has a prepended * ?

This was already here before the PR. It looks like the extension prepends * to favourite playlists and it doesn't for all the other playlists. No clue of what's the difference because they are shown the same way in the UI, so I'm happy to remove it.

Wiped cache, but that error is still there. Something weird is happening

That error definitely shouldn't happen, because the new version caches playlists as objects, not lists. If you have wiped the cache and the error still persist then you may be running an older version of the code. Run a pip uninstall mopidy-tidal followed by pip install . from the source folder, just in case. Oh, and wipe the cache again before restarting, because it may have a mix of lists and objects.

fmarzocca commented 2 years ago

Run a pip uninstall mopidy-tidal followed by pip install .

Yes! I didn't uninstall it before building the new version. Now everything is OK. The new tracks are in and the removed tracks are out. Thank you and I apologize to have bothered you this morning!

fmarzocca commented 2 years ago

Albums and Artists are OK too.

blacklight commented 2 years ago

@tehkillerbee is there any reason why playlist names are prepended by * if marked as favourite? It's quite redundant, and it breaks a lot of integrations that rely on playlist name because some playlists may suddenly get a * as soon as they are updated. Any objections to removing that logic?

tehkillerbee commented 2 years ago

@BlackLight It was added a long time ago by one of the previous maintainers. Probably just as a simple way to display favourites.

No objections from me - let's delete it :)

FYI, I have not yet had time to test your latest fixes but I will test it again ASAP.

tehkillerbee commented 2 years ago

@BlackLight Looks like most is working as expected, after removing the old version of Mopidy-Tidal and removing cache. I have noted the following:

  1. Excessive Tidal requests result in the following exception. This often happens when creating the cache for the first time. Usually it works again when restarting mopidy.

    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]: ERROR    [Core-7] mopidy.core.library TidalBackend backend caused an exception.
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]: Traceback (most recent call last):
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/lib/python3/dist-packages/mopidy/core/library.py", line 17, in _backend_error_handling
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     yield
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/lib/python3/dist-packages/mopidy/core/library.py", line 181, in get_images
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     if future.get() is None:
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/lib/python3/dist-packages/pykka/_threading.py", line 45, in get
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     _compat.reraise(*self._data['exc_info'])
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/lib/python3/dist-packages/pykka/_compat/__init__.py", line 29, in reraise
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     raise value
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/lib/python3/dist-packages/pykka/_actor.py", line 193, in _actor_loop
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     response = self._handle_receive(envelope.message)
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/lib/python3/dist-packages/pykka/_actor.py", line 299, in _handle_receive
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     return callee(*message.args, **message.kwargs)
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/local/lib/python3.8/dist-packages/Mopidy_Tidal-0.2.8-py3.8.egg/mopidy_tidal/library.py", line 202, in get_images
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     images[uri] = self._get_images(uri)
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/local/lib/python3.8/dist-packages/Mopidy_Tidal-0.2.8-py3.8.egg/mopidy_tidal/library.py", line 183, in _get_images
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     item = getter(item_id)
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/local/lib/python3.8/dist-packages/tidalapi/__init__.py", line 385, in get_album
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     return self._map_request('albums/%s' % album_id, ret='album')
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/local/lib/python3.8/dist-packages/tidalapi/__init__.py", line 455, in _map_request
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     json_obj = self.request('GET', url, params).json()
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/local/lib/python3.8/dist-packages/tidalapi/__init__.py", line 357, in request
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     request = self.basic_request(method, path, params, data, headers)
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/local/lib/python3.8/dist-packages/tidalapi/__init__.py", line 348, in basic_request
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     if not request.ok and request.json()['userMessage'].startswith("The token has expired.") and self.refresh_token:
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/lib/python3/dist-packages/requests/models.py", line 897, in json
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     return complexjson.loads(self.text, **kwargs)
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/lib/python3/dist-packages/simplejson/__init__.py", line 518, in loads
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     return _default_decoder.decode(s)
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/lib/python3/dist-packages/simplejson/decoder.py", line 370, in decode
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     obj, end = self.raw_decode(s)
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:   File "/usr/lib/python3/dist-packages/simplejson/decoder.py", line 400, in raw_decode
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]:     return self.scan_once(s, idx=_w(s, idx).end())
    Jun 17 14:57:29 jlinde-ThinkPad-T430 mopidy[4062]: simplejson.errors.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
  2. 404 errors result in a rather large error message. Perhaps we should strip the URL and keep the URI? Otherwise, the log output quickly gets very messy since it is not uncommon to get 404 errors when an album is removed from Tidal.

    Jun 17 15:02:19 jlinde-ThinkPad-T430 mopidy[4397]: ERROR    [TidalBackend-6] mopidy_tidal.library <class 'requests.exceptions.HTTPError'> when processing URI 'tidal:album:57746860': 404 Client Error: Not Found for url: https://api.tidalhifi.com/v1/albums/57746860?sessionId=0048dc60-7104-42d3-b73c-77122c1fa1a0&countryCode=DK&limit=999
  3. When browsing My Albumsfor the first time, Album art is usually not shown after caching, before refreshing Iris window manually. For some reason, this does not occur when browsing Playlists and Artists. Perhaps this is just an Iris bug.

  4. Browsing Iris Playlists directory and refreshing still results in a huge error message. As I understood, this was already fixed?

    Jun 17 15:05:38 jlinde-ThinkPad-T430 mopidy[4397]: INFO     [TidalBackend-6] mopidy_tidal.playlists Refreshing TIDAL playlists..
    Jun 17 15:05:52 jlinde-ThinkPad-T430 mopidy[4397]: INFO     [TidalBackend-6] mopidy_tidal.playlists TIDAL playlists refreshed
    Jun 17 15:05:52 jlinde-ThinkPad-T430 mopidy[4397]: INFO     [TidalBackend-6] mopidy_tidal.library Browsing uri tidal:my_playlists
    Jun 17 15:05:52 jlinde-ThinkPad-T430 mopidy[4397]: INFO     [TidalBackend-6] mopidy_tidal.library Lookup uris 'tidal:playlist:ae24e029-6024-4fc4-9c83-f77487a77bc7'
    Jun 17 15:05:52 jlinde-ThinkPad-T430 mopidy[4397]: INFO     [TidalBackend-6] mopidy_tidal.library Returning 1 tracks
    Jun 17 15:05:52 jlinde-ThinkPad-T430 mopidy[4397]: ERROR    [Core-7] mopidy.core.library TidalBackend backend returned bad data: Expected a list of Track, not [Playlist(last_modified=0, name='* Decaf', tracks=[Track(album=Album(artists=[Artist(name='Dominic Fike', uri='tidal:artist:7774122')], ...

    To replicate this error, I just browse playlist in Iris and click "Refresh". This error does not occur with Artists and Albums image

fmarzocca commented 2 years ago

@tehkillerbee I am using this branch for a lot of days now, without concerns. I cannot confirm your errors but my suggestion is: do you have wiped tidal cache after updating to this new version?

I have fixed any error by wiping the cache.

tehkillerbee commented 2 years ago

@fmarzocca Thanks for the suggestion but I already stopped mopidy, cleared the cache, reinstalled latest version and restarted mopidy. Everything works except refreshing in Iris playlists returns a similar error before. It looks to me as if the tuple is not unpacked to list of tracks in some cases so the error is returned.

tehkillerbee commented 2 years ago

@BlackLight @fmarzocca I can only get the bug to occur if I refresh in Iris Playlists directory. But as before, the bug is fixed by changing the following:

 tracks += data if hasattr(data, '__iter__') else [data]

change to

if item_type == 'playlist' and not cache_miss:
    tracks += data.tracks
else:
    tracks += data if hasattr(data, '__iter__') else [data]

There might be a cleaner way to do it. If so, feel free to change it. I have updated the PR accordingly.

tehkillerbee commented 2 years ago

@BlackLight @fmarzocca I have merged the PR and made some adjustments. Please feel free to test, if there are no issues, I will make a new PyPi release in a few days time.

fmarzocca commented 2 years ago

@tehkillerbee as far as I understand the cache mechanism relies on @BlackLight's python-tidal fork 0.6.x ( see: https://github.com/tehkillerbee/mopidy-tidal/pull/60#issuecomment-1152297646). How are you going to handle this?

tehkillerbee commented 2 years ago

@fmarzocca While, that fork must be used for pagination and auto playlist updating, the rest of the cache improvements work on the default python-tidal package available on PyPi. Since the fork by @BlackLight is not going to be maintained, and since it is not available on PyPi, I have to stick to the package provided by tamland for now.

This is not ideal, of course. An alternative could be making yet another fork of python-tidal and providing a PyPi package for it but I do not plan on maintaining that as well.

Instead, I would much rather switch to pyopenTIDAL and help rewriting the library into pure Python so it could be provided as a PyPi package.

blacklight commented 2 years ago

I have opened a new issue (https://github.com/tamland/python-tidal/issues/104) on python-tidal.

The developer promised that 0.7 would have come "very soon" (meaning end of May/beginning of June), but nothing has been released so far.

My options:

blacklight commented 2 years ago

Btw it's possible to configure pip to install a dependency from a Github branch (https://stackoverflow.com/questions/20101834/pip-install-from-git-repo-branch), so things should work if we want to go down that path.

Of course, I'd rather not, but if python-tidal shows no signs of life I'm also happy to maintain a new fork with what's there of the 0.7 branch, rather than being constrained by a stuck dependency.

In the meantime, maybe it's worth mentioning in the README that if users want some features (namely, support for collections with >999 items and for cached playlist updates) they need to install my fork? It'd probably save us from a lot of questions.

fmarzocca commented 2 years ago

In the meantime, maybe it's worth mentioning in the README that if users want some features (namely, support for collections with >999 items and for cached playlist updates) they need to install my fork? It'd probably save us from a lot of questions.

From a user point of view, this is not the best solution. Maybe adding the branch dependency could be better, at least temporarily. Then, if python-tidal will publish 0.7, it will be a matter of just changing the dependency in the package.

The decision is up to @tehkillerbee