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

POST playlist_add_items not retrying (HTTPError: 429 API rate limit exceeded) #577

Open
stephanebruckert opened this issue Sep 15, 2020 · 9 comments · Fixed by #596
Open
Labels

Comments

@stephanebruckert
Copy link
Member

stephanebruckert commented Sep 15, 2020

All methods should be retrying but playlist_add_items doesn't seem to:

import spotipy

sp = spotipy.Spotify(auth_manager=spotipy.SpotifyOAuth(scope='playlist-modify-public playlist-modify-private',
                                                       show_dialog=True))

playlist = sp.user_playlist_create(user_id, "test", False)

i = 0
while i < 1000:
    some_track_id = sp.search('test')['tracks']['items'][0]['uri']
    sp.playlist_add_items(playlist['id'], [some_track_id], 0)
    i += 1
    print(i)
493
494
495
496
497
498
499
500
501
HTTP Error for POST to https://api.spotify.com/v1/playlists/11k8jzmrwudvDnjnhfgP76/tracks returned 429 due to API rate limit exceeded
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/spotipy/client.py", line 244, in _internal_call
    response.raise_for_status()
  File "/Library/Python/3.8/lib/python/site-packages/requests/models.py", line 941, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 429 Client Error: unknown for url: https://api.spotify.com/v1/playlists/11k8jzmrwudvDnjnhfgP76/tracks?position=0

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "test_spotipy.py", line 21, in <module>
    sp.playlist_add_items('11k8jzmrwudvDnjnhfgP76', [some_track_id], 0)
  File "/usr/local/lib/python3.8/site-packages/spotipy/client.py", line 1015, in playlist_add_items
    return self._post(
  File "/usr/local/lib/python3.8/site-packages/spotipy/client.py", line 289, in _post
    return self._internal_call("POST", url, payload, kwargs)
  File "/usr/local/lib/python3.8/site-packages/spotipy/client.py", line 259, in _internal_call
    raise SpotifyException(
spotipy.exceptions.SpotifyException: http status: 429, code:-1 - https://api.spotify.com/v1/playlists/11k8jzmrwudvDnjnhfgP76/tracks?position=0:
 API rate limit exceeded, reason: None

Tried with 2.15.0 and 2.10.0

@Quizz1Cal
Copy link
Contributor

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?

@stephanebruckert
Copy link
Member Author

stephanebruckert commented Oct 10, 2020

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:

https://github.com/plamere/spotipy/blob/4bb42598e1e8e5411e36108af32bcc68acfa03aa/spotipy/client.py#L201

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:

https://github.com/plamere/spotipy/blob/4bb42598e1e8e5411e36108af32bcc68acfa03aa/spotipy/client.py#L290

https://github.com/plamere/spotipy/blob/4bb42598e1e8e5411e36108af32bcc68acfa03aa/spotipy/client.py#L295

PS: I just found this https://stackoverflow.com/a/35707701/1515819, that might be our solution!

@stephanebruckert
Copy link
Member Author

@Quizz1Cal @ritiek please review this fix if you get a chance #596

stephanebruckert added a commit that referenced this issue Oct 24, 2020
* Retry for POST, PUT and DELETE, fixes #577

* Lint
@Beahmer89
Copy link
Contributor

@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:

  • Adding retries to POSTs by default is not something should be added to this project. If a user needs that to happen in a special case that is up to them to code.
  • I do not think there is a need to is a need to whitelist them as the urllib2 library already has support for performing retries for PUT and DELETE. (However I did not test DELETE). I have tested PUT requests with the custom session included in this and I believe it to be executing the retries and backoffs appropriately. Did anyone test that before hand for those methods before the change was implemented and confirm that they were not executing the retry?
  • Changing the error code from 599 -> 429 is incorrect. Spotipy is not doing anything to limit the number of requests being sent or trying to deal with too many requests. It retried the request multiple times and it didnt succeed because of network error. I am open to changing it but I believe a 500 error is better suited for the response.

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.

@stephanebruckert
Copy link
Member Author

stephanebruckert commented Oct 28, 2020

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 More granular retry support urllib3/urllib3#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.

  • fair point, PUT is already part of the default, but PATCH / POST / DELETE aren't. Anyway, I wanted to "allow all", so probably we could update it to be method_whitelist=false? if we agree on the point above (only retrying idempotent 429s)

  • yeah I initially removed 599 because I didn't know where it came from but you are right, it's not always a 429 but one of 429, 500, 502, 503, 504. What do you think about showing the last status code we got from Spotify?

Great to discuss this!!

@stephanebruckert
Copy link
Member Author

stephanebruckert commented Oct 28, 2020

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

@Beahmer89
Copy link
Contributor

Beahmer89 commented Oct 29, 2020

@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:

  • I like the idea of using of 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 class
    • I believe these are used here to check the response and issue a sleep before retrying when urllib3 makes the request.

Also just to clear some things up in the comment with bullet points:

  • I believe PUT, DELETE, and GET are all apart of the default which is used in the allowed_methods param if not specified. This param controls what methods are retried.
    • I further did a test with DELETE and it respects the backoff. It also makes sense that PATCH is not apart of the default and we shouldnt include that be default either because it is not idempotent either.

So where does this leave us?

  • I think that the class should be able to work how we want it to without the changes recently implemented.
  • Originally this issue was opened because it wasnt playing nice with POST, which is to be expected. I havent tested it yet, but I bet if you would change the 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.
    • If that works then maybe it might be worth adding the 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.
      • Although I am mixed about this as I feel like it instilling bad practices, but urllib3 does allow it. Although I would think it is more for the ability for SOAP requests where POST is used for literally everything. What are your thoughts?
  • 599 I like your idea of returning the last status_code of the response before it hit he max retries. We should make sure that if there is no status code we have some default though.

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!

@Mokson
Copy link

Mokson commented Nov 9, 2020

I'm having the same issue and decided to downgrade to spotipy==2.4.4 where I know it works for sure.

retrying ...1secs

@shillshocked
Copy link

I'm getting the same issue occasionally in my scripts. I'm wondering if it's an issue at Spotify's end?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants