Add TIDAL autotagger plugin#5637
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
- Initial implementation of LRC writing support - Split imported file processing to its' own function
- Actually remove the session file instead of blanking it out
- Check for auto status before loading the TIDAL session
- Implement LFUCache on hot functions - Fix docstrings - Add backoff handler for debugging - Add fatal argument to _load_session to throw UserError - Reduce excessive calls for track metadata retrival - _search_track now returns tidalapi types and no longer grabs the full album metadata - Add user configurable result limits for both metadata and lyrics
- tidalapi was changed from a star import to a regular import - Logic error in _process_item was fixed, was used undefined track
- Fix duplicate track removal from _search_lyrics - Stop adding top_hit result twice in _search_album and _search_track
- Add output when the login was successful
It was never used and it can always be dumped with a regular `cat` command
- Break out _get_lyrics specific metadata searching algorithm into seperate function (_search_from_metadata) - Remove _search_album as it is now unused - Fix candidates function shadowing parameters - Candidate functions now use _search_from_metadata - Candidate functions now handle va_likely
The plugin has been operating on the original files this entire time. Changes: - Add a write event listener to write out sidecar files - Remove file management from the import stage, as this is already handled in Item.write()
This currently only works with synced lyrics and needs to be changed if TIDAL ever changes their lyrics format.
- Change _validate_lyrics to use tidalapi Track length instead of calculating length from lyrics - Change _search_from_metadata to search for alternative artists - Add config toggle for items with no duration to assume valid lyrics or not - Change _search_track so queries are stripped of special characters and medium details, as the TIDAL search engine is picky
Technically, Items can have an infinite number of max artists. Let's add a user-configurable cap to avoid excessive TIDAL API usage.
- Add another implementation of _serach_album - Remove unused variables from LRC timestamp calculation - Change candidates function to use _search_album instead of grabbing a new Album instance from _search_track - Change _search_from_metadata to search for albums as well - Fix logic errors and unused variables in _validate_lyrics
- Move the initial session load into __init__ as it is called before any of the plugin functions - Move query fixing into _tidal_search from the respective _search functions - Reworded UIError message in _load_session
- Ran `poe lint` and fixed all errors - Ran `poe format` - Reworded some docstrings to make them shorter - Split some strings between two lines to avoid going over line length limit
- Move _load_session from __init__ to an import_begin event handler
jcjordyn130
force-pushed
the
tidal_metadata
branch
from
19781aa to
c1b9f21
Compare
jcjordyn130
force-pushed
the
tidal_metadata
branch
from
3b3e506 to
851b2b4
Compare
Turns out, _search_lyrics didn't use its own limit parameter and was directly using self.config.
jcjordyn130
force-pushed
the
tidal_metadata
branch
from
851b2b4 to
68a1df7
Compare
- _load_session now calls _save_session if the login succeeds and the expiry date is in the past, meaning the token has changed. This saves on excessive authentication traffic.
- Add support for TIDAL popularity with tracks and albums - Remove debug logging from _parse_copyright as it was overly verbose, once per track - Move TIDAL specific metadata out of TrackInfo/AlbumInfo constructor - Add support for grabbing cover art from TIDAL with a max resolution limit, as it is lossy encoded and TIDAL provides a few set resolutions for us - Add more TIDAL IDs in albums and tracks, now tracks contain the track id, album id, and primary artist ID - Update documentation for new config tunables
- Add --fetch to fetch lyrics for arbitrary TIDAL track IDs. This is useful when the lyrics are correct, but other sources have better metadata. - Add --refresh to refresh metadata for all TIDAL tagged tracks and albums in the library. Currently this only supports popularity.
snejus
left a comment
snejus
left a comment
There was a problem hiding this comment.
Added a couple of comments. Please add some tests to assert this works fine and to document the JSON data that TIDAL API returns.
Thanks for this great work!
| # Nothing above invalidated the lyrics, assuming valid | ||
| return True | ||
|
|
||
| def _search_lyrics(self, item, limit=10): |
There was a problem hiding this comment.
I don't think this is the right place for lyrics search. We have lyrics plugin for this, so I would expect TIDAL to be added as a backend there (in a separate PR).
There was a problem hiding this comment.
That's what I thought at first, but I though that from #4641 (comment) that they wanted it to be part of the autotagger.
Anyways, would I be reimplementing parts of this plugin in the lyrics plugin or would I be able to call into this plugin from the lyrics plugin? Either that or monkeypatch the lyrics plugin to add a new backend
|
|
||
| return trackinfo | ||
|
|
||
| def _search_from_metadata(self, item, limit=10): |
There was a problem hiding this comment.
It seems to me that this method will be slow to return results due to the number of search requests it makes. Have you tested it?
They were pretty similar and both ended up calling _tidal_search anyways, so they were abstracted into it. The cache decorator was also adjusted to use the query and return type as a cache key
jcjordyn130
force-pushed
the
tidal_metadata
branch
from
1d022e6 to
5cedf66
Compare
|
No problem, I enjoy contributing to and using beets. Anyways, as for testing, are you wanting me to probe the TIDAL API directly with requests for testing or to just use the tidalapi wrapper for it? |
Additionally, some asserts were added to ensure that _load_session was called and to make MyPy happy. Docstring type comments were removed as they were made redundant by the type hints.
Turns out, I used the wrong quotes in an f-string and it split the string into two... whoops
- Add logger assertion in backoff_handler - Change candidates signature to match the superclass - Fix incorrect signature in commands - Import annotations from __future__ to fix X | Y union shorthand on older Python versions - Fix incorrect assignment to logger in TidalPlugin
The asserts were comparing the object to a type vs checking the result of isinstance
This should hopefully resolve the missing attribute errors from MyPy
|
I ask because tidalapi already has tests implemented https://github.com/tamland/python-tidal/tree/master/tests, and using tidalapi types is impossible without a session meaning that the majority of the plugin would need to use integration tests instead of regular tests |
I forgot to do this in the last commit and didn't notice until importing a big album that triggered rate limiting
There was a problem hiding this comment.
Pull Request Overview
This PR implements the new TIDAL autotagger plugin using the TIDAL API to fetch metadata and lyrics, supporting direct TIDAL URLs and multiple features.
- Adds optional dependencies (tidalapi, cachetools, backoff) in pyproject.toml
- Introduces a dependency group for TIDAL-related libraries
Files not reviewed (2)
- docs/plugins/index.rst: Language not supported
- docs/plugins/tidal.rst: Language not supported
Comments suppressed due to low confidence (1)
pyproject.toml:152
- [nitpick] The dependency group 'tidal' is a bit ambiguous; consider renaming it to a more descriptive name such as 'tidal_autotagger' to clarify its purpose.
tidal = ["tidalapi", "cachetools", "backoff"]
| sphinx-copybutton = { version = "^0.5.2", optional = true } | ||
| tidalapi = {version = "^0.8.3", optional = true} | ||
| cachetools = {version = "^5.5.1", optional = true} | ||
| backoff = {version = "^2.2.1", optional = true} |
There was a problem hiding this comment.
You might want to consider switching to python-backoff, the original is unmaintained.
|
I changed this PR to a draft because the client token that the tidalapi module was using got revoked. |
It should update the token automatically. I have a custom tidal plugin that works the same way. Is the token not updating? |
|
@jcjordyn130 are you still planning to work on this plugin? |
|
Hey y'all! This PR had stalled for quite some time, but I think a Tidal plugin could be really valuable, so I picked it back up (see #6520) 🙃 The approach is more minimal (for now): only handles metadata lookup via Tidal, and I rolled my own small API wrapper instead of using @jcjordyn130 If you are still around, I'd love to get your feedback, especially since I also took some inspiration here. |
|
Closing this PR as it's superseded by #6520. Thanks for your contribution @jcjordyn130! ❤️ |
## Description This PR introduces tidal as metadatasource. It add both an minimal api layer and the typical metadata source plugin capabilities. ### Details The implementation provides a small API layer consisting of `TidalAPI` for high-level album and track fetching, and `TidalSession` which extends `requests.Session` with token authentication, automatic rate limiting (~4 req/s via `RateLimitAdapter`), and pagination resolution following the JSON:API spec. Authentication is handled through an OAuth2 PKCE flow accessible via `beet tidal --auth`, with automatic token refresh when the access token expires. Metadata parsing handles Tidal's JSON:API response format, extracting album and track information including ISO 8601 duration conversion, artist relationships, and copyright/label data. ## Input wanted The API layer currently lacks comprehensive test coverage. Setting up proper tests would require either mocking all outgoing requests or creating a dedicated test token (which necessitates an account and might require read/write to github secrets). Are we comfortable with the current approach of unit testing the plugin itself while mocking all requests? ## TODOs - [x] Documentation - [x] `candidate` and `item_candidates` lookup - [x] It should be possible to optimize batched lookups - [x] Add tests for candidates and item_candidates - [x] Implement batching for more than 20 filters ## Refs thanks to @jcjordyn130 for his initial implementations in #5637 and #4641
Description
This is a replacement of the last TIDAL PR I opened back in 2023 (#4641), as in, it is a proper autotagger implementation using the TIDAL API.
Candidate retrieval and support for direct TIDAL album and track URLs are supported.
It also fetches lyrics (both time-synced and not), and embeds it into both the file tags and a LRC file if configured.
Adding testing is a little complicated as testing lyrics support requires a paid account, however, basic metadata queries only require an account with or without an active subscription.
I have been using this plugin for about a week now with my library and it seems to be holding up, obtaining lyrics that are otherwise unavailable through the available lyrics backends.
Poetry files were changed to add optional dependencies for this plugin, which are tidalapi, cachetools (for LFU cache in hot functions), and backoff (to slow down queries when rate limited).
Also, please do let me know if you want me to squash my commits, as I know there are quite a few of them.
To Do
Changelog(Will add once change is approved)Tests