spotify2tidal / spotify_to_tidal

A command line tool for importing your Spotify playlists into Tidal
GNU Affero General Public License v3.0
241 stars 44 forks source link

Too many requests - blocked by Tidal's Cloudfront #20

Closed Kafkamorph closed 1 month ago

Kafkamorph commented 1 year ago

Any chance to implement some kind of throttling?

Getting a lot of these: "429 Client Error: Too Many Requests for url: https://api.tidal.com/v1/search?sessionId=XXX", 'X-Cache': 'Error from cloudfront' ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response')) occurred, retrying 2 times

and then the script just hangs and the playlist created on Tidal has 0 tracks.

timrae commented 1 year ago

Ahh yeah that would be a useful addition. I'm not sure when I'd be able to get around to it, but the ratelimit package would probably be a good path forward if someone would like to make a PR

Kafkamorph commented 1 year ago

That would be an elegant solution indeed.

For now, my not-elegant-at-all alternative was to reduce subprocesses from 50 to 5: _tidal_tracks = call_async_with_progress(tidal_search, spotify_tracks, task_description, config.get('subprocesses', 5), tidal_session=tidalsession)

I know it's ugly and slows down everything, but it seems to work, as Tidal's Cloudfront is no longer complaining.

tehkillerbee commented 1 year ago

@Kafkamorph This is a problem that I have experienced too when using the Tidalapi to update a local Mopidy playlist. Perhaps these improvements would make sense to add directly to the python-tidal api?

While it might be a hack, it will at least give a more predictable behaviour.

RobinHirst11 commented 2 months ago

That would be an elegant solution indeed.

For now, my not-elegant-at-all alternative was to reduce subprocesses from 50 to 5: _tidal_tracks = call_async_with_progress(tidal_search, spotify_tracks, task_description, config.get('subprocesses', 5), tidal_session=tidalsession)

I know it's ugly and slows down everything, but it seems to work, as Tidal's Cloudfront is no longer complaining.

to be fair, this is the best solution, id probably close this issue and suggest slowly increasing the number until you get to a sweet spot, id recommend halving it each time, so if 50 doesn't work, go to 25, 25 doesn't, try 12. keep going until it DOES work, then work your way up. for example, if 12 works, just do (12+25)/2. you get the gist

xtarlit commented 2 months ago

For now, my not-elegant-at-all alternative was to reduce subprocesses from 50 to 5:

In my case, I had to reduce it to just 1 or else it would get blocked after about 600 songs.

BlueDrink9 commented 1 month ago

https://pypi.org/project/backoff/

Great library for this purpose. Don't have time to figure out exactly where to place the decorator, but it'd replace the existing backoff code.

Also check out https://pypi.org/project/ratelimit/ if you want to ratelimit at the client rather than waiting for the tidal API to return an error

RobinHirst11 commented 1 month ago

https://pypi.org/project/backoff/

Great library for this purpose. Don't have time to figure out exactly where to place the decorator, but it'd replace the existing backoff code.

Also check out https://pypi.org/project/ratelimit/ if you want to ratelimit at the client rather than waiting for the tidal API to return an error

thanks for this, im planning on incoroprating it into my new version

timrae commented 1 month ago

Sounds like a good idea, though it might be a bit tricky to get rate limiting working properly due to the use of the multiprocessing module. It would be a lot easier to do this if we could use Async await syntax with the tidal API... I would strongly recommend to submit a prototype for review to get feedback from us before going too far down a given route

BlueDrink9 commented 1 month ago

fyi iirc both ratelimit and backoff are thread-safe

timrae commented 1 month ago

We use multiprocessing here though, not multi-threading... So I suspect they will not work. I think we can probably just implement some basic rate-limiting with multiprocessing.Semaphore though, I'll have a go tomorrow as this error has been becoming more and more of a problem for me recently

timrae commented 1 month ago

OK this should be working now in #43, I've added a new rate limit configuration parameter. Please give it a try and let me know how it goes for you (and if the default parameters work). I'm hopefully going to add one more performance optimisation to that PR.

BlueDrink9 commented 1 month ago

Out of curiosity, why use multiprocessing rather than multithreading? This is IO-bound rather than cpu-bound, isn't it?

timrae commented 1 month ago

Yes, it's io bound. I'm currently investigating if there's a performance gain to be had from using threads instead of processes. It's been a few years since I implemented that part, so I don't recall if there was some issue I ran into with the GIL, or if I just went straight to multiprocessing because it was guaranteed to work...

On Sun, 26 May 2024, 9:12 pm Silico_Biomancer, @.***> wrote:

Out of curiosity, why use multiprocessing rather than multithreading? This is IO-bound rather than cpu-bound, isn't it?

— Reply to this email directly, view it on GitHub https://github.com/spotify2tidal/spotify_to_tidal/issues/20#issuecomment-2132368171, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVQBYQRQV4THZXQ2N6HUJLZEIX2FAVCNFSM6AAAAAAXO574L6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZSGM3DQMJXGE . You are receiving this because you modified the open/close state.Message ID: @.***>

BlueDrink9 commented 1 month ago

In that case, I would suggest switching to multithreading (eg with futures, or you could abstract it with asyncio) next time you go in for a big refactor.

All my reading when I researched a similar API-driven task states multithreading is a better choice when io-bound. In general it simplifies execution and works with more libraries. Can't remember the exact details for why, but probably worth reading up on. Probably easier/more performant to sync things with shared memory rather than relying on locks?

I'd give you some code snippets but they're all work code, I haven't used it recently for OSS

timrae commented 1 month ago

Thanks yeah I'm just finishing up a big refactor right now. I've got an implementation with asyncio working, and it's looking pretty good so far AFAICT. You can test it / code review it now at #43 :)