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

Added timeout to requests and let them run in their own threads #865

Closed
wants to merge 3 commits into from

Conversation

gerritzen
Copy link

Addresses issue #863.

  1. The timeout ensures that the program does not get stuck.
  2. By letting the requests run in their own threads, back pressure is significantly reduced when the connection is flakey.
  3. The lock is necessary since birdweather does not seem to accept more than one connection at a time. Without it I got occasional 503 errors.

A potential improvement to deal with longer network downtimes would be to not just let the threads die, but to either increase the timeout, or retry.

@jaredb7
Copy link

jaredb7 commented Apr 20, 2023

This is a slightly different approach to threading the Birdweather submissions than I took with more thorough exception handling and debugging output, but no setting of a timeout.

But very much the same goal with trying to relieve the main analysis thread so we don't get behind it stuck. So analysis can still happen in near real-time and birdweather submissions can happen alongside that.

I think the 503 may have just been because birdweather was having actually issues or because the soundscape id was invalid for some reason. Since I can have around 4 or more simultaneous submissions (4 species detected in the same recording) without any issues.

It'd be good combine efforts and have it queue retry for upto 30 seconds or something

@ehpersonal38
Copy link
Collaborator

@gerritzen could you respond to Jared's message above? I don't have access to BirdWeather and I don't know the ins and outs of how BirdNET-Pi interfaces with it, so I'll hopefully let you two figure out a solution here and tell me when you think it's all good to be merged in.

@jaredb7
Copy link

jaredb7 commented Apr 26, 2023

@ehpersonal38 I've been chipping at a implementing the retry system @ gerritzen mentioned, plus a back-off system to apply a wait time between attempts.

I've created PR #875 as a draft but it is essentially feature complete, tested and working....(works for me :P ). I'd still like sit on it for a few more days and see how it goes.

@gerritzen
Copy link
Author

Unfortunately, I have not found out the details from the API documentation. Maybe @timsterc can tell us more?

While the version of this PR runs a lot more stable for me, it still seems to get stuck once a week and I have not managed to figure out why/how/where.

@jaredb7 I think I don't understand how different detections belonging to the same soundscape are handled as of now, but

In this example only 1 x 2.2MB wav (gzipped) is uploaded to BirdWeather instead of 5x 2.2MB files (11MB total).

if I saw that correctly, it only gets uploaded once already. Please correct me if I'm wrong. I'll also add some comments to your PR directly.

@jaredb7
Copy link

jaredb7 commented May 3, 2023

Unfortunately, I have not found out the details from the API documentation. Maybe @timsterc can tell us more?

While the version of this PR runs a lot more stable for me, it still seems to get stuck once a week and I have not managed to figure out why/how/where.

Would it because the lock isn't released if there's a exception and relies on a successful soundscape post before it gets released?
Would that block on the next loop if the lock was still?

if I saw that correctly, it only gets uploaded once already. Please correct me if I'm wrong. I'll also add some comments to your PR directly.

Whoops, yeah you're right the original code handles this correctly. That functionality was lost when I played around threading and ran it for so long (since easter) that I forgot what the original behaviour was and really just created my own issue. The "More efficient BirdWeather Sound scape uploads" speil is a lie then and just fixing my own bug :(.

@gerritzen
Copy link
Author

Would it because the lock isn't released if there's a exception and relies on a successful soundscape post before it gets released?

Do you mean an exception in line 580? I think you're right. I'm not sure if with lock_birdweather: or finally: lock_birdweather.release() would be better here. I'm sure there's a lot of room for improvement still.

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.

None yet

3 participants