async runner #1352

technillogue · 2023-10-28T00:21:09Z

stacked PR on top of #1350 as well as #1355, this similarly only takes steps to eventually allow concurrent predictions. this PR starts with changing multiprocessing.pool.AsyncResult to asyncio.Task, and propagates the necessary async changes

have runner return asyncio.Task instead of AsyncFuture
making tests async

nickstenning · 2023-10-31T12:05:15Z

I started rebasing this but without context it quickly turned into a bigger job than I hoped. Would you mind rebasing this on main so it's reviewable in its own right?

technillogue · 2023-10-31T15:19:36Z

(did you mean something else about rebasing?)

nickstenning · 2023-11-06T14:22:43Z

(did you mean something else about rebasing?)

Yes! Your description here says:

stacked PR on top of #1350 as well as #1355,

Currently this PR is for a set of commits that we'd not want to merge to main without a rebase. Please rebase this branch on main so that it's a reviewable unit. Merging main achieves the same overall diff but it doesn't allow this PR to be reviewed commit by commit.

technillogue · 2023-11-06T18:21:29Z

had to brush up on how to rebase but once I remembered --onto I had it work without any conflicts. this is still stacked on #1355

nickstenning · 2023-11-06T20:02:23Z

python/tests/server/test_http.py

@@ -450,6 +450,7 @@ def test_prediction_idempotent_endpoint_conflict(client, match):
        json={"input": {"sleep": 1}},
        headers={"Prefer": "respond-async"},
    )
+    time.sleep(0.001)


Hmmmm. What's going on here?

when I added it the test would fail because the second request would come in too quickly, but I think maybe the limiter change from async routes fixes it

python/tests/server/test_http.py

python/cog/server/runner.py

python/cog/server/http.py

nickstenning · 2023-11-06T20:08:01Z

python/cog/server/http.py

            return

-        result = app.state.setup_result.get()
+        result = await app.state.setup_result


I think it might be clearer to do

Suggested change

result = await app.state.setup_result

result = app.state.setup_result.result()

here, because as written, this kind of implies that we might actually be waiting for it, which shouldn't happen.

Also do we need to handle cancellation of this task here?

hm, yes, both result and await can raise CancelledError (or errors from setup). that code path would happen if we get a healthcheck while shutting down after being told to shut down... is that SETUP_FAILED? there's not a shutting-down state

unlike await, .result() can raise InvalidStateError but that should never be the case after checking .done()

it's possible just propagating CancelledError and setup errors through to the healthcheck isn't the worst behavior, it should return 500 which seems reasonable

python/cog/server/runner.py

python/tests/server/test_runner.py

nickstenning

LGTM.

technillogue · 2023-11-21T18:31:52Z

is this good to merge then?

don't make Worker._wait async for this PR because it's not strictly necessary and makes testing more confusing Signed-off-by: technillogue <technillogue@gmail.com>

Signed-off-by: technillogue <technillogue@gmail.com>

Revert "review changes to tests and server" Revert "delete remaining runner thread code :)" Revert "make tests async and fix them" Revert "have runner return asyncio.Task instead of AsyncFuture" This reverts commit b002d54. This reverts commit 087f482. This reverts commit 6729d53. This reverts commit dc5ef44. Signed-off-by: technillogue <technillogue@gmail.com>

This reverts commit 828eee9.

* have runner return asyncio.Task instead of AsyncFuture * make tests async and fix them * delete remaining runner thread code :) * review changes to tests and server (reverts commit 828eee9)

* have runner return asyncio.Task instead of AsyncFuture * make tests async and fix them * delete remaining runner thread code :) * review changes to tests and server (reverts commit 828eee9) Signed-off-by: technillogue <technillogue@gmail.com>

technillogue force-pushed the syl/async-runner branch from ff833c7 to ba06ec2 Compare October 28, 2023 23:58

technillogue marked this pull request as ready for review October 30, 2023 05:23

technillogue requested review from nickstenning and mattt October 30, 2023 05:23

technillogue mentioned this pull request Oct 31, 2023

async webhooks #1359

Closed

technillogue force-pushed the syl/async-runner branch 5 times, most recently from 415d824 to 5b0b716 Compare November 6, 2023 17:52

technillogue force-pushed the syl/async-runner branch from 5b0b716 to a1f92b3 Compare November 6, 2023 18:44