-
Notifications
You must be signed in to change notification settings - Fork 24
Address high cpu usage with AWSCRTHTTPClient #573
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
Conversation
b24f494 to
b68df1b
Compare
| [project.optional-dependencies] | ||
| awscrt = [ | ||
| "awscrt>=0.23.10", | ||
| "awscrt~=0.28.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.
Note
Setting awscrt v0.28.2 is necessary since we need the change added in awslabs/aws-crt-python#688. Also, this was previously unbounded which has been fine for now, but constraining it to ~=0.28.2 is much safer for customers
| ): | ||
| # AsyncByteStream has async read method but is not iterable | ||
| while True: | ||
| chunk = await body.read(65536) # Read in 64KB chunks |
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.
Why this number in particular?
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 was a common buffer size I'd come across, however, I do see our default value is -1 which will read all available data at once.
smithy-python/packages/smithy-core/src/smithy_core/aio/interfaces/__init__.py
Lines 23 to 27 in d9b9e2b
| @runtime_checkable | |
| class AsyncByteStream(Protocol): | |
| """A file-like object with an async read method.""" | |
| async def read(self, size: int = -1) -> bytes: ... |
Do you prefer going with the default?
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.
Updated to use the default in 02e3e20. Let me know what you think
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 default is for the protocol - when you call .read on a file-like object, for instance, it just reads everything. We should read in chunks, i was just wondering why that size in particular. Just make it a constant or something that's configurable on the crt client and we'll be good.
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.
See response to other comment - we should still limit how much we read at a time from a file-like object, I just wanted to know why that number.
Summary
This PR integrates with the new aync CRT APIs introduced in awslabs/aws-crt-python#658 with the goal of addressing the high CPU usage reported in awslabs/aws-sdk-python#11.
The changes in this PR have been tested and successfully reduce CPU usage for examples such as the bidirectional nova_sonic.py. This change will be available in a new versions of
smithy-httpandaws-sdk-bedrock-runtimewith no changes to the user experience.Testing
gh pr checkout 573uv venv && source .venv/bin/activatemake installuv pip install aws-sdk-bedrock-runtime psutil pyaudio rxpython monitor_cpu.py bidirection.pyBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.