search

suaviloquence / mopidy-listenbrainz

Mopidy extension for recording played tracks and getting recommendations to Listenbrainz, a libre alternative to Last.fm
https://mopidy.com/ext/listenbrainz/
Apache License 2.0
3 stars 1 forks source link

Search for recommended tracks using title and artist from MusicBrainz #15

Closed daniele-athome closed 3 months ago

daniele-athome commented 3 months ago

This is meant to fix #2. It is still a work in progress and it's to be considered a PoC. I wanted to get in touch with you first @suaviloquence to see if we could be on the same page. The idea could be:

(although my current code doesn't do that yet :-) for now it just searches for artist and title without even bothering to look for tracks having the mb ID - it was just for experimentation and it works actually)

Let me know what you think.

For anyone willing to try this: Iris is still suffering from a regression so you'll need to use another client to see the playlists created by mopidy-listenbrainz.

suaviloquence commented 3 months ago

Hi @daniele-athome, thanks for the PR!

  • search in Mopidy with the mb ID

  • if nothing is found, get track title and artist from musicbrainz

  • retry search in Mopidy again with track title and artist

That sounds like a good plan! Since finding the metadata from MB is going to be more expensive than a local mopidy query, we definitely only want to do it when we don't have a track locally that matches the MBID.

I'm not sure if you know, but is the iris regression an iris problem for certain (is it broken for all backends), or is it a problem in how we create our playlists in mb-listenbrainz?

daniele-athome commented 3 months ago

I'm experimenting with local:, spotify: as the search schemes, unfortunately Spotify searches with the query musicbrainz_trackid:MB-ID... return all sort of weird songs :smile:

My use case (which I guess could very well be many other users' use case) is to listen to music that I get from the ListenBrainz recommendations using either my local library or Spotify (or anything else really, even YouTube for that matter). This fallback to artist/title mechanism isn't working very well with the current configuration. I was thinking about using two different parameters:

One could configure them in different ways, for example:

If I have a perfect library (with all my music having MB ids) I could:

search_schemes = local:
search_schemes_fallback = spotify:

Otherwise:

search_schemes = local:
search_schemes_fallback = local:, spotify:

Parameter names could be different of course, it's just to get the idea. What do you think?

EDIT: damn it Mopidy-Spotify just stopped working... I either got banned or something changed server-side... I'll try with YouTube.

daniele-athome commented 3 months ago

I'm not sure if you know, but is the iris regression an iris problem for certain (is it broken for all backends), or is it a problem in how we create our playlists in mb-listenbrainz?

I wouldn't know because I don't have other backends to try. I only have Local and Spotify, both working in Iris.

daniele-athome commented 3 months ago

I got some time to code and I wrote it anyway :-) It's still a bit messy but the idea is there and it works - apart from the fact that Spotify occasionally breaks...

Also I had to make a any query because very few backends support artist+title queries (I used the format "{artist} - {title}", but let's discuss this).

EDIT: I'll keep it draft until I tidy up the code a bit.

suaviloquence commented 3 months ago

  • search_schemes which is used for the MB id query

  • search_schemes_fallback which is used for the artist/title query

That looks good!

suaviloquence commented 3 months ago

Mopidy-Spotify just stopped working...

Do you think the way we are searching made you ratelimited in spotify? And would there be a way to avoid this? I don't have spotify so I can't check with that.

daniele-athome commented 3 months ago

Do you think the way we are searching made you ratelimited in spotify? And would there be a way to avoid this? I don't have spotify so I can't check with that.

Maybe... Mopidy-Spotify is not really helpful with its error messages (Failed to lookup Spotify track 'spotify:track:4Qq66rN8HWHk7NGPYfNvun': Invalid track response) so it might be rate limiting or it might not. Although I was doing like 1 request/second, I don't think that qualifies as excessive usage...

daniele-athome commented 3 months ago

On a deeper analysis: I begin to get those errors when I load the generated playlist. As soon as I try to open it from my client I get a burst of those "Invalid track response" errors on the server, so maybe it is rate limiting. And if it really is, there is no easy way to fix it - surely not by doing something in this package. I'm guessing Mopidy-Spotify could "batch" those requests somehow - if those are supported by Spotify API, that is.

EDIT: of course Mopidy-Spotify implemented batch requests, but it's for Mopidy v4: https://github.com/mopidy/mopidy-spotify/commit/dbb653ab4821c6e68cff1eb2de29082b95d95210 I'll backport that in my fork and see how it goes.

daniele-athome commented 3 months ago

Ok I think I'm finished here. I did some tests by backporting (ugh!) batch lookup mode from Mopidy v4 and while it did improve the overall experience, I still systematically get (apparently after a random time, I couldn't determine a pattern) 401 responses (not even 429!) even if I forcibly put a sleep between requests of 2 seconds. But this is a problem for another time.

I believe the code works now and it is ready for review. I left a couple of review notes myself for some things that might need your special attention.

suaviloquence commented 3 months ago

Also I've been using black to format the code, it would be great if you could format this branch so CI passes.

daniele-athome commented 3 months ago

I should have fixed everything. I've also thrown in a couple of warning fixes because why not :-)

daniele-athome commented 3 months ago

I forgot to update the readme with the new parameter! I'll get on it asap.

suaviloquence commented 3 months ago

Here's what I was going to add in #17 before I saw your comment. Feel free to use any of it and change any part, or not if you think something would work better:

daniele-athome commented 3 months ago

It's perfect.

daniele-athome commented 3 months ago

No sorry :-) the default value is "local:" - should we use that or allow all schemes by default? Considering the rate limiting issues and all... What do you think?