Support writing ISRC to tracks in Spotify plugin

[arsaboo]

Description

Fixes #4992

To Do

• Documentation.
• Changelog.
• Tests.

[arsaboo]

@JOJ0 can you test this out with an album that has ISRC? It seems Spotify returns ISRC at both the album and track levels. Right now, I am importing both. I have never used ISRC, so I'm not sure if that is the right approach. I will look for your guidance here.

[JOJ0]

@arsaboo thanks for getting this started.

In the meantime I quickfixed this for myself so I could move on with my personal project. Please just cherry-pick this commit into your PR branch: JOJ0@30e0fb1

It implements proper fetching of the full track details and then writing to the TrackInfo object. It's tested, I'm using it.

Your current implementation won't work since, as I tried to explain here, we currently only fetch a, so called "SimplifiedTrackObject", which definitely does not contain the track's ISRC. To get it we need to fetch a full track object!

Have a look at the example response of the items object when fetching an album, the track information is very minimal, and does not contain tracks ISRC: https://developer.spotify.com/documentation/web-api/reference/get-an-album

Then have a look at the full track infromation when fetching via the track's endpoint (which my code implements): https://developer.spotify.com/documentation/web-api/reference/get-track
We do get an ISRC like that :-)

[JOJ0]

Regarding album ISRC I don't have a finished commit at hand but I just read the docs again and can report that I would understand that the album-level ISRC is already included via the endpoint we fetch, which is again this one: https://developer.spotify.com/documentation/web-api/reference/get-an-album and the example response contains it already :-) So your current implementation will do!

If it helps, I can test from your branch, once you implemented both track-level and album-level ISRC's :-)

[arsaboo]

Good catch.

Would it make sense to create a separate function to update ISRC for those who want to use it? Otherwise, we are significantly increasing the number of API calls.

[JOJ0]

Good catch.

Would it make sense to create a separate function to update ISRC for those who want to use it? Otherwise, we are significantly increasing the number of API calls.

Yeah I'm also thinking about "too many requests errors" (where is our rate-limiting code actually? do we have one? :-))

On the other hand, beets fetches track ISRC per default when using MusicBrainz and Deezer. So making it configurable with Spotify plugin only would be odd or at least would be "non-streamlined" behaviour within beets. IMHO the way to go is trying to support similar things as good as possible throughout sources and try to have similar defaults, to get a streamlined user experience. (repeat: as good as possible)

This is how Deezer plugin does it in the album_for_id method, I tried to implement it similarly:

beets/beetsplug/deezer.py

Lines 111 to 117 in 708a04d

	tracks_obj = requests.get(self.album_url + deezer_id + "/tracks").json()
	tracks_data = tracks_obj["data"]
	if not tracks_data:
	return None
	while "next" in tracks_obj:
	tracks_obj = requests.get(tracks_obj["next"]).json()
	tracks_data.extend(tracks_obj["data"])

[JOJ0]

I was curious, we have ISRC for tracks fetching in MusicBrainz since 2020: b6fda63

BTW the MusicBrainz API is smarter about "I now want more info that is not available by default" - you can just tell the API to "include more info" on the initial requests. I wish Spotify's API could do that with albums - it could simply make it configurable of what info to include for the items object.

Off-topic but interesting

From another personal project I played around a lot with the playlists endpoint. Here we have a fields option that allows to "include additional" information as well as "restrict to what we really need", I see it as a kind of "filter" possibility. Have a look at the fields option in the docs here: https://developer.spotify.com/documentation/web-api/reference/get-playlist

This is pretty similar to what the MusicBrainz API offers for regular albums and recordings already! configurable API endpoints! Actually an awesome feature!

[arsaboo]

We do have the _fetch_info function that can be updated to fetch the ISRC:

beets/beetsplug/spotify.py

Line 640 in 6cda741

def _fetch_info(self, items, write, force):

[JOJ0]

The only place where this function is called is here:

beets/beetsplug/spotify.py

Lines 480 to 482 in 6cda741

	def func(lib, opts, args):
	items = lib.items(ui.decargs(args))
	self._fetch_info(items, ui.should_write(), opts.force_refetch)

Will it be called when doing a regular import? I'm having a hard time reading that code at the moment......

beets/beetsplug/spotify.py

Lines 438 to 485 in 6cda741

	def commands(self):
	# autotagger import command
	def queries(lib, opts, args):
	success = self._parse_opts(opts)
	if success:
	results = self._match_library_tracks(lib, ui.decargs(args))
	self._output_match_results(results)
	spotify_cmd = ui.Subcommand(
	"spotify", help=f"build a {self.data_source} playlist"
	)
	spotify_cmd.parser.add_option(
	"-m",
	"--mode",
	action="store",
	help='"open" to open {} with playlist, '
	'"list" to print (default)'.format(self.data_source),
	)
	spotify_cmd.parser.add_option(
	"-f",
	"--show-failures",
	action="store_true",
	dest="show_failures",
	help="list tracks that did not match a {} ID".format(
	self.data_source
	),
	)
	spotify_cmd.func = queries
	# spotifysync command
	sync_cmd = ui.Subcommand(
	"spotifysync", help="fetch track attributes from Spotify"
	)
	sync_cmd.parser.add_option(
	"-f",
	"--force",
	dest="force_refetch",
	action="store_true",
	default=False,
	help="re-download data when already present",
	)
	def func(lib, opts, args):
	items = lib.items(ui.decargs(args))
	self._fetch_info(items, ui.should_write(), opts.force_refetch)
	sync_cmd.func = func
	return [spotify_cmd, sync_cmd]

[arsaboo]

It is not called during regular import. The user will have to call beet spotifysync, which will refresh the optional information. While we are at it, I also included EAN and UPC.

[JOJ0]

It is not called during regular import. The user will have to call beet spotifysync, which will refresh the optional information. While we are at it, I also included EAN and UPC.

Well, then this is not the solution I was aiming for. I think this should be similar to other plugins and ISRC should definitely be fetched on initial import. Why make it different? I do not want a second step when importing my tracks via Spotify. I do not have to issue a second step when I use MusicBrainz or Deezer.

[arsaboo]

The problem with that approach is that it will definitely cause API errors. Additional API calls for all the tracks for all the albums (even those that are not used) will cause excessive API usage (especially when importing a large number of albums). Even with the current implementation, I sometimes get API errors (partly fixed with #4943). I don't think we should add multiple calls for everyone (not sure how popular/useful the ISRC field is).

Once we have the track_ids, we can limit the calls for only those ids. This will significantly economize the number of calls.

Just run beet import ... && beet spotifysync 😉 or we can even use the hook plugin, which will have the same effect

[arsaboo]

While the rate limits for Spotify are not published, folks suggest that Spotify allows for approximately 180 requests per minute without returning the error 429. For comparison, Deezer allows around 50 per 5 seconds, i.e., 600 calls per minute.

[JOJ0]

Hi and thanks a lot for the detailed information about the why's and how's! I now understand that at this point just adding an additional fetch to the tracks endpoint will cause rate-limiting trouble. I also now see that you already put some effort into rate-limiting in #4943 and #4946. I'm wondering why this still is not enough, it's actually pretty much what the Spotify docs suggest (Develop a backoff-retry strategy) https://developer.spotify.com/documentation/web-api/concepts/rate-limits).

Should we open another issue, or de we have one already, to further discuss rate limiting ideas?

[arsaboo]

The problem is that Spotify often (or rather mostly) does not return the retry-after interval. The only option is to give up after a few tries, which is what I did in the previous PR. We can open another issue for a better retry logic, if required.

I'll try to fix the other issues tomorrow (thanks for your detailed review).

[JOJ0]

The problem is that Spotify often (or rather mostly) does not return the retry-after interval. The only option is to give up after a few tries, which is what I did in the previous PR. We can open another issue for a better retry logic, if required.

Yes please, if you find a minute, please open one. You already started in the very right direction with your retry logic. Going a little further there I'd like to tell you what we've learned when we implemented rate-limiting magic in python3-discogs-client a couple of years ago.

I'll try to fix the other issues tomorrow (thanks for your detailed review).

Welcome! Thanks for quickly implementing this! Awesome new feature!

[JOJ0]

..., I sometimes get API errors (partly fixed with #4943). I don't think we should add multiple calls for everyone (not sure how popular/useful the ISRC field is).

Regarding ratelimiting: As suggested, let's better open a new issue and let's collect ideas together :-)

Regarding "how useful is ISRC / what can it be used for": As far as I understand it and use it, it is a unique ID throughout tagging sources and I think it currently is the best way to "link" Deezer, MusicBrainz and Spotify tagged items together

My personal usecase is, generating offline-m3u-playlists as pools for my DJ use. I have most of the music I collect on Spotify in my offline-collection on my DJ computer as well. I'm building and ordering playlists on my mobile within the Spotify app, and then can generate m3u playlists by looking up ISRCs via the Spotify playlist API, as well as search in the local beets lib for them.

I have fallback searches for artist/title search within my beets library, but actually ISRC is the most accurate way to find exact versions of tracks and the best way for my use-case to generate those playlists.

And no sorry I can't make this program available online, it is a super-poluted-all-purpose-personal-super-opinionated-DJ-preparation-tool-with-a-lot-of-crappy-code-and-ten-different-subcommands...

Once we have the track_ids, we can limit the calls for only those ids. This will significantly economize the number of calls.

Makes sense!

Just run beet import ... && beet spotifysync 😉 or we can even use the hook plugin, which will have the same effect

Oh, great idea using the hook plugin! Thanks!

[JOJ0]

While the rate limits for Spotify are not published, folks suggest that Spotify allows for approximately 180 requests per minute without returning the error 429. For comparison, Deezer allows around 50 per 5 seconds, i.e., 600 calls per minute.

Discogs has even less, 60 per minute: https://www.discogs.com/developers#page:home,header:home-rate-limiting

-> Let's discuss in new issue

PS: Sorry I accidentially edited your original post, but I reverted it as it was before! 😅