-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support import of ListenBrainz recommendations playlists #1
Support import of ListenBrainz recommendations playlists #1
Conversation
Something is wrong with current implementation because UPDATE: Fixed an import exception with Mopidy 3.4.2, also missing name while creating playlists, and finally missing tracks. |
9fd3925
to
b487711
Compare
d23988f
to
5e830db
Compare
3c62f20
to
294de17
Compare
ca2f78d
to
96deeeb
Compare
Thanks for the PR! Sorry for being so late, for some reason github did not notify me about it. |
FYI, there's a regression in Iris, jaedb/Iris#953 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good but I can't test it super well because I don't have the songs in my local backend. If you wanted to add configuration (here, in another PR) for that it would be awesome but I can still merge this one without it.
README.rst
Outdated
@@ -45,7 +45,8 @@ The following configuration values are available: | |||
Defaults to enabled. | |||
- ``listenbrainz/token``: Your `Listenbrainz user token <https://listenbrainz.org/profile/>`_ | |||
- ``listenbrainz/url``: The URL of the API of the Listenbrainz instance to record listens to (default: api.listenbrainz.org) | |||
|
|||
- ``listenbrainz/import_playlists``: Whether to import Listenbrainz playlists (default: ``false``) | |||
- ``listenbrainz/periodic_playlists_update``: Enable periodic import of Listenbrainz playlists (default: ``true``) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- ``listenbrainz/periodic_playlists_update``: Enable periodic import of Listenbrainz playlists (default: ``true``) | |
- ``listenbrainz/update_playlists``: Enable periodic import of Listenbrainz playlists (default: ``true``) |
or something to keep the verb_noun
order consistent. let me know if you think there's a better name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand and like the verb_noun
paradigm.
May I suggest update_playlists_periodically
, or monitor_playlists
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking for a meaning full name, I realized that this configuration key is useless. Listenbrainz generates new recommendation playlists on each Monday, why would a user import playlists once and refuse to update when new recommendations are published? If Mopidy is restarted for whatever reason, the new recommendations are imported! Non-sense.
I thus propose to remove this configuration key.
# finally be deleted | ||
|
||
logger.debug(f"Already known playlist {playlist_uri}") | ||
# maybe there're new tracks in Mopidy's database... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that the playlist has been updated since import? Do the playlists change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my experience, the playlists don't change from Listenbrainz backend point of view (but can't find the information in their documentation). From mopidy's point of view, they may change due to new tracks being imported in the library. The comment is related to that part.
mopidy_listenbrainz/frontend.py
Outdated
) | ||
# search only in local database since other backends can | ||
# be quite long to answer, should we offer choice through | ||
# config? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be useful. For me no playlists are showing up because I use the mopidy-subidy
extension exclusively
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suaviloquence The track search is not limited to Mopidy-Local anymore. The default is to search through all backends. The search_schemes
configuration key allows to specify a list of schemes to limit the results to URIs with the given schemes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arggg... I just tried with my living room instance of Mopidy, a "production instance" with multiple backends. And
some backends seems to not support search by musicbrainz_trackid
and not respect the API...
No idea how to fix this...
mai 14 22:18:07 argos mopidy[1925]: INFO [ListenbrainzFrontend-15 (_actor_loop)] mopidy_listenbrainz.frontend Importing ListenBrainz playlists
mai 14 22:18:08 argos mopidy[1925]: INFO [InternetArchiveBackend-6 (_actor_loop)] mopidy_internetarchive.library Not searching Mopidy-InternetArchive: Keyword "musicbrainz_trackid" not supported
mai 14 22:18:08 argos mopidy[1925]: ERROR [Core-11 (_actor_loop)] mopidy.core.library FileBackend backend returned bad data: Expected a SearchResult instance, not []
mai 14 22:18:08 argos mopidy[1925]: ERROR [Core-11 (_actor_loop)] mopidy.core.library StreamBackend backend returned bad data: Expected a SearchResult instance, not []
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to have local:
as default scheme for track searches and keep the documentation for users to be able to adapt to their favorite backend but warn them about many backends not supporting searches by musicbrainz id... Simple and acceptable to start with I guess.
Good point. I'll do it here, next week seems reasonable. |
Co-authored-by: Max Carr <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, awesome! ❤️ There are a few bugs about default config, but I think these suggestions should take care of that, let me know if they don't. Thanks for your work on this!
|
||
for track_mbid in playlist_data.track_mbids: | ||
query = self.library.search( | ||
{"musicbrainz_trackid": [track_mbid]}, uris=search_schemes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For an enhancement later (doesn't need to be in this PR), does LB expose the artist/track name in the playlist response? Might help with finding tracks without tagged MBIDs (or maybe from a different album) if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I received recommendations on this topic in the Listenbrainz channel on Discourse: See https://community.metabrainz.org/t/support-for-recommendations-in-mopidy-listenbrainz/688258/6
Also feel free to take credit and link yourself in the credits section of README.rst for your awesome work! |
Co-authored-by: Max Carr <[email protected]>
🎉 |
Proposal
The proposed branch adds support for playlists returned by the playlists created for endpoint (recommendations playlists) from ListenBrainz API.
AFAICT the returned playlists are made of suggestions taken from previously listened tracks.
Implementation
The playlist descriptions are fetched by the existing Mopidy frontend during setup. Mopidy-Local database is then searched for matching tracks and Mopidy playlists are created using a trivial Mopidy backend dedicated to those playlists management.
The playlist import must be enabled using a new configuration key
import_playlists
(default tofalse
).Playlists are periodically updated (each week, on monday).
Possible improvements
Test
python -m pip install --upgrade --break-system-packages git+https://github.com/orontee/mopidy-listenbrainz.git@feature/playlists
Tried with both Argos and Iris.