blib-la / runpod-worker-comfy

ComfyUI as a serverless API on RunPod
GNU Affero General Public License v3.0
301 stars 205 forks source link

refactor: default check_server method arguments #33

Closed angelolamonaca closed 6 months ago

angelolamonaca commented 6 months ago

Motivation

Default check_server method arguments are wrong (inverted)

Issues closed

angelolamonaca commented 6 months ago

@pixelass Hi :) Hope you appreciate my contribution. The worker right now is checking the connection 500 times every 50ms by default.

pixelass commented 6 months ago

@angelolamonaca thank you for your contribution. I think the intention is to check very fast and often to get a fast connection.

@TimPietrusky Do we need/want this? Should this be configurable?

angelolamonaca commented 6 months ago

Hi @pixelass, thanks for your reply and thanks for the hard work you and the other guys are doing. This repo was a life saver for me :)

I think the original implementation was checking 50 times with an interval of 500ms (in fact the default values of the check_server method are correct: check_server(url, retries=50, delay=500) but we override it by default.

Logically is not an issue, it still checks for 25 seconds, but I'm pretty sure that checking every 50ms is not intentional and should be fixed

TimPietrusky commented 6 months ago

@angelolamonaca thanks for taking your time to look into this.

If ComfyUI is available after 200ms and we wait for another 300ms, then the overall cold-start time of the serverless API is increased by 300ms.

But you are right, the default values of check_server are wrong. Initially we used them like this, but during testing in production we saw, that it's better to decrease the delay and increase the repeats to have a faster cold start.

So what we could do is to update the default values of check_server or just close the PR.

angelolamonaca commented 6 months ago

Hi @TimPietrusky , thanks for taking a look and explain why it's like this :) I would really appreciate to have this PR merged, this would be my first code contribution in a public repository 🎉

I've just updated the commit and changed the default values of check_server as you suggested

TimPietrusky commented 6 months ago

@angelolamonaca thank you!

And please ⭐ https://github.com/blib-la/captain