Skip to content
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

LB-1700: Get all tracks for apple music playlist #154

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MonkeyDo
Copy link
Member

@MonkeyDo MonkeyDo commented Dec 9, 2024

Following #153, fixing the same issue for Apple Music.

The current implementation using the Apple Music API "?include=tracks" only returns the first 100 tracks with no pagination option.
To get the rest of the tracks if there are more, we need to fetch them separately using another endpoint ("/tracks"). Thankfully we can reuse the included tracks from the first call and start at offset 100.

The current implementation using the Apple Music API "?include=tracks" only returns the first 100 tracks.
To get the rest if there are more, we need to fetch them separately using another endpoint ("/tracks")
@MonkeyDo
Copy link
Member Author

MonkeyDo commented Dec 9, 2024

Currently up on test.listenbrainz.org, but probably not for long.
Screenshot of a playlist with > 100 tracks that I tested manually:
image

Copy link

github-actions bot commented Dec 9, 2024

Test Results

  4 files  ±0    4 suites  ±0   4s ⏱️ ±0s
 50 tests ±0   50 ✅ ±0  0 💤 ±0  0 ❌ ±0 
200 runs  ±0  200 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit a82eaa3. ± Comparison against base commit 1bcb0e0.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 9, 2024

Package Line Rate Complexity Health
. 32% 0
content_resolver 0% 0
content_resolver.formats 6% 0
content_resolver.model 0% 0
external 39% 0
listenbrainz 46% 0
musicbrainz 71% 0
patches 27% 0
patches.lb_radio_classes 0% 0
tools 20% 0
. 32% 0
content_resolver 0% 0
content_resolver.formats 6% 0
content_resolver.model 0% 0
external 39% 0
listenbrainz 45% 0
musicbrainz 69% 0
patches 27% 0
patches.lb_radio_classes 0% 0
tools 20% 0
. 32% 0
content_resolver 0% 0
content_resolver.formats 6% 0
content_resolver.model 0% 0
external 39% 0
listenbrainz 46% 0
musicbrainz 71% 0
patches 27% 0
patches.lb_radio_classes 0% 0
tools 20% 0
. 32% 0
content_resolver 0% 0
content_resolver.formats 6% 0
content_resolver.model 0% 0
external 39% 0
listenbrainz 46% 0
musicbrainz 71% 0
patches 27% 0
patches.lb_radio_classes 0% 0
tools 20% 0
Summary 21% (4190 / 20149) 0

@MonkeyDo MonkeyDo requested a review from mayhem December 9, 2024 14:59
Copy link
Member

@mayhem mayhem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are no error handling improvements, good to merge.

offset = len(tracks)
# endpoint returns 100 tracks per call max -> run iteratively with an offset until there are no more tracks
while True:
url = f"{APPLE_MUSIC_URL}/me/library/playlists/{playlist_id}/tracks?limit=100&offset={offset}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does any more error checking need to be added here? rate limiting or otherwise?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to ensure that errors that might happen are not silently dropped.

Copy link
Member Author

@MonkeyDo MonkeyDo Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A retry strategy is set up at the class level, not sure if that handles rate limiting though looks like urllib3 Retry class handles rate limiting headers by default
https://github.com/metabrainz/troi-recommendation-playground/blob/6293c4993d40048492e20270269ec546b1b21be9/troi/tools/utils.py#L183C5-L197C16

What should be the strategy for handling other errors?
The API will sometimes return a json body with an error key, so I could try parsing for that and logging the error message (instead of the current caught KeyError) as an improvement.
Anything else that I should look out for?

Parse the response body for errors, log any error found
@MonkeyDo MonkeyDo force-pushed the apple-playlists-tracks branch from 0e6eaaf to a82eaa3 Compare December 10, 2024 16:38
@MonkeyDo MonkeyDo requested a review from mayhem December 11, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants