Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
anyio integration #1157
anyio integration #1157
Changes from 44 commits
d06f40c
75310b5
a660684
42b83cb
9870a1f
6997eb9
e1c2adb
de84b4a
e91ec33
7e2cd46
03e312e
fd4569e
d785513
58d5331
268547d
57b2f79
444a3ac
4d31a60
681c348
01dd813
9f76d42
5c8818d
f0e4cd8
376f9db
34da2b4
31cc220
73590aa
4192bf7
3a4b472
cc3be48
9b6e722
b8c43cf
d51d5ff
82431f4
2504237
87c614c
cf915bc
72586ba
1800f7a
89e2dae
f62a2ec
edba5dc
60d95e1
fc60420
27283aa
3cce6a9
cbc2e68
c4d49a7
4dd8c5d
dde5079
df53965
6e0f05f
3a359e3
5c77b7d
390b7a1
e420181
6a3f94d
0c225a3
5667a4b
19685db
4e43146
a1ceb35
63cfcb9
efbe6a1
6208ca5
8e6115b
e0c9967
27ec6f7
2b9dd22
643d107
d0ca3f2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we link this to the anyio documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 6208ca5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be interesting to know what quart chooses to do wrt. a request/response-wide task group. Eg. if you're running under trio do they provide an anyio TaskGroup or a trio Nursery?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've explored this elsewhere, and for an application lifespan to run background tasks with structured concurrency, it'd be better to have an app-wide task group from which all other tasks are spawned:
This then allows you to write a lifespan generator or startup/shutdown events such as:
The only issue is, what happens if a task in that task group raises an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lifespan task is cancelled
and the server is shutdown. For the apps I write this is the desired behaviour.Other users of Starlette might want a task wrapper that catches Exceptions and logs them instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's a little demo of what quart_trio does:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graingert that's interesting. A similar test app in Starlette would return "/" without error but raise an exception on the server, using uvicorn.
Using hypercorn, it crashes and never responds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uSpike I think this task_group should be scoped to the dispatch, and not attached to the app: uSpike#1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graingert yep that makes sense to me, I'll get that in today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something we need to be aware of here, that's kinda silly, but also is at least worth pointing out...
This is on the critical path, so it'll have some impact on the benchmarking.
Now, I'm acutely aware that the micro-benchmarking is a big ol' pile of nonsense. But also it does, tediously, lead some folks into particular evaluations.
When I started Starlette one of the design arguments that I felt needed to be knocked down was that integrated server+application frameworks would be necessarily "faster" than frameworks with a clean server / application interface split. In this case ASGI.
To that extent it was important that Starlette ought to be able to equal sanic or aiohttp when evaluated with microbenchmarks, however tedious that might be.
Adoptiong this would likely change that.
Which might well be okay, but it's something we need to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can provide some benchmarks for this conversation. Are there any that you recommend as meaningful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomchristie I created a branch here: https://github.com/uSpike/FrameworkBenchmarks/tree/starlette-anyio
See results: https://www.techempower.com/benchmarks/#section=test&shareid=c92a1338-0058-486c-9e47-c92ec4710b6a&hw=ph&test=query&a=2
There is a performance penalty to using anyio. The weighted composite score is 173 vs 165, so a 4.5% drop in performance.
I'll add this result to the top comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, the multiple queries test was ~25% faster with anyio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran the benchmark again against sanic and aiohttp, see https://www.techempower.com/benchmarks/#section=test&shareid=a2023abc-ee86-458f-a0e9-849b5c9f282f&hw=ph&test=composite&a=2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only see starlette and starlette-anyio. How do I run these benchmarks locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agronholm if you click the other test tabs they should show up.
You can run locally with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to get reproducible results on my local machine. I ran it again now with @graingert changes to make task groups per-request, and now starlette-anyio is much closer to starlette performance: https://www.techempower.com/benchmarks/#section=test&shareid=d1edb7b5-09f0-43c0-a4cb-cca51f87801c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
task groups were per request before, they were just attached to the app