Persist caches to disk to avoid slow initialization upon restart

blacklight commented 2 years ago

Note: this is still work in progress, but it already considerably reduces the extension bootstrap time upon restart.

Currently implemented:

[x] Playlists cache
[x] Tracks cache
[x] Artists cache
[x] Albums cache
[x] Search cache
[x] Unify the caches: they should all share the same LruCache base class, with filesystem caching support

blacklight commented 2 years ago

@tehkillerbee I have added cache_dir as an optional config parameter (default: <data-dir>/cache) and added disk persistence both for the tracks and playlists cache. After the first load, load time for some of my large playlists (~2500 items) went from ~15 minutes to a couple of seconds.

I also think that it may be worth refactoring of the caches used by the extension, what do you think? Right now there is library.Cache (used for tracks), lru_cache.LruCache (used by search, album tracks and images) and the simple dictionary cache used for playlists. Maybe it's worth using one single class to model them all, so we don't have to reinvent filesystem cache persistency for all of them?

tehkillerbee commented 2 years ago

@BlackLight Great to see you've made progress! I agree, refactoring the caches used in Mopidy-Tidal is a good idea. I noticed the same thing back when I started maintaining this project but did not find the time to do it. (If it works, right?). I am no expert on cache frameworks so I cant say for sure which type that would work best for all use-cases - or if the use of different cache types were intentional by the previous developer.

I'm looking forward to test it later today. We also need to make sure that the default directory is suitable as a cache directory. I'd look at Mopidy documentation for pointers on where these data should be stored: link

fmarzocca commented 2 years ago

Other caches are here:/var/lib/mopidy/.cache/

blacklight commented 2 years ago

Other caches are here:/var/lib/mopidy/.cache/

Good point. I'll prepare a commit to change the cache dir to core/cache_dir instead of creating a subfolder under core/data_dir.

blacklight commented 2 years ago

The persisted cache on storage has been implemented and the caches in the extension have been refactored. We now have a single LruCache class that implements (optional) filesystem persistence, and all the caches are now using it.

The LRU cache keeps a limited amount of items in memory (default: 1024) and all the other cached items are persisted on an indexed cache on the filesystem. The structure of the fs cache is inspired by that of mopidy-spotify:

<cache-dir>
  -> <type>
    -> <prefix>  # First two chars of the TIDAL ID
      -> tidal:<type>:<tidal-id>

In case of a miss on the memory cache, the filesystem cache will be inspected, and in case of a miss on the fs as well the extension would perform a lookup over API.

Some design decisions:

The search cache is not persisted. Search results can change over time and I don't want to go down the rabbit hole of cache invalidation for search results.
Image caches are not persisted. Caching them would blow the size of the cache directory. The price to pay is that images will be re-downloaded on-demand upon restart, but I think it's a quite reasonable trade-off.

Still pending:

I need to properly test it with playlist updates. Things should work in theory (upon load we compare the stored last_modified of the cached Mopidy playlist with the last_updated of the TIDAL playlist, and invalidate the cache entry if the upstream playlist has changed since last time), but I'd like to properly validate it.
I need to test artist caches.

Testing volunteers are welcome!

p.s. @kingosticks @adamcik I guess that several extensions that rely on slow remote backends have this cache persistence problem. The issue has been tackled quite well on mopidy-spotify, and I have tried to replicate some of the lessons learned there in this PR. But maybe it's worth to have something like a CacheProvider in mopidy itself that takes care of the boilerplate of implementing a backend-agnostic LRU persisted cache, so individual extensions don't have to reinvent the wheel?

tehkillerbee commented 2 years ago

@BlackLight Great work, looking forward to test it!

adamcik commented 2 years ago

I don't disagree that this type of cache case is probably shared by multiple plugins, but I'm not sure if adding this to mopidy itself really makes sense. In a similar vein I've been wanting to factor out a bunch of the webservice/oauth type helpers so that we can just reuse things more. But I suspect that it might be better to just have someone create a mopidy-client-libs or a mopidy-plugin-helpers (or whatever a suitable name is).

If there are parts of this that can only be solved in mopidy, and there is real value to having it solved in that codebase then definitely open a bug with why this is the case. Otherwise I think this would benefit more from not being in "core", similar to how we've moved e.g. mopidy-mpd out so it can be improved independently of mopidy releases.

blacklight commented 2 years ago

@adamcik I totally agree - if the plan is to keep the core "lightweight" then non-core features should be moved out of the core, not in.

There's still a valid point though for features that are non-core but still shared by many extensions (such as caching, OAuth flows and playlist updates). Caching and OAuth in particular are implemented independently by multiple extensions (Spotify, SoundCloud, Tidal, YouTube etc.) even though they mostly share the same code - and, in some cases, most of the code of these features is copy-pasted from other extensions.

I like the idea of something like a mopidy-ext-helpers package that could collect most of these features, as well as provide some room for customization/extensibility on top of the boilerplate. We would have a long tail of extensions that should probably migrate at some point, but it's good to at least lay out the foundations.

blacklight commented 2 years ago

The PR is ready for test/review.

The latest changes involve a more consistent cache implementation/usage in library.lookup. I have tested the caching features for artists and albums across restarts with Iris and they also seem to work fine.

I have also fixed the logic for playlist cache invalidation upon updates. We still have a problem though because the current stable branch of python-tidal doesn't currently expose the last_updated attribute of the Tidal playlists, and with no last_updated we don't know if a playlist has been updated since last time we cached it.

More attributes are apparently exposed on the 0.7.x branch of python-tidal, but that version isn't on pip yet (the author said that it should be coming soon, but after waiting about two years I'm not sure of their definition of "soon"). In the meantime, I have pushed a fix myself in my existing PR for pagination, so you may want to use the pagination-support branch from my fork of python-tidal if you want both pagination, and your playlists to be up-to-date when you add/remove tracks.

tehkillerbee commented 2 years ago

Browsing appears very fast indeed! Only issue I see is the images are continuously requested again from the server. I guess this is intentional, as we do not have a local cache of the downloaded images - only the URIs themselves. Perhaps we should consider a local image cache as well?

Edit: This is mainly an issue when requesting the full resolution images. See below comments for details on how to reduce the resolution.

tehkillerbee commented 2 years ago

Sometimes when refreshing a directory, an error is thrown. Perhaps this is just because of excessive Tidal requests?

May 26 10:44:15 jlinde-ThinkPad-T430 mopidy[8625]: ERROR    [TidalBackend-6] mopidy_tidal.library HTTPError when processing URI 'tidal:album:38264741': 404 Client Error: Not Found for url: https://api.tidalhifi.com/v1/albums/38264741?sessionId=<session>8&countryCode=DK&limit=999
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]: ERROR    [Core-7] mopidy.core.library TidalBackend backend caused an exception.
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]: Traceback (most recent call last):
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/lib/python3/dist-packages/mopidy/core/library.py", line 17, in _backend_error_handling
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     yield
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/lib/python3/dist-packages/mopidy/core/library.py", line 181, in get_images
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     if future.get() is None:
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/lib/python3/dist-packages/pykka/_threading.py", line 45, in get
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     _compat.reraise(*self._data['exc_info'])
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/lib/python3/dist-packages/pykka/_compat/__init__.py", line 29, in reraise
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     raise value
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/lib/python3/dist-packages/pykka/_actor.py", line 193, in _actor_loop
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     response = self._handle_receive(envelope.message)
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/lib/python3/dist-packages/pykka/_actor.py", line 299, in _handle_receive
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     return callee(*message.args, **message.kwargs)
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/local/lib/python3.8/dist-packages/Mopidy_Tidal-0.2.8-py3.8.egg/mopidy_tidal/library.py", line 181, in get_images
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     album = session.get_album(album_id)
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/local/lib/python3.8/dist-packages/tidalapi/__init__.py", line 385, in get_album
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     return self._map_request('albums/%s' % album_id, ret='album')
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/local/lib/python3.8/dist-packages/tidalapi/__init__.py", line 455, in _map_request
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     json_obj = self.request('GET', url, params).json()
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/local/lib/python3.8/dist-packages/tidalapi/__init__.py", line 357, in request
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     request = self.basic_request(method, path, params, data, headers)
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/local/lib/python3.8/dist-packages/tidalapi/__init__.py", line 348, in basic_request
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     if not request.ok and request.json()['userMessage'].startswith("The token has expired.") and self.refresh_token:
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/lib/python3/dist-packages/requests/models.py", line 897, in json
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     return complexjson.loads(self.text, **kwargs)
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/lib/python3/dist-packages/simplejson/__init__.py", line 518, in loads
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     return _default_decoder.decode(s)
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/lib/python3/dist-packages/simplejson/decoder.py", line 370, in decode
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     obj, end = self.raw_decode(s)
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:   File "/usr/lib/python3/dist-packages/simplejson/decoder.py", line 400, in raw_decode
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]:     return self.scan_once(s, idx=_w(s, idx).end())
May 26 10:44:20 jlinde-ThinkPad-T430 mopidy[8625]: simplejson.errors.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

tehkillerbee commented 2 years ago

When 404 occurs for a specific URI, the URI is not added to the cache.

Instead, we should probably add a dummy entry to the cache to avoid retrying on next refresh.

May 26 11:01:25 jlinde-ThinkPad-T430 mopidy[9505]: ERROR    [TidalBackend-6] mopidy_tidal.library HTTPError when processing URI 'tidal:album:38264741': 404 Client Error: Not Found for url: https://api.tidalhifi.com/v1/albums/38264741?sessionId=<session ID>&countryCode=DK&limit=999

kingosticks commented 2 years ago

Am I right in saying that for every cache miss in lookup() you will write once to disk? Can you write them once in bulk at the end?

blacklight commented 2 years ago

Perhaps we should consider a local image cache as well?

I thought of it, but I have kept things like this for now because I'm not sure about the storage requirements. AFAIK mopidy-spotify caches images too, and for this reason mopidy-spotify's cache directory can easily get to a few GB in size. If a 320x320 image is about 40 KB in size then storing 10,000 of them (that's quite realistic after a few months/years spent browsing music) can easily result in ~400 MB of cache storage. Maybe there could be a configuration flag for caching images? It can be enabled by default, and if one is running the app on a system with low storage it can be disabled. And maybe it's also good to implement an LRU mechanism for this cache so it doesn't explode in size. @kingosticks any suggestions on this?

Sometimes when refreshing a directory, an error is thrown. Perhaps this is just because of excessive Tidal requests?

I've also got the same error occasionally. Maybe it's worth investigating if it reproduces with the same track? The 404 error makes me think that it may be a temporarily unavailable track, or maybe a track not available in the user's market. If they're using a 404 for a limit exceeded error then they're doing HTTP wrong :)

Instead, we should probably add a dummy entry to the cache to avoid retrying on next refresh.

It depends on what the error is IMHO. If 404 means "this track is not available (at all)", then it makes sense to add a dummy entry to avoid reloading it. If instead it's due to a service temporarily unavailable or a limit exceeded, then we want to try and reload it on the next lookup - and a dummy entry wouldn't allow us to refresh the cache entry. In my tests I have noticed that when these errors happen it's only the invalid track that gets a cache miss, not the whole playlists, so hopefully this shouldn't have a big performance impact.

Am I right in saying that for every cache miss in lookup() you will write once to disk? Can you write them once in bulk at the end?

We cache tracks outside of the for in library.lookup, but playlists, albums and tracks are indeed cached within the for. They shouldn't be as write-intensive as tracks, but it's indeed a good idea to write them to the cache outside of the cycle.

tehkillerbee commented 2 years ago

I thought of it, but I have kept things like this for now because I'm not sure about the storage requirements. AFAIK mopidy-spotify caches images too, and for this reason mopidy-spotify's cache directory can easily get to a few GB in size. If a 320x320 image is about 40 KB in size then storing 10,000 of them (that's quite realistic after a few months/years spent browsing music) can easily result in ~400 MB of cache storage. Maybe there could be a configuration flag for caching images? It can be enabled by default, and if one is running the app on a system with low storage it can be disabled. And maybe it's also good to implement an LRU mechanism for this cache so it doesn't explode in size. @kingosticks any suggestions on this?

@BlackLight Interesting, I did not know mopidy-spotify already did this. My suggestion would also be to make the image caching an option that can be enabled if necessary. It would speed up the browsing experience when using clients that rely heavily on album art (such as Iris), that's for sure.

I've also got the same error occasionally. Maybe it's worth investigating if it reproduces with the same track? The 404 error makes me think that it may be a temporarily unavailable track, or maybe a track not available in the user's market. If they're using a 404 for a limit exceeded error then they're doing HTTP wrong :)

Yes, I see the 404 error every time I load my list of albums. Some of the albums I have added has probably been removed again by the artist. In the Tidal player, the albums are still displayed but cannot be played back. Unfortunately, tidalapi does not handle unavailable albums gracefully so they cannot be displayed properly.

The following JSONDecodeError error is something I have not seen before the caching changes has been introduced. It seems to occur after refreshing the folder a few times within a short period of time. Perhaps it is just Tidal's response to too many repeated requests?

It depends on what the error is IMHO. If 404 means "this track is not available (at all)", then it makes sense to add a dummy entry to avoid reloading it. If instead it's due to a service temporarily unavailable or a limit exceeded, then we want to try and reload it on the next lookup - and a dummy entry wouldn't allow us to refresh the cache entry. In my tests I have noticed that when these errors happen it's only the invalid track that gets a cache miss, not the whole playlists, so hopefully this shouldn't have a big performance impact.

Agreed, although I have only seen 404 errors on unavailable albums not as a general error. In any case, the albums are just left unpopulated in the list and the remaining albums can still be listed.

blacklight commented 2 years ago

It seems to occur after refreshing the folder a few times within a short period of time.

It sounds like caching implies faster throughput -> more requests per second -> we're more likely to hit the API's throttling thresholds. But again, if that's indeed the case, they should probably return 429 (even a 403 would be better), not 404. And, also, a JSON response even in case of error 👇

The following JSONDecodeError error is something I have not seen before the caching changes has been introduced.

If the actual response code is 404 then it's likely that they just return HTML/text, hence the JSONDecodeError, but it may be worth to add a print to check that it's indeed the case.

blacklight commented 2 years ago

@tehkillerbee indeed, it seems that we always have a cache miss on images. I didn't wire everything properly with the existing caching logic, and now I'm just in the middle of refactoring it so some of the repeated that can be easily parametrized goes away.

On a closer look of the existing logic (as well as the image lookup logic in mopidy-spotify) it seems though that we always cache Image objects - I'm not sure if those eventually get cached like full images on mopidy-spotify or just URIs. I'll stick to the same logic for this PR - and if caching an Image object involves only caching the URL then an additional cache_images flag may be redundant.

kingosticks commented 2 years ago

I'm a bit confused. As I remember, Mopidy-Spotify doesn't cache actual image data, just the result of the Spotify API call which has the actual image URI. Are you trying to cache the actual image data here? How are clients then getting access to that data? Do I misunderstand? I'm sorry but I've not reviewed the latest code fully.

blacklight commented 2 years ago

@kingosticks

As I remember, Mopidy-Spotify doesn't cache actual image data, just the result of the Spotify API call which has the actual image URI.

You're right indeed. Initially I though that the Image object would perform a lazy inizialitation of the image data as well, but at a closer look I noticed that it only contains the data necessary to construct the image URL.

So to answer @tehkillerbee - I guess that we can't really talk of server-side full image caching: the web client is already in charge of caching static images usually, on the server we'll only cache the URLs.

I've also pushed new changes that involve a major rewrite of the caching logic in the library module (it was very spaghetti-like and full of copy-pasted sections). This should also fix the issue with image cache misses, as well as rationalize the caches used in the code (we now use both the same lookup logic and the same cache objects, no more nested if's per URI type, no more different caches for track lists and object models). I've tested it both against Iris and mopidy for several cases (artists, albums, playlists and single tracks) and things seem to work.

fmarzocca commented 2 years ago

IMHO there is no sense to cache the image data, if ever the images url...

tehkillerbee commented 2 years ago

IMHO there is no sense to cache the image data, if ever the images url...

@fmarzocca It appears to be faster to use the cached image URL instead of requesting it anew. So it will provide a better user experience and images will be displayed faster.

tehkillerbee commented 2 years ago

@BlackLight These are my comments after testing. It appears there are a few things that appear broken (In Iris), mostly related to unavailable images.

1a. Artist images are never loaded when opening "Artists" directory (no caching occurs). When clicking directory "My Artist", Images are requested but never displayed. Instead an error is shown.

May 29 20:32:32 jlinde-ThinkPad-T430 mopidy[110780]: INFO     [TidalBackend-6] mopidy_tidal.library Browsing uri tidal:my_artists
May 29 20:32:32 jlinde-ThinkPad-T430 mopidy[110780]: INFO     [TidalBackend-6] mopidy_tidal.library Searching Tidal for images for ['tidal:artist:3567061', 
...
'tidal:artist:9810587', 'tidal:artist:15295', 'tidal:artist:4130', 'tidal:artist:3588012', 'tidal:artist:8875626', 'tidal:artist:13686']
May 29 20:32:33 jlinde-ThinkPad-T430 mopidy[110780]: ERROR    [Core-7] mopidy.core.library TidalBackend backend returned bad data: Expected a list of Image, not None

1b. Playlists images are never loaded when opening "Playlists" directory (no caching occurs). Opening "My Playlists" directory results in partial loading of album art. Only some of the first playlists has an image. The remaining ones are blank.

May 29 20:26:09 jlinde-ThinkPad-T430 mopidy[108654]: INFO     [TidalBackend-6] mopidy_tidal.library Browsing uri tidal:my_playlists
May 29 20:26:09 jlinde-ThinkPad-T430 mopidy[108654]: INFO     [TidalBackend-6] mopidy_tidal.library Searching Tidal for images for ['tidal:playlist:ae24e029-6024-4fc4-9c83-f77487a77bc7', 'tidal:playlist:9d80a1e2-2f5d-4825-af0e-aa7ab4dd1ea9', 'tidal:playlist:eeade4f0-7874-4867-9346-5f35bb281d18', 'tidal:playlist:ff963aeb-2e4b-4a6b-a5e2-8da10740902b', 'tidal:playlist:58b26365-afc9-413c-9437-04b58ec9748f', 'tidal:playlist:cd1d80e2-43c3-45c8-8f10-9a8c91b7777e', 'tidal:playlist:9698473c-af02-42ec-b6fb-e9da82565cf4', 'tidal:playlist:b4d9fc6f-36ee-408c-ab92-94012af35d84', 'tidal:playlist:7ab5d2b6-93fb-4181-a008-a1d18e2cebfa', 'tidal:playlist:48085a2b-62cb-42ba-9e03-cdb7a9a9d9ab', 'tidal:playlist:1a7d78db-6f0f-4bbd-8223-bdd8826f7d7c', 'tidal:playlist:8b5fa9ad-7310-48c7-b468-0f574390e39f', 'tidal:playlist:76083a45-f5fa-4723-953c-09674ce48fdf', 'tidal:playlist:f7841e40-d38a-4de2-90f6-2326bf5f164a', 'tidal:playlist:c38b1def-0944-442a-94cf-b7d86e8c8ab0', 'tidal:playlist:f31bc2f9-118e-4359-bf74-16593a6aabc8', 'tidal:playlist:de60da1f-8269-419b-a9fc-9a9f15dfecab', 'tidal:playlist:11fa1642-2394-47d5-ad3a-4da653a4907e', 'tidal:playlist:f4400845-fd1c-434d-8da1-1e818124a417', 'tidal:playlist:17df59b6-fb51-4aa9-a1ab-f43386f20859', 'tidal:playlist:f974f0a9-026a-4e6a-b061-f458ab4bc634', 'tidal:playlist:2c30f6c0-d3ce-458b-9a0d-d0569d115014', 'tidal:playlist:578a0303-c876-4f1b-afd7-eb1e7f4e12fb']
May 29 20:26:10 jlinde-ThinkPad-T430 mopidy[108654]: ERROR    [Core-7] mopidy.core.library TidalBackend backend returned bad data: Expected a list of Image, not None

This error Expected a list of Image, not None appears to occur when a user created playlist has no image. In general, most of the above are related to missing images not handled gracefully.

It appears we should return a "dummy" image and not a "None" type to mopidy, otherwise the above errors are shown whenever no image exists for a given track/playlist/album.

The issues with images not being requested when clicking "Album" and "Artist" directory on the left side of the web interface is probably an issue with Iris, as it also occurred before the changes.

2. Attempting to open "Playlists" (not My Playlists) directory, results in the following (VERY long log message), although playlists get displayed. Error is only shown immediately after mopidy restart. Otherwise, it can be replicated by clicking "refresh" in the Iris GUI. The error is Expected a list of Track, not [Playlist...

May 29 20:20:54 jlinde-ThinkPad-T430 mopidy[108654]: INFO     [TidalBackend-6] mopidy_tidal.library Lookup uris 'tidal:playlist:17df59b6-fb51-4aa9-a1ab-f43386f20859'
May 29 20:20:54 jlinde-ThinkPad-T430 mopidy[108654]: INFO     [TidalBackend-6] mopidy_tidal.library Returning 1 tracks
May 29 20:20:54 jlinde-ThinkPad-T430 mopidy[108654]: ERROR    [Core-7] mopidy.core.library TidalBackend backend returned bad data: Expected a list of Track, not [Playlist(last_modified=0, name='Series theme', tracks=[Track(album=Album(artists=[Artist(name='Game of Thrones Orchestra', uri='tidal:artist:4090193')], name='Game of Thrones (Main Opening Theme from The Series)', uri='tidal:album:46026085'), artists=[Artist(name='Game of Thrones Orchestra', uri='tidal:artist:4090193')], disc_no=1, length=105000, name='Game of Thrones (Main Theme of the TV Series)', track_no=1, uri='tidal:track:4090193:46026085:46026086'), ...

I could list the full error but it basically contains all my added playlists and their tracks :)

After selecting one of the playlists, everything works fine and images are displayed (for that particular playlist). The playlists get populated with the available tracks so that part works too.

4 Currently, we have not set the max resolution to use when requesting images. We should consider changing it, as mentioned above. Otherwise we request images/album art that is 1080x1080. It looks like iris does not like images that are too large(?).

5 Browsing "My Albums" result in the following error, followed by the caching itself.

May 29 20:51:33 jlinde-ThinkPad-T430 mopidy[111030]: INFO     [TidalBackend-6] mopidy_tidal.library Lookup uris 'tidal:my_albums'
May 29 20:51:33 jlinde-ThinkPad-T430 mopidy[111030]: ERROR    [TidalBackend-6] mopidy_tidal.library <class 'AttributeError'> when processing URI 'tidal:my_albums': 'TidalLibraryProvider' object has no attribute '_my_albums_cache'

6 Browsing My Playlists, result in the following error, followed by the caching itself.

May 29 20:25:15 jlinde-ThinkPad-T430 mopidy[108654]: INFO     [TidalBackend-6] mopidy_tidal.library Lookup uris 'tidal:my_playlists'
May 29 20:25:15 jlinde-ThinkPad-T430 mopidy[108654]: ERROR    [TidalBackend-6] mopidy_tidal.library <class 'AttributeError'> when processing URI 'tidal:my_playlists': 'TidalLibraryProvider' object has no attribute '_my_playlists_cache'

7 While refreshing "My Albums, certain albums cannot be displayed. Instead, an error is thrown:

May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]: INFO     [TidalBackend-6] mopidy_tidal.library Lookup uris 'tidal:album:110827651'
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]: INFO     [TidalBackend-6] mopidy_tidal.library Returning 12 tracks
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]: ERROR    [Core-7] mopidy.core.library TidalBackend backend caused an exception.
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]: Traceback (most recent call last):
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:   File "/usr/lib/python3/dist-packages/mopidy/core/library.py", line 17, in _backend_error_handling
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:     yield
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:   File "/usr/lib/python3/dist-packages/mopidy/core/library.py", line 217, in lookup
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:     result = future.get()
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:   File "/usr/lib/python3/dist-packages/pykka/_threading.py", line 45, in get
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:     _compat.reraise(*self._data['exc_info'])
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:   File "/usr/lib/python3/dist-packages/pykka/_compat/__init__.py", line 29, in reraise
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:     raise value
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:   File "/usr/lib/python3/dist-packages/pykka/_actor.py", line 193, in _actor_loop
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:     response = self._handle_receive(envelope.message)
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:   File "/usr/lib/python3/dist-packages/pykka/_actor.py", line 299, in _handle_receive
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:     return callee(*message.args, **message.kwargs)
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:   File "/usr/local/lib/python3.8/dist-packages/Mopidy_Tidal-0.2.8-py3.8.egg/mopidy_tidal/library.py", line 236, in lookup
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:     self._track_cache.update({track.uri:track for track in tracks})
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:   File "/usr/local/lib/python3.8/dist-packages/Mopidy_Tidal-0.2.8-py3.8.egg/mopidy_tidal/library.py", line 236, in <dictcomp>
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]:     self._track_cache.update({track.uri:track for track in tracks})
May 29 21:10:57 jlinde-ThinkPad-T430 mopidy[112636]: AttributeError: 'Track' object has no attribute 'uri'

The album in question is "The Black Keys, Lets Rock"

With all those bugs and comments out of the way, in general, the browsing experience is pretty good - after the first initial caching sequence. Great work and very nice to see the spaghetti code refactored! We should make sure to update the changelog accordingly (and give credit!). PR's like this are greatly(!) appreciated.

@fmarzocca Regarding release timeframe; I will do my best to get this PR merged into master as soon as we get the last issues ironed out. Then I will release an updated PyPi release

blacklight commented 2 years ago

@tehkillerbee can you try and remove your cache directory and try again? In the previous implementation some things were cached as mopidy objects and some (the lru_*_tracks objects) just as lists of tracks.

I've made things more consistent by always caching things as mopidy objects (after all, that's what we usually want to unpack anyway), but this means that cached items created previously break the current implementation.

tehkillerbee commented 2 years ago

@tehkillerbee can you try and remove your cache directory and try again? In the previous implementation some things were cached as mopidy objects and some (the lru_*_tracks objects) just as lists of tracks.

I've made things more consistent by always caching things as mopidy objects (after all, that's what we usually want to unpack anyway), but this means that cached items created previously break the current implementation.

@BlackLight I tried deleting the caching directory but it did not have a huge effect.

Certain folders (My Artists) still return an error and no album art. "My Playlists" only displays album art on some playlists, others appear uninitialized. EDIT: See the comments below for a solution to this issue.
Error is thrown whenever playlist directory is refreshed from Iris. This looks like a wrong return type used when browsing Playlists. TidalBackend backend returned bad data: Expected a list of Track, not [Playlist(last_modified=0 It looks like we are not returning result data corresponding to the data expected by Mopidy. It seems the refactoring of lookup() function in library broke something, as we can now return a non-track list type to Mopidy. EDIT: See the comments below for a solution to this issue.
Error "TidalLibraryProvider object has no attribute xx_cache" is still thrown when browsing the various directories (My Artists, My Albums, My Tracks). Not sure if this is intentional?

EDIT: I have added a couple of comments below that describe how we can fix the issues.

fmarzocca commented 2 years ago

Error is thrown whenever playlist directory is refreshed from Iris.

Not completely true. It is thrown whenever Mopidy receive a [core.playlists.refresh](https://docs.mopidy.com/en/latest/api/core/#mopidy.core.PlaylistsController.refresh) call.

blacklight commented 2 years ago

Am I right in saying that for every cache miss in lookup() you will write once to disk? Can you write them once in bulk at the end?

@kingosticks this is now addressed in https://github.com/tehkillerbee/mopidy-tidal/pull/60/commits/6416ed3c18355daa11c35b7833d35101a38ae416

blacklight commented 2 years ago

1a. Artist images are never loaded when opening "Artists" directory (no caching occurs). When clicking directory "My Artist", Images are requested but never displayed. Instead an error is shown.

1b. Playlists images are never loaded when opening "Playlists" directory (no caching occurs). Opening "My Playlists" directory results in partial loading of album art. Only some of the first playlists has an image. The remaining ones are blank.

These should both be fixed by https://github.com/tehkillerbee/mopidy-tidal/pull/60/commits/b92064caa62d4980185ccb277de31dc6822ba2b6

2. Attempting to open "Playlists" (not My Playlists) directory, results in the following (VERY long log message), although playlists get displayed. Error is only shown immediately after mopidy restart. Otherwise, it can be replicated by clicking "refresh" in the Iris GUI. The error is Expected a list of Track, not [Playlist...

This should be fixed by https://github.com/tehkillerbee/mopidy-tidal/pull/60/commits/2adfb83435a9e7196fef85891ca77efb3d84c749

4 Currently, we have not set the max resolution to use when requesting images. We should consider changing it, as mentioned above. Otherwise we request images/album art that is 1080x1080. It looks like iris does not like images that are too large(?).

This should be fixed by https://github.com/tehkillerbee/mopidy-tidal/pull/60/commits/063fd0ba7cc38a3ccb7e4fb40814ac5cabf1f210

5 Browsing "My Albums" result in the following error, followed by the caching itself.

6 Browsing My Playlists, result in the following error, followed by the caching itself.

This is actually expected, it's just the error logging trace that is misleading.

It happens because the extension builds URIs such as tidal:my_artists or tidal:my_albums. While browse provides a way to map those results to folders, lookup wants a list of tracks to be returned, so it's safe to just skip these entries. I have removed the error log trace from the file.

7 While refreshing "My Albums, certain albums cannot be displayed. Instead, an error is thrown:

I have tried to look up the same album (both through ncmpcpp and Iris) and I couldn't reproduce the error. I can't reproduce it when rendering My Albums either. Maybe it's a cache issue? If you have tried several versions of this PR it's likely that the cache folder may be screwed up :) if wiping the cache doesn't work can you try and replicate it when printing dir(track) and type(track) as well? The uri attribute should definitely be there on a Track instance. If it's missing on some tracks perhaps it's because is unavailable in the configured market?

fmarzocca commented 2 years ago

This should be fixed

Thank you. I'm going to update to latest commit and I will report any test.

fmarzocca commented 2 years ago

I have updated to the latest version of better-cache. These are the findings:

This is after a refresh (for every playlist):

Jun 10 10:50:00 pab mopidy[1056]: ERROR [Core-17] mopidy.core.library TidalBackend backend returned bad data: Expected a list of Track, not [Playlist(last_modified=0, name='I miei brani di Shazam', tracks=[Track(album=Album(artists=[Artist(name='C. Tangana', uri='tidal:artist:7995155')], name=

I don't understand this other error: Jun 10 10:50:22 pab mopidy[1056]: ERROR [TidalBackend-6] mopidy_tidal.library <class 'requests.exceptions.HTTPError'> when processing URI 'tidal:track:3579856:70841514:70841515': 404 Client Error: Not Found for url: https://api.tidalhifi.com/v1/albums/70841514/tracks?sessionId=e5947e8d-14f4-45b7-a77e-3c363cac6ccd&countryCode=IT&limit=999

blacklight commented 2 years ago

I have updated to the latest version of better-cache. These are the findings:

This is after a refresh (for every playlist):

Jun 10 10:50:00 pab mopidy[1056]: ERROR [Core-17] mopidy.core.library TidalBackend backend returned bad data: Expected a list of Track, not [Playlist(last_modified=0, name='I miei brani di Shazam', tracks=[Track(album=Album(artists=[Artist(name='C. Tangana', uri='tidal:artist:7995155')], name=

I don't understand this other error: Jun 10 10:50:22 pab mopidy[1056]: ERROR [TidalBackend-6] mopidy_tidal.library <class 'requests.exceptions.HTTPError'> when processing URI 'tidal:track:3579856:70841514:70841515': 404 Client Error: Not Found for url: https://api.tidalhifi.com/v1/albums/70841514/tracks?sessionId=e5947e8d-14f4-45b7-a77e-3c363cac6ccd&countryCode=IT&limit=999

The second error is either an album that isn't found in a market, or a maximum requests exceeded.

The first one should be fixed. Have you installed the latest version? If it still occurs, you may want to wipe the cache (older cache files may store playlists as list of tracks instead of objects).

kingosticks commented 2 years ago

I'm not saying we need this during development but what do you think about a mechanism to automatically clear the cache when the extension version changes?

fmarzocca commented 2 years ago

If it still occurs, you may want to wipe the cache (older cache files may store playlists as list of tracks instead of objects).

Yes, wiping the cache the error is gone. But wiping the cache is anyway a major PITA because I have to refresh EACH playlist, one by one. Sending a core.playlists.refresh doesn't do the job. Do you know a way to automatically refresh all playlists? I thought mopidy-tidal did this on server restart

blacklight commented 2 years ago

I'm not saying we need this during development but what do you think about a mechanism to automatically clear the cache when the extension version changes?

I don't expect changes in the cached formats to occur that often. While developing the feature and figuring things out the format can obviously change, but now that we've settled for pickle-ing native mopidy objects I don't expect many changes that would justify the effort of a version-based cache management logic.

But wiping the cache is anyway a major PITA because I have to refresh EACH playlist, one by one. Sending a core.playlists.refresh doesn't do the job. Do you know a way to automatically refresh all playlists? I thought mopidy-tidal did this on server restart

Manually wiping the cache should no longer be required - we've settled for storing playlists as native mopidy objects instead of lists of tracks, so there shouldn't be any more breaking changes.

When core.playlists.refresh is called, the playlist cache is hit if available, and it will be refreshed if the last_modified of the upstream object is greater than the cached one (e.g. if the playlist has changes). Otherwise, there should be no reason to force a cache refresh. Note that this works only if you're using my fork of python-tidal 0.6.x, because the standard version doesn't export the lastUpdated field.

Depending on the client implementation, however, a mopidy restart may be required in case of updates. Or at least that's the case for ncmpcpp, because the current playlist lookup logic doesn't call the upstream API to refresh the last modified date. I could fix this by forcing an API playlist lookup every time a playlist is looked up, but this could impact performance.

fmarzocca commented 2 years ago

Note that this works only if you're using my fork of python-tidal 0.6.x

Got it, thanks. So i have to install your python-tidal 0.6.x. Don't you think it should be set as a dependency on the final mopidy-tidal?

blacklight commented 2 years ago

Don't you think it should be set as a dependency on the final mopidy-tidal?

I still hope that I won't need to do that.

My PR is a temporary workaround while waiting (for the past two years) for the maintainer of python-tidal to finally push 0.7.x, which is supposed to be a total rewrite of the library.

The maintainer recently promised that 0.7.x should be coming up soon, and that's why I'm holding on that PR: it may soon become irrelevant, and since 0.7.x may also require the rewrite of some parts of mopidy-tidal, I don't want to become the de facto maintainer of the stale 0.6.x branch in the meantime.

However, if this PR gets approved before the new python-tidal comes out, it's probably worth to add at least a couple of lines to the documentation about the temporary workaround.

fmarzocca commented 2 years ago

I still hope that I won't need to do that.

I fully understand and agree with you. A friend of mine is used to say: "We are on this Earth to suffer". (maybe too much Calvinistic!)

fmarzocca commented 2 years ago

Yes. That's what I meant: caching the url, not the image data.

Sent from my mobile phone. Sorry for typos -

Il Dom 29 Mag 2022, 19:04 Johannes L @.***> ha scritto:

IMHO there is no sense to cache the image data, if ever the images url...

@fmarzocca https://github.com/fmarzocca It appears to be faster to use the cached image URL instead of requesting it anew. So it will provide a better user experience and images will be displayed faster.

— Reply to this email directly, view it on GitHub https://github.com/tehkillerbee/mopidy-tidal/pull/60#issuecomment-1140487762, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACBCQ3XHBKBAQG3ZG5Z2HDLVMOPRNANCNFSM5WTJERFA . You are receiving this because you were mentioned.Message ID: @.***>

tehkillerbee / mopidy-tidal

Persist caches to disk to avoid slow initialization upon restart #60