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

make HTTP routes async #1355

Merged
merged 3 commits into from
Nov 6, 2023
Merged

make HTTP routes async #1355

merged 3 commits into from
Nov 6, 2023

Conversation

technillogue
Copy link
Contributor

@technillogue technillogue commented Oct 30, 2023

this doesn't change any logic, but as far as I understand sync routes use threads instead of the starlette event loop: https://fastapi.tiangolo.com/async/#path-operation-functions

…pear to use threads to handle requests

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

@nickstenning nickstenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this may not be as harmless as it looks.

There's a --threads argument to cog.server.http and I believe that it functions as a concurrency control for the synchronous prediction create endpoint.

At least some people rely on this behaviour so they can use the network socket's accept queue as a mechanism to queue up work. If this breaks that behaviour (i.e. those requests are immediately accepted and we serve a 409 instead) that may break those folks' expectations.

Is this change a requirement or just a nice-to-have for the batching/concurrent predictions work?

@technillogue
Copy link
Contributor Author

Do we have any specific examples of someone actually using that, or is hypothetical?

I think it will be required somewhere down the line, I put it up front here because it can be reviewed as an independent PR

@nickstenning
Copy link
Member

Yes, in that if I remember correctly, we broke it previously while working on Cog and had to put it back.

@technillogue
Copy link
Contributor Author

we could stick a timeout or something where the endpoint waits for the worker for a similar time as what the tcp timeout would have been? do you remember what exactly the use case was and why they didn't want to use retries instead?

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

@nickstenning nickstenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nice if we could have a test for this behaviour, but I'm not going to block this on that. I've tested it locally and it works well!

@technillogue technillogue merged commit 3517511 into main Nov 6, 2023
13 checks passed
@technillogue technillogue deleted the syl/async-http-routes branch November 6, 2023 18:40
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.

2 participants