Support for playlist management (creation and tracks add/rm/swap). #236
Conversation
Codecov Report
@@ Coverage Diff @@
## master #236 +/- ##
==========================================
- Coverage 96.56% 91.37% -5.19%
==========================================
Files 13 13
Lines 1136 1183 +47
==========================================
- Hits 1097 1081 -16
- Misses 39 102 +63
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I've not finished reviewing this but I think we need to decide what we want to do before we go any further. The duplicate track issue is a problem, can we have wildly different behaviour between this and the m3u backend? You don't seem to have handled playlist renaming and there's also restrictions on the number of tracks you can submit to some endpoints which also isn't handled.
Is it possible to reimplement save using their PUT method to simply replace the entire playlist? You'd still have to batch it up in chunks of 100 tracks. Presumably there's no problem with adding new tracks this way. Maybe we can have a simpler algorithm to see if the new playlist is a very simple track addition or subtraction compared to the original and in that case avoid doing an entire replacement.
There was a problem hiding this comment.
I've not finished reviewing this but I think we need to decide what we want to do before we go any further. The duplicate track issue is a problem, can we have wildly different behaviour between this and the m3u backend? You don't seem to have handled playlist renaming and there's also restrictions on the number of tracks you can submit to some endpoints which also isn't handled.
Is it possible to reimplement
saveusing theirPUTmethod to simply replace the entire playlist? You'd still have to batch it up in chunks of 100 tracks. Presumably there's no problem with adding new tracks this way. Maybe we can have a simpler algorithm to see if the new playlist is a very simple track addition or subtraction compared to the original and in that case avoid doing an entire replacement.
I thought of it but I didn't like that approach much - what if something goes wrong in the middle of the tracks replacement and you end up with half the songs on a playlist removed?
Maybe we could create a temporary auxiliary playlist that will store the original content before performing modifications on the original playlist, and restore the tracks from there if something goes wrong, but while that would make things more robust it'll also add quite some overhead in the case of simple interactions.
I believe however that 99% of the cases supported by most of the mpd clients will be simple additions and removals, not hybrid add/remove/swap operations. Maybe something can indeed optimized by taking that into account.
How does mopidy deal with playlist changes in the m3u backend? Does it replace the whole playlist every time?
|
That same argument can be applied to what we have here. If something goes wrong in the remove step or the add step then you've got a mess left behind. And you still need to batch these requests up into chunks of 100 tracks. If the playlist is 100 tracks or less its one replace call and done, regardless of the operations performed. You've already got a local copy of the original playlist so if something goes majorly wrong you could then restore the original from that. The m3u backend replaces the entire playlist. It's by far the simplest and most robust approach, I do appreciate it's not as easy here. |
It also addresses the comments in mopidy#236
The difference is that with the current approach the mess only impacts the newly added/removed/swapped tracks, not the entire playlist.
True, but most of my playlists, for instance, have more than 100 tracks. I also have playlists with >1000 tracks and one with 1500 tracks. In the latter case adding a single track would result in 17 API calls (one for clearing the playlist, 15 to add tracks in chunks of 100 each, and one more to refresh the playlist), and if anything goes wrong in the process the revert will also cost 17 calls. The latency and the risk of hitting the web API rate limits will both be high in this case. I had even thought of sacrificing the performance gain introduced by the URI-based indexing to be more consistent when it comes to duplicate tracks, but an inner loop to check the changes in the two playlists position-by-position would mean an Eventually, I chose the current approach because it seemed by far the most efficient, and maybe we could live with the inconsistent behaviour when it comes to duplicate tracks, as long as it's properly documented. And I think we are kind of consistent anyway. The current implementation won't allow addition of duplicate tracks either, and the Spotify apps also show a confirmation message to prevent duplicates from being added.
A rollback |
I agree that's not acceptable and what's why we'd need to detect these simpler add/remove cases and do something more sensible. Which, in this example, would be a single replace request at the playlist end and a refresh (which I think is always going to be unavoidable so we can ignore it). Now consider adding new tracks sparsely, in the very worst case consider adding a track after every other track. For your 1500 track playlist that's 750 API requests. Less contrived versions are possible in real-world usuage. I appreciate it might not be how you edit your playlists but we need to consider the broader cases, up to a realistic point. And for the record, I don't think you can clear a large playlist with a single API call as you are still restricted by the 100 track limit when removing. But you don't need to clear it anyway so that's moot. Ultimately I think we need to think about this more. And we definitely need to have it soak before including it in a release. I'm going to concentrate on our Mopidy 3 release (including merging parts of Mopidy-Spotify-Web into Mopidy-Spotify) as that's comming up next week. Once that's out of the way I, and hopefully also others, will come back to this. |
kingosticks
added
C-enhancement
Which clients support such a way of inserting tracks in a playlist? I agree that we have to consider the broadest use cases, but none of the MPD/mopidy clients I know of support things like sparse add/remove/swap on the same playlist within the same action. I'm not even sure if any of the clients out there support add/remove/swap within the same atomic action, so anyway the I agree however that we must find a trade-off point between handling only the delta and clearing and re-populating the whole playlist. As a thumb rule the logic should minimize the number of API calls. It means that if the number of computed non-contiguous addition/removals grouped in batches of 100 is greater than the size of the playlist divided by 100 then we should clear and re-populate the playlist, otherwise do an incremental addition/removal - what do you think?
Actually the Spotify Web API provides the |
There's no reason you can't add tracks at whatever position you want to. I've no idea what MPD clients support that as I don't use MPD clients day to day. I had intended musicbox-webclient to allow you to manipulate playlists however you want and save all the modifications in one go.
If you don't sppecify
If you do specify |
Got it - that's more a kind of edit-and-commit approach as opposed to the edit-on-the-fly approach implemented by most of the clients I use (ncmpcpp, Iris and MPDroid). However my approach will also work in these cases - it might be slower if compared to a clear-and-repopulate approach, but if we assume that in most of the cases the user adds and removes a couple of tracks instead of restructuring the entire playlist (is it a correct assumption or am I biased by my own use cases?) it may still be an acceptable trade-off.
You're right, I missed the required part on the If that's the case then it's one more argument against the clear-and-repopulate approach: in case of a large playlist the number of calls will really be cumbersome. What do you think about the trade-off I've proposed in my previous comment? expected_api_calls_delta = [int(len(block)/100 + 1) for block in contiguous_updates]
expected_api_calls_repopulate = 2 * int(len(playlist.tracks)/100 + 1)
if expected_api_calls_delta < expected_api_calls_repopulate:
do_delta_update()
else:
do_clear_and_repopulate() |
It's easily done, their documentation is poor. They don't even mention the Sorry, but I need to come back to this after the 3.x release and get into it properly. |
|
Any chance to resume the discussion about this topic and/or get more developers involved on what's the best strategy to tackle it? |
|
I, for one, can look again next week. |
|
(Following this with interest as it's funcionality I desperately want) As far as an MPD client is concerned, the only things it can do with a stored playlist are: Each of these is one operation, however MPD has the concept of command lists, where multiple operations can be batched into one atomic operation, so it's perfectly possible for a client to do any combination of the above in one go. 50 commands is the default maximum length of a command list; so 49 adds and one move would be the sort of thing you'd expect to have to handle. If a user tires to add more than 50 tracks, this would result in 50 adds, a write, 50 more adds, and so on. As far as a client goes, responsiveness is important. As far as the user of a client is concerned, it's everything. 17 API calls for a simple playlist operation is far too slow. Some clients will freeze while they wait for a response, most users will get impatient. In the early days of Rompr it was quite slow at retrieving the play queue, and people complaied because it took about 3 seconds to populate when there were more than 5000 tracks in the queue. I thought that was a ridiculous amout of tracks and therefore hadn't optimised any of the code, but it actually seems to be not unusual. People like big playlists. In my view, anything that can be done to make the process as efficient as possible should be done, or people will moan :) |
|
@fatg3erman I totally agree, I also feel that minimizing the number of API calls, by treating as many changes as possible as deltas compared to the current state, should be favoured over the wipe-and-populate approach. And responsiveness should be everything, even if the cost is a loss of consistency in some corner cases like e.g. duplicate IDs within the same playlist in order to implement a hash lookup instead of a linear lookup. However, I also acknowledge that in some corner cases the wipe-and-populate approach may be more efficient. If I want to remove all the tracks in a playlist and add a single new track then it's faster to do a wipe-all instead of a scan-the-deltas (provided that the client actually supports such a feature within a single atomic call). So my questions are:
Once settled, we've still got the rollback issue to tackle. I believe that if some calls inside |
|
|
||
| def delete(self, uri): | ||
| pass # TODO | ||
| # Playlist deletion is not implemented in the web API, see |
There was a problem hiding this comment.
there is an enpoint for it, but they called it 'unfollow': https://developer.spotify.com/documentation/web-api/reference/follow/unfollow-playlist/. their rationale here
It also addresses the comments in mopidy#236
It also addresses the comments in mopidy#236
|
@blacklight: What's the state of this PR? As far as I can see from the comments, minimizing API requests is still to-do? I'm not sure if you and @kingosticks have agreed a plan for this yet or not. If you aren't interested in working on this anymore, I can pick it up, given a list of outstanding tasks. I've worked a bit on that code (fix some errors, make tests pass, implement deletion, less cache invalidation) at https://github.com/fork-graveyard/mopidy-spotify/tree/bug22, fyi. ETA: i had a play around with ncmpcpp and cantata; both seem to add/move tracks to playlists one-by-one. this means, that for many clients, the api request optimisation might be a moot point. ETA2: one more thing I forgot to mention is that the current api scope is too narrow: we're missing |
|
@girst I believe that we eventually want a reasonable trade-off depending on the case - if the user is adding a single track to a playlist then updating the whole playlist is overkill; if the user is clearing all the tracks in a playlist then doing track-by-track updates is overkill. I think that we just need to agree on where to draw the dividing line for the cases between these two extremes - the code should probably try and estimate the average-case number of required API calls in both the cases, given the size of the playlist and the number of requested changes, and apply the logic that minimizes such number. However, for large playlists the estimation logic itself can be overkill - the mpd I don't have much time to work on this PR in this period (I've recently expanded the family and babies seem to require much more maintenance than code), but feel free to pick it up if you have some time. |
|
regarding the algorithm, i propose the following:
Given the lists of new tracks (playlist.tracks) and old tracks
(saved_playlist.tracks), calculate the diff between them (probably using
something based on LCS). This gives us a list of block-additions and
block-deletions.
In a second step, try to pair an addition-block with a deletion-block;
this is a block move. finally, split ranges > 100 tracks into chunks.
now that we have a set of instructions on how to transform the playlist,
we can check if replacing it outright (in chunks of 100) is cheaper. the
spotify api has an endpoint for "clear-and-put-100-tracks", so
clear_and_repopulate needs exactly ceil(len(playlist.tracks)/100)
operations.
as pseudocode:
operations = diff(saved_playlist.tracks, playlist.tracks)
# operations := { {type:add, tracks:[...]}, {type:del, tracks:[...] }
additions = {op for op in operations if op.type == 'add'}
deletions = {op for op in operations if op.type == 'del'}
moveblock = addition & deletions
patchall = (additions ^ deletions) | moveblock
replaceall = math.ceil(len(playlist.tracks) / 100)
if replaceall < len(adddelmov):
clear_and_repopulate(playlist.tracks)
else:
patch_playlist(adddelmov)
# finally, check if playlist was renamed
i have noticed that both mpd clients i tested (ncmpcpp and cantata) will
not use 'command lists'. assuming this is common practise (i belive so),
the diffing algorithm will hardly ever do more than insert or remove a
single track.
hence, as a further optimisation, we could delay 'writing' the playlist
to the API by a few seconds to wait for more save() commands. that will
require storing the playlist in memory temporarily, and only when no new
save commands came in for e.g. 5 seconds, write to the API
asynchronously.
On Tue, Nov 03, 2020 at 03:46:05PM -0800, Fabio Manganiello wrote:
(I've recently expanded the family and babies seem to require much more
maintenance than code)
congrats!
|
|
Command lists are very commonly used. You chose two of the most basic clients for your test.
… On 4 Nov 2020, at 17:12, girst ***@***.***> wrote:
regarding the algorithm, i propose the following:
Given the lists of new tracks (playlist.tracks) and old tracks
(saved_playlist.tracks), calculate the diff between them (probably using
something based on LCS). This gives us a list of block-additions and
block-deletions.
In a second step, try to pair an addition-block with a deletion-block;
this is a block move. finally, split ranges > 100 tracks into chunks.
now that we have a set of instructions on how to transform the playlist,
we can check if replacing it outright (in chunks of 100) is cheaper. the
spotify api has an endpoint for "clear-and-put-100-tracks", so
clear_and_repopulate needs exactly ceil(len(playlist.tracks)/100)
operations.
as pseudocode:
operations = diff(saved_playlist.tracks, playlist.tracks)
# operations := { {type:add, tracks:[...]}, {type:del, tracks:[...] }
additions = {op for op in operations if op.type == 'add'}
deletions = {op for op in operations if op.type == 'del'}
moveblock = addition & deletions
patchall = (additions ^ deletions) | moveblock
replaceall = math.ceil(len(playlist.tracks) / 100)
if replaceall < len(adddelmov):
clear_and_repopulate(playlist.tracks)
else:
patch_playlist(adddelmov)
# finally, check if playlist was renamed
i have noticed that both mpd clients i tested (ncmpcpp and cantata) will
not use 'command lists'. assuming this is common practise (i belive so),
the diffing algorithm will hardly ever do more than insert or remove a
single track.
hence, as a further optimisation, we could delay 'writing' the playlist
to the API by a few seconds to wait for more save() commands. that will
require storing the playlist in memory temporarily, and only when no new
save commands came in for e.g. 5 seconds, write to the API
asynchronously.
On Tue, Nov 03, 2020 at 03:46:05PM -0800, Fabio Manganiello wrote:
>(I've recently expanded the family and babies seem to require much more
>maintenance than code)
congrats!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
On Wed, Nov 04, 2020 at 09:48:14AM -0800, Mark Greenwood wrote:
Command lists are very commonly used. You chose two of the most basic clients for your test.
point me to some then, please.
|
|
There's this one, for a start https://fatg3erman.github.io/RompR/ this will, if required, use command lists of up to 50 commands (which is the default setting of MPD) |
|
On Wed, Nov 04, 2020 at 10:03:35AM -0800, Mark Greenwood wrote:
There's this one, for a start
https://fatg3erman.github.io/RompR/
thanks! i'll use it to test the proposed algorithm once implemented.
this will, if required, use command lists of up to 50 commands (which
is the default setting of MPD)
what are you using command lists for? just adding/deleting multiple
tracks, or fully mixing different commands (e.g. add 3 tracks here, 2
there and remove that one)?
|
|
I think we covered not assuming one-track-at-a-time changes previously. Regardless of what some MPD clients do (I'm not sure if the current Mopidy-MPD implementation has anything to prevent each stored playlist command in your list performing a
Sounds good to me. I'd really like to avoid making this crazy complicated. We do have to test this. And a belated congratulations to you @blacklight! |
|
Basically I build a list of commands and if it’s longer than 1 I use a command list. But for playlists it would be add a group of tracks to the end and then move them to a position (because MPD doesn’t support adding tracks at a position). Adding 2 here 3 there isn’t possible in one operation through the UI so I wouldn’t do that.
… On 4 Nov 2020, at 18:10, girst ***@***.***> wrote:
On Wed, Nov 04, 2020 at 10:03:35AM -0800, Mark Greenwood wrote:
>There's this one, for a start
>
>https://fatg3erman.github.io/RompR/
thanks! i'll use it to test the proposed algorithm once implemented.
>this will, if required, use command lists of up to 50 commands (which
>is the default setting of MPD)
what are you using command lists for? just adding/deleting multiple
tracks, or fully mixing different commands (e.g. add 3 tracks here, 2
there and remove that one)?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
On Wed, Nov 04, 2020 at 10:27:38AM -0800, Nick Steel wrote:
Regardless of what some MPD clients do [...], web clients and
potentially other frontends allow you to make multiple changes at a
time so we need to support that fully.
of course. i mentioned the single-track changes only to propose a
further optimisation of bundling multiple save()s together.
…> I think that we just need to agree on where to draw the dividing line
> for the cases between these two extremes
Sounds good to me. I'd really like to avoid making this crazy
complicated. We do have to test this.
|
|
On Wed, Nov 04, 2020 at 10:35:01AM -0800, Mark Greenwood wrote:
But for playlists it would be add a group of tracks to the end and then
move them to a position
that sounds reasonable and is probably a good fit for my proposed
diff-based algorithm. mopidy save() will see one large(ish) insertion of
x tracks at offset y, and that will map nicely to one of the available
endpoints on spotify's api.
|
|
On Wed, Nov 04, 2020 at 09:48:14AM -0800, Mark Greenwood wrote:
Command lists are very commonly used. You chose two of the most basic clients for your test.
i tried rompr, and it too caused seperate requests for each track. so i
started digging around in mopidy-mpd's source.
turns out, my mpd clients were not the culprit: mopidy-mpd is issuing a
context.core.playlists.save(playlist) for every mpd command -- so it
won't matter if one is using command_lists.
(see mopidy_mpd/protocol/stored_playlists.py lines 181, 265, 308)
does anyone know of a non-mpd-client that can edit playlists? i tried
all the web clients on mopidy.com/ext (mobile, mowecl, muse,
musicbox_webclient and iris). the first three can't select more than 1
track to add/move/delete, musicbox can't edit at all and iris talks to
the spotify api "behind my back" on its own, so mopidy_spotify is never
involved in handling it.
|
Yes, I mentioned this. I think it's a bit of a chicken and egg problem. There hasn't so far been a compelling use-case for rich playlist modification using Mopidy's API - m3u playlists are not very exciting - so web clients implementations have so far been basic, if at all. For the record, musicbox-webclient allows you to edit a "favourites" m3u playlist, but that is it. If you want to test the functionality, write some tests. If you want to test the user experience then we are a bit stuck right now. If you are trying to better understand the status quo in order to support a limited subset of the full Mopidy Playlist API then that is something else and I don't think that's a good idea considering the chicken+egg mentioned. If we have to limit the scope (why?) then we can but we need to reason about that rather than looking at current frontend implementations. EDIT: |
|
On Thu, Nov 05, 2020 at 08:31:40AM -0800, Nick Steel wrote:
Yes, [I mentioned this].
sorry, i read right over this :/
I think it's a bit of a chicken and egg problem. There hasn't so far
it's a bit of a downer to implement a feature that in practise can't be
used. still, i intend to continue working on this.
If you want to test the functionality, write some tests. If you want to
test the user experience then we are a bit stuck right now. If you are
trying to better understand the requirements in order to limit the
scope then that is something else.
i was just trying to get a feel for my implementation before figuring
out how to test stuff. looks like i won't be able to procrastinate on
that.
|
|
I'm up for attempting to write the feature for musicbox webclient (or maybe just a standalone proof of concept). |
|
And if you can come up with a way for Mopidy-MPD to do something more optimised with a command list of playlist modifications targeting the same playlist then that sounds interesting. My thoughts right now are all massive hacks that we'd never merge (I hope). |
|
I often wonder what on earth saved playlists are for in this era, but "better support for playlist editing" is amongst my most frequently requested features. So I'd definitely say don't artificially limit what you can do just because you can't see a use case for it... someone out there will disagree :)
… On 5 Nov 2020, at 17:49, Nick Steel ***@***.***> wrote:
And if you can come up with a way for Mopidy-MPD to do something more optimised with a command list of playlist modifications targeting the same playlist then that sounds interesting. My thoughts right now are all massive hacks that we'd never merge (I hope).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
i've pushed some more commits to fork-graveyard/mopidy-spotify/bug22.
there is now logic implemented for generating an edit script and
applying it, replacing playlists outright, which path to take and the
beginnings of a backend mock that can modify the playlists.
The diffing algorithm is essentially Myers diff, with some
post-processing to group insertions/deletions and merge them into a
move. it lives in mopidy_spotify/utils.py.
On Thu, Nov 05, 2020 at 09:44:51AM -0800, Nick Steel wrote:
I'm up for attempting to write the feature for musicbox webclient (or
maybe just a standalone proof of concept).
That'd be great!
On Thu, Nov 05, 2020 at 09:49:06AM -0800, Nick Steel wrote:
And if you can come up with a way for Mopidy-MPD to do something more
optimised with a command list of playlist modifications targeting the
same playlist then that sounds interesting. My thoughts right now are
all massive hacks that we'd never merge (I hope).
I had a go at this yesterday, but didn't get far. My idea was to let
command handlers know whether they were executed inside a command_list,
and instead of saving the playlist, return a list of which playlists
were modified. the command_list_end handler would then read this list
and save all the modified playlists at once.
but I couldn't figure out how to pass the state in or the list back out.
|
There's still a valid use-case when it comes to mpd-based clients though. ncmpcpp and other clients can add/remove/swap items in playlists, although that only applies AFAIK to single tracks (that's the main use-case I had in mind for my PR, like adding single tracks from the Discover Weekly/Release Radar playlists into my playlists). Also, AFAIK ncmpcpp can only append a song at the end of a playlist, not insert it somewhere in the middle. I'm not aware of clients that allow richer playlist modifications either (like scattered inserts/deletes of multiple tracks in the middle of the list), but I also believe that once mopidy provides a consistent API for this use-case the uses will come. |
|
On Sat, Nov 07, 2020 at 11:25:48AM -0800, Fabio Manganiello wrote:
ncmpcpp and other clients can add/remove/swap items in playlists,
although that only applies AFAIK to single tracks (that's the main
use-case I had in mind for my PR).
i think you can select multiple tracks and move them around as a group.
default bindings for selection is <Ins>, move up is <m> and move down in
<n>. i tested this in the playqueue, not a playlist, though.
|
|
For those following along in this thread: I've opened a new PR #287. To me, it's pretty much done, with the exception of what to do when the API returns an error, which I'd like to discuss. Feedback on the PR more than welcome! |
It also addresses the comments in mopidy#236
It also addresses the comments in mopidy#236
It also addresses the comments in mopidy#236
MPD isn't that optimal in managing playlist changes - it sends the whole snapshot of the new playlist to the
savemethod, while the Spotify Web API only requires the track changes to be submitted. It means that we have to build the deltas within thesavemethod itself. This implementation gets the job done but I'm sure that it could be further optimized.Known limitations:
In order to make the delta calculation a bit more optimized the old and new playlists are currently indexed by track
uri. It means that things can get messy if a playlist has duplicate track URIs - if a user removes an instance of a track that appears twice in the playlist then both the instances will be removed, if a user adds a track to a playlist where the track is already present then no changes will be calculated (this isn't such a bad thing IMHO, even the Spotify frontend asks for user confirmation if a track is added to a playlist where it's already present).The current implementation won't allow the invalidation of single cache items. It means that we have to call
self._backend._web_client.clear_cache()and invalidate the whole cache even if we have changed only one playlist.The
savemethod doesn't look that good now, as it handles removals, additions and swaps within the same block and progressively updates thecur_tracksmap. I believe however that it can be split into three distinctrm,addandswapmethods: all the MPD clients I know allow user interactions that fall into only one of those categories, not "bulk edits" that require a whole playlist to be updated through multiple rm/add/swap at the same time.Tests should be added.