Web API playlists v3 #235

kingosticks · 2019-12-03T00:15:38Z

Third time lucky here. This is the Python 3 version of #228 aimed at fixing #140 and #182. Everything discussed in the previous PR is probably still valid for this version.

Cache playlist API response data
Support Spotify's new playlist URI scheme
~~Save cached data to file on exit and restore on login~~ - not in this PR
Translator should use @memoized decorator?
Track translators need to consider market availability
Tests
Handle playlists refresh
Include performance info

I have redone the performance measurements that I included in the commits for #228 which helped explained the code's evolution.

I'm still not happy with returning the raw (mutable) cache data dict to SpotifyOAuthClient and this led to the hacky workaround:

mopidy-spotify/mopidy_spotify/web.py

Lines 462 to 465 in 681debe

    
           # Take a copy to avoid changing the cached response. 
        
           playlist = copy.deepcopy(playlist) 
        
           playlist.setdefault("tracks", {}).setdefault("items", []) 
        
           playlist["tracks"]["items"] += more_tracks

. Returning something immutable like a namedtuple might be better or something Python 3-ish (dataclass?).

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.

codecov · 2019-12-03T00:17:54Z

Codecov Report

Merging #235 into master will increase coverage by 0.23%.
The diff coverage is 98.26%.

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
+ Coverage   97.13%   97.37%   +0.23%     
==========================================
  Files          13       13              
  Lines         979     1105     +126     
==========================================
+ Hits          951     1076     +125     
- Misses         28       29       +1

Impacted Files	Coverage Δ
mopidy_spotify/distinct.py	`95.08% <ø> (ø)`	⬆️
mopidy_spotify/utils.py	`100% <ø> (ø)`	⬆️
mopidy_spotify/backend.py	`98.94% <100%> (-0.02%)`	⬇️
mopidy_spotify/browse.py	`93.93% <100%> (ø)`	⬆️
mopidy_spotify/library.py	`100% <100%> (ø)`	⬆️
mopidy_spotify/search.py	`94.87% <100%> (ø)`	⬆️
mopidy_spotify/translator.py	`98.76% <100%> (-0.54%)`	⬇️
mopidy_spotify/playback.py	`100% <100%> (ø)`	⬆️
mopidy_spotify/lookup.py	`100% <100%> (ø)`	⬆️
mopidy_spotify/playlists.py	`94.52% <95.74%> (+0.23%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36bf330...0a78635. Read the comment docs.

kingosticks · 2019-12-03T23:51:44Z

I redid the performance numbers and added the to them commit messages.

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 1500 songs (mpc load X). 1. ~7.5 seconds INFO 2019-12-03 23:38:17,800 Logged into Spotify Web API as kingosticks4 INFO 2019-12-03 23:38:25,464 Refreshed 68 playlists INFO 2019-12-03 23:38:25,464 Refresh Playlists took 7663ms INFO 2019-12-03 23:38:25,465 Starting Mopidy core 2. real 0m0.403s INFO 2019-12-03 23:38:30,612 playlists.as_list() took 181ms INFO 2019-12-03 23:38:30,781 playlists.as_list() took 167ms 2. real 0m0.235s INFO 2019-12-03 23:38:32,632 playlists.as_list() took 214ms 3. MPD error: Timeout 3. real 0m3.131s

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.7 seconds INFO 2019-12-03 23:44:41,470 Logged into Spotify Web API as kingosticks4 INFO 2019-12-03 23:44:49,247 Refreshed 68 playlists INFO 2019-12-03 23:44:49,247 Refresh Playlists took 7777ms INFO 2019-12-03 23:44:49,248 Starting Mopidy core 2. real 0m0.412s INFO 2019-12-03 23:44:52,197 playlists.as_list() took 162ms INFO 2019-12-03 23:44:52,392 playlists.as_list() took 192ms 2. real 0m0.167s INFO 2019-12-03 23:44:53,447 playlists.as_list() took 146ms 3. real 0m1.859s 3. real 0m1.535s

Workaround for Spotify's use of max-age=0 for the me/playlists endpoint. 1. ~7.7 seconds INFO 2019-12-03 23:47:16,030 Logged into Spotify Web API as kingosticks4 INFO 2019-12-03 23:47:23,659 Refreshed 68 playlists INFO 2019-12-03 23:47:23,659 Refresh Playlists took 7628ms INFO 2019-12-03 23:47:23,659 Starting Mopidy core 2. real 0m0.057s INFO 2019-12-03 23:47:26,070 playlists.as_list() took 1ms INFO 2019-12-03 23:47:26,073 playlists.as_list() took 1ms 2. real 0m0.223s INFO 2019-12-03 23:47:27,668 playlists.as_list() took 212ms 3. real 0m0.837s 3. real 0m0.470s

mopidy_spotify/playlists.py

kingosticks · 2019-12-07T17:40:59Z

I've no idea what the "manual method" is. We only support one method as far as I know, and its the one outlined in this project readme. If you run Mopidy with verbose logging that would probably be useful.

Those attribute errors are a different issue, they have a bug on here already. They won't be the problem here.

I also suggest you check that mopidy config and mopidy deps agree with what you think is going on.

girst · 2019-12-07T18:40:42Z

ok, i've redone everything in a fresh VM (fc30), and now it works. sorry for the noise.

code itself looks good. i personally don't see anything wrong with the deepcopy 'hack' and would just leave it.

jodal

This looks very good and works very well! :-)

mopidy_spotify/web.py

mopidy_spotify/lookup.py

mopidy_spotify/playlists.py

mopidy_spotify/translator.py

mopidy_spotify/playlists.py

kingosticks · 2019-12-11T00:16:02Z

I did find a bug where the first page of a web object won't update the etag. That bug wasn't introduced in this PR so it's not a blocker to merging this now.

jodal

Merge at will. My only remaining question is whether to reduce the log level on the timings from INFO to DEBUG.

mopidy_spotify/playlists.py

jodal · 2019-12-11T23:06:23Z

And a changelog entry!

kingosticks · 2019-12-11T23:12:58Z

Yes to reducing the log levels. I'll also change get_one to use trace logging.

kingosticks · 2019-12-11T23:29:32Z

Done and done.

jodal · 2019-12-12T07:47:05Z

🎉 🚀

blacklight · 2019-12-12T11:04:25Z

Is Python 2 still supported? I've tried to run the installation after merging my changes for playlist modifications, and I've got setup.py failing on Python 3 format strings:

SyntaxError: invalid syntax
    logger.info(f'Failed to browse "{uri}": Toplist URI parsing failed')

If Python 2 is no longer supported does it mean that the master for this extension (and for mopidy itself and for most of its extensions) is now supposed to be run only on Python 3?

Don't get me wrong, it's about time that mopidy finally moves to Python 3, but there's tons of extensions out there that also need to be migrated :)

kingosticks · 2019-12-12T11:37:56Z

Is Python 2 still supported?

The next planned release of Mopidy-Spotify is v4.0.0 and it will require Mopidy v3.0.0+. Python 2 will not be supported in these releases.

does it mean that the master for this extension (and for mopidy itself and for most of its extensions) is now supposed to be run only on Python 3?

Mopidy's master branch still reflects the latest release and thus supports Python 2 only. Mopidy's develop branch contains v3.x work and supports Python 3 only. Extensions (at least those living under mopidy-org) now only have a master branch and soon they will all support Python 3 only. We decided that having a develop branch alongside master isn't worth the extra complication (particuarly during releases) because for extensions they are almost identical most of the time. We are currently in an unsual period of enormous change so master has diverged signicatly from what was last released - hopefully it won't last much longer.

but there's tons of extensions out there that also need to be migrated :)

We are very aware! And we've been working on that: https://github.com/orgs/mopidy/projects/2

If you have more playlist enhancements that can go into Mopidy v4 we'd love to have them.

jodal · 2019-12-12T12:03:49Z

We've also built out a new extension registry to make it easy for end-users to find extensions that will work with Python 3: https://mopidy.com/ext/

What's lacking is outreach to the long tail of extension developers. I think that should be our first focus after the Mopidy 3.0 release, which is targeted at Dec 21.

kingosticks added 3 commits November 20, 2019 23:15

Spotify specific OAuthClient to hold user profile and login.

4eef4d0

playlists: Use the Web API (Fixes mopidy#122, mopidy#182).

4aafc68

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.

kingosticks added the A-webapi Area: Spotify Web API label Dec 3, 2019

kingosticks added this to the v4.0 milestone Dec 3, 2019

kingosticks self-assigned this Dec 3, 2019

kingosticks changed the title ~~Fix/web api playlists v3~~ Web API playlists v3 Dec 3, 2019

kingosticks requested a review from jodal December 3, 2019 00:16

kingosticks force-pushed the fix/web-api-playlists-v3 branch from 681debe to e9b83a0 Compare December 3, 2019 23:50

kingosticks added 4 commits December 3, 2019 23:54

Move cache data into SpotifyOAuthClient.

ec9d914

kingosticks force-pushed the fix/web-api-playlists-v3 branch from e9b83a0 to 6dd39cf Compare December 3, 2019 23:55

Fix import order

02cf0e1

kingosticks commented Dec 4, 2019

View reviewed changes

mopidy_spotify/playlists.py Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jodal approved these changes Dec 8, 2019

View reviewed changes

jodal marked this pull request as ready for review December 9, 2019 23:17

kingosticks added 4 commits December 10, 2019 21:09

Run pyupgrade --py37-plus

501dc5b

More consistent use of repr formatting.

9282db8

Use dataclass and Enum for WebLink

44e94fe

Remove on_playlists_loaded

40722f6

Force kwargs for SpotifyOAuthClient constructor.

87308b8

jodal approved these changes Dec 11, 2019

View reviewed changes

mopidy_spotify/playlists.py Outdated Show resolved Hide resolved

mopidy_spotify/playlists.py Outdated Show resolved Hide resolved

mopidy_spotify/playlists.py Outdated Show resolved Hide resolved

kingosticks added 2 commits December 11, 2019 23:19

Reduce log verbosity

6fc61b3

Update changelog and readme.

0a78635

jodal merged commit f52babd into mopidy:master Dec 12, 2019

jodal mentioned this pull request Dec 12, 2019

Playlists stopped working #182

Closed

kingosticks deleted the fix/web-api-playlists-v3 branch December 12, 2019 11:38

lovethisshit mentioned this pull request Mar 7, 2020

Play any Playlist from spotify MiczFlor/RPi-Jukebox-RFID#834

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web API playlists v3 #235

Web API playlists v3 #235

kingosticks commented Dec 3, 2019 •

edited

Loading

codecov bot commented Dec 3, 2019 •

edited

Loading

kingosticks commented Dec 3, 2019 •

edited

Loading

This comment has been minimized.

kingosticks commented Dec 7, 2019

This comment has been minimized.

girst commented Dec 7, 2019

jodal left a comment

kingosticks commented Dec 11, 2019

jodal left a comment

jodal commented Dec 11, 2019

kingosticks commented Dec 11, 2019

kingosticks commented Dec 11, 2019

jodal commented Dec 12, 2019

blacklight commented Dec 12, 2019

kingosticks commented Dec 12, 2019

jodal commented Dec 12, 2019

	# Take a copy to avoid changing the cached response.
	playlist = copy.deepcopy(playlist)
	playlist.setdefault("tracks", {}).setdefault("items", [])
	playlist["tracks"]["items"] += more_tracks

Web API playlists v3 #235

Web API playlists v3 #235

Conversation

kingosticks commented Dec 3, 2019 • edited Loading

codecov bot commented Dec 3, 2019 • edited Loading

Codecov Report

kingosticks commented Dec 3, 2019 • edited Loading

This comment has been minimized.

kingosticks commented Dec 7, 2019

This comment has been minimized.

girst commented Dec 7, 2019

jodal left a comment

Choose a reason for hiding this comment

kingosticks commented Dec 11, 2019

jodal left a comment

Choose a reason for hiding this comment

jodal commented Dec 11, 2019

kingosticks commented Dec 11, 2019

kingosticks commented Dec 11, 2019

jodal commented Dec 12, 2019

blacklight commented Dec 12, 2019

kingosticks commented Dec 12, 2019

jodal commented Dec 12, 2019

kingosticks commented Dec 3, 2019 •

edited

Loading

codecov bot commented Dec 3, 2019 •

edited

Loading

kingosticks commented Dec 3, 2019 •

edited

Loading