Web API playlists v2 #228
Closed
Web API playlists v2 #228
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cache playlist web API responses in a simple dict. playlists: Support Spotify's new playlist URI scheme (Fixes mopidy#215). search: uses 'from_token' market.
Spotify Track relinking guide says must use the original uri for any playlist manipulation etc so we should return that when translating to Mopidy models. libspotfy will handle any relinking when track is loaded for playback. Re-use track ref logic to extract correct URI. Added valid_web_data helper for checking common web object fields.
Block attemps to get user's playlists until first refresh completes. Temporarily increased timing logging verbosity. Using this as a baseline for optimisations where we: 1. Start Mopidy 2. Get the playlists (mpc lsplaylists) 3. Load a playlist of 454 songs (mpc load X). 1. ~7 seconds INFO 2019-09-12 23:00:21,985 Logged into Spotify Web API as kingosticks INFO 2019-09-12 23:00:22,346 playlists.as_list() took 360ms INFO 2019-09-12 23:00:28,830 Refreshed 66 playlists INFO 2019-09-12 23:00:28,830 Refresh Playlists took 6845ms INFO 2019-09-12 23:00:28,831 Starting Mopidy core 2. real 0m0.539s INFO 2019-09-12 23:01:03,461 playlists.as_list() took 270ms INFO 2019-09-12 23:01:03,641 playlists.as_list() took 176ms 2. real 0m0.202s INFO 2019-09-12 23:01:16,267 playlists.as_list() took 174ms 3. real 0m8.926s 3. real 0m0.410s 1. Cold cache. 2. Cache expired. 3. because libspotify has to load the all the track data.
Even once we have all playlist data from the Web API, any *track* lookups are done through libspotify which do a full load of the data. Clients may try to lookup all tracks in a playlist and this is therefore now very slow for large playlists. Previously, libspotify would start loading playlist track data in the background on first access to the playlist_container; this meant that any subsequent track lookups we did were fast. libspotify will do this background loading for any object once it has a Link to it so we can recreate this optimisation by simply grabbing libspotfy Link objects for all playlist track URIs we got from the Web API. We do need to maintain a reference to all these libspotify Links else they'll be freed and the track data will not be loaded. 1. ~7 seconds INFO 2019-09-12 23:07:28,755 Logged into Spotify Web API as kingosticks INFO 2019-09-12 23:07:35,521 Refreshed 66 playlists INFO 2019-09-12 23:07:35,521 Refresh Playlists took 6765ms INFO 2019-09-12 23:07:35,521 Starting Mopidy core 2. real 0m0.356s INFO 2019-09-12 23:07:55,680 playlists.as_list() took 143ms INFO 2019-09-12 23:07:55,830 playlists.as_list() took 147ms 2. real 0m0.167s INFO 2019-09-12 23:08:06,595 playlists.as_list() took 145ms 3. real 0m0.467s 3. real 0m0.399s 1. Unchanged 2. Unchanged 3. Big speedup for first call
Workaround for Spotify's use of max-age=0 for the me/playlists endpoint. 1. ~7 seconds INFO 2019-09-12 23:09:16,752 Logged into Spotify Web API as kingosticks INFO 2019-09-12 23:09:23,377 Refreshed 66 playlists INFO 2019-09-12 23:09:23,377 Refresh Playlists took 6625ms INFO 2019-09-12 23:09:23,377 Starting Mopidy core 2. real 0m0.194s INFO 2019-09-12 23:09:52,968 playlists.as_list() took 145ms INFO 2019-09-12 23:09:52,974 playlists.as_list() took 4ms 2. real 0m0.033s INFO 2019-09-12 23:09:54,108 playlists.as_list() took 13ms 3. real 0m0.353s 3. real 0m0.119s 1. Unchanged 2. Speedup for subsequent call 3. As above
|
❤️ |
|
Hi, I've tested your changes yesterday. You've done good job there @kingosticks! Do you need help with last fixes? I'd love to help merge it asap just tell me where are bugs :) |
|
To simplify a bit, I removed the For this PR, that shouldn't be a big problem, as I know @kingosticks intended to rebase this PR on the now Python 3 compatible |
|
If I re-write this code enough times, chances are I'll get it right at some point. Maybe. |
|
@kingosticks, tell me if you need more hands on board ;) |
|
I'll port the branch tonight, after which you'll need the latest (and very greatest) pre-release version of Mopidy 3 in order to test it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch is the successor to #188 and aimed at fixing #140 and #182.
Save cached data to file on exit and restore on login- not in this PRI'm not happy with returning the raw (mutable) cache data dict to
SpotifyOAuthClientand this led to the hacky workaround at https://github.com/kingosticks/mopidy-spotify/blob/fix/web-api-playlists-v2/mopidy_spotify/web.py#L440-L443. Returning something immutable like anamedtuplemight be better.