Open stephanebruckert opened 4 years ago
Hey @stephanebruckert I wasn't able to replicate this issue in my environment (spotipy==2.15.0, ubuntu 20.04.1 LTS, xterm) - though that might be because our networks are different. I think the root issue is that spotipy can't currently handle API rate limit exceeding as we're raising exceptions on any HTTPError. A solution might exist in the parsing of response.json()['Retry-After']
(see this link) but this would be hard to test without the error actually being thrown. Any ideas on replicable code to (quickly) exceed the rate limit?
Try this:
(by the way, to test it against the main branch on your local clone, the script just needs to be placed at the root of the project)
from concurrent.futures import ThreadPoolExecutor
import spotipy
TOTAL_ITEMS_TO_ADD = 400
MAX_WORKERS = 50
PLAYLIST_OWNER_ID = "your_user_id"
# Spotify init
sp = spotipy.Spotify(auth_manager=spotipy.SpotifyOAuth(scope='playlist-modify-public playlist-modify-private',
show_dialog=True))
some_track_id = sp.search('test')['tracks']['items'][0]['uri']
playlist = sp.user_playlist_create(PLAYLIST_OWNER_ID, "test", False)
ids_to_add = [some_track_id] * TOTAL_ITEMS_TO_ADD
def add_item(track_id):
# It does not work with POST:
sp.playlist_add_items(playlist['id'], [track_id], 0)
# It works with GET
# sp.search('test')
print("Success")
with ThreadPoolExecutor(max_workers=MAX_WORKERS) as pool:
pool.map(add_item, ids_to_add)
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
HTTP Error for POST to https://api.spotify.com/v1/playlists/0njyczNVNKFynnlzM6Tm9y/tracks returned 429 due to API rate limit exceeded
Success
HTTP Error for POST to https://api.spotify.com/v1/playlists/0njyczNVNKFynnlzM6Tm9y/tracks returned 429 due to API rate limit exceeded
HTTP Error for POST to https://api.spotify.com/v1/playlists/0njyczNVNKFynnlzM6Tm9y/tracks returned 429 due to API rate limit exceeded
Success
Success
Success
HTTP Error for POST to https://api.spotify.com/v1/playlists/0njyczNVNKFynnlzM6Tm9y/tracks returned 429 due to API rate limit exceeded
Success
Success
HTTP Error for POST to https://api.spotify.com/v1/playlists/0njyczNVNKFynnlzM6Tm9y/tracks returned 429 due to API rate limit exceeded
HTTP Error for POST to https://api.spotify.com/v1/playlists/0njyczNVNKFynnlzM6Tm9y/tracks returned 429 due to API rate limit exceeded
Success
HTTP Error for POST to https://api.spotify.com/v1/playlists/0njyczNVNKFynnlzM6Tm9y/tracks returned 429 due to API rate limit exceeded
HTTP Error for POST to https://api.spotify.com/v1/playlists/0njyczNVNKFynnlzM6Tm9y/tracks returned 429 due to API rate limit exceeded
Success
HTTP Error for POST to https://api.spotify.com/v1/playlists/0njyczNVNKFynnlzM6Tm9y/tracks returned 429 due to API rate limit exceeded
I think here Requests
is handling retry-after
for us, so we shouldn't need to do anything around that:
Also it works well for GET as it correctly retries:
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Max Retries reached
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
Success
I wonder, why does it work for GET and not for POST, knowing that both our GET and POST methods call the same _internal_call
:
PS: I just found this https://stackoverflow.com/a/35707701/1515819, that might be our solution!
@Quizz1Cal @ritiek please review this fix if you get a chance https://github.com/plamere/spotipy/pull/596
@stephanebruckert I think its worth noting that there is a reason that urllib does not do retries for POSTS and that is because they are not considered safe or idempotent. GET, PUT, and DELETE fall into these categories but POST however does not. More detail can be found in RFC7231. Having retries on POSTs can have unintended consequences like having multiple records created in the database if there was an error upon creation.
It is my personal opinion that this should not have been created as it is not an issue for the following reasons:
Please let me know if I am missing something and I am open to hearing out the reasons for the change, but I figured I would add my 2 cents as I helped contribute the change to the retry logic.
Sorry for the late response I was bit busy recently. Thanks also for the inputs that's all fair points. It's my bad, I should have waited for feedback on the PR, but I'm happy to discuss it here now and am also happy to revert things if needed
Answering your points in order:
https://github.com/plamere/spotipy/blob/b68a70221457951d18321773936ddcf7a5c49795/spotipy/client.py#L36
I think you are right, it's probably a problem for 500, 502, 503, 504.
However my main motivation is to retry when Retry-After
is provided, that means when we get 429, I believe we should honour Retry-After
and retry. Perhaps we should instead update default_retry_codes
and limit it to 429
only? Although it could be challenging to configure urllib3 to retry on all GET status codes, but only retry on POST 429s... This seems related https://github.com/urllib3/urllib3/issues/260
If a user needs that to happen in a special case that is up to them to code.
Let's try to solve this problem in the library, so that devs don't have to worry about that exact same thing.
method_whitelist=false
? if we agree on the point above (only retrying idempotent 429s)Great to discuss this!!
Related to the first point about always retrying on 429, I just found we could use respect_retry_after_header
respect_retry_after_header (bool) – Whether to respect Retry-After header on status codes defined as Retry.RETRY_AFTER_STATUS_CODES or not. https://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html
I'm going to open a PR to set respect_retry_after_header
to true and revert the change on method_whitelist
as you suggested
Edit: did a quick test and doesn't seem to work
@stephanebruckert no worries! Everyone gets busy and I am late to responding as well. I am glad we are talking about this as I always find this topic interesting! It seems like your main reason was to have to respect the Retry-After
header if present and I think that is fair and we should do the work to ensure this is enabled for devs by default.
After looking into it:
respect_retry_after_header
option, but I believe it is enabled by default. And on further inspection it looks like it is used in these two functions is_retry and sleep
is_retry
checks to make sure the retry is needed and sleep
does the appropriate waiting. It will wait for the time that is in header or it will respect the backoff that is specified in the classAlso just to clear some things up in the comment with bullet points:
allowed_methods
param if not specified. This param controls what methods are retried.
So where does this leave us?
allowed_methods
to include the POST as well as actual idempotent defaults that urllib3 provides if it would honor the retries. I will try this today at some point hopefully.
allowed_methods
to the [SpotipyClient](https://github.com/urllib3/urllib3/blob/16b7b332fd1b84c2d465f11d17658c1e83d3f20f/src/urllib3/connectionpool.py#L834-L844)
that way users can programmatically add the POST method if they want to.
Again really enjoying talking about this :) good stuff to think about and a fun topic! Let me know your thoughts and if anything I said doesnt make sense or is off base!
I'm having the same issue and decided to downgrade to spotipy==2.4.4 where I know it works for sure.
retrying ...1secs
I'm getting the same issue occasionally in my scripts. I'm wondering if it's an issue at Spotify's end?
All methods should be retrying but
playlist_add_items
doesn't seem to:Tried with 2.15.0 and 2.10.0