Fix bug in coroutine scheduling. Add ByteStream implementation and simplify Python symbol visitor. #1633

crisidev · 2022-08-12T16:37:32Z

Description

Bug in coroutine scheduling:

In the previous iteration, the runtime crate aws-smithy-http-server-python
was exposing the python application implementation as a struct, which
was wrapped by the codegenerated App to allow to dynamically building
the router.

This caused scheduling of coroutines (handlers prefixed with async def)
to block becuse we were passing the Python eventloop of the parent
process that was stored pre-fork().

This commit changes the runtime PyApp to become a trait, allowing us to
dynamically build the router post-fork() and with the right event loop.

This change also allowed us to remove a bunch of unnecessary Arc(s).

Add ByteStream implementation

Implementation of a ByteStream type for Python that can be roughly used like this:

let b = await ByteStream.from_path("file.txt")
async for chunk in b:
    print(chunk)

Implement futures_core::stream::Stream for Python ByteStream wrapper.

The BytesStream implementation in python can now use a sync Mutex from
parking_lot because we are now using pyo3_asyncio::tokio::local_future_into_py()
to read a chunk, which supports !Send futures.

This allows to simply forward the implementation of Stream (poll_next()
and size_hint()) directly to our inner SDK ByteStream.

Simplify Python symbol visitor

Inherit from Symbol visitor and override just what is needed to swap Python complex
types.

Signed-off-by: Bigo [email protected]

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…mplify Python symbol visitor. * Bug in coroutine scheduling: In the previous iteration, the runtime crate aws-smithy-http-server-python was exposing the python application implementation as a struct, which was wrapped by the codegenerated App to allow to dynamically building the router. This caused scheduling of coroutines (handlers prefixed with async def) to block becuse we were passing the Python eventloop of the parent process that was stored pre-fork(). This commit changes the runtime PyApp to become a trait, allowing us to dynamically build the router post-fork() and with the right event loop. This change also allowed us to remove a bunch of unnecessary Arc(s). * Add ByteStream implementation Implementation of a ByteStream type for Python that can be roughly used like this: let b = await ByteStream.from_path("file.txt") async for chunk in b: print(chunk) Implement futures_core::stream::Stream for Python ByteStream wrapper. The BytesStream implementation in python can now use a sync Mutex from parking_lot because we are now using pyo3_asyncio::tokio::local_future_into_py() to read a chunk, which supports !Send futures. This allows to simply forward the implementation of Stream (poll_next() and size_hint()) directly to our inner SDK ByteStream. * Simplify Python symbol visitor Inherit from Symbol visitor and override just what is needed to swap Python complex types. Signed-off-by: Bigo <[email protected]>

github-actions · 2022-08-12T18:32:41Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-08-16T20:30:45Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-08-17T11:57:28Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

...in/software/amazon/smithy/rust/codegen/smithy/protocols/HttpBoundProtocolPayloadGenerator.kt

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/util/TypeConversionGenerator.kt

jdisanti · 2022-08-17T21:37:14Z

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/util/TypeConversionGenerator.kt

+ * with wrappers compatible with Python, without touching the original implementation coming from `aws-smithy-types`.
+ */
+class TypeConversionGenerator(private val model: Model, private val symbolProvider: RustSymbolProvider, private val runtimeConfig: RuntimeConfig) {
+    private fun findOldSymbol(shape: Shape): Symbol {


It seems like this is making assumptions about what the differences will be between the default symbol provider and the language bindings code generator's symbol provider. Is there a way to make this more future proof?

Good question. TBH I don't know how to make this more future proof as we are learning as we go here :)

jdisanti · 2022-08-17T21:39:00Z

rust-runtime/aws-smithy-http-server-python/Cargo.toml

-tracing = "0.1.35"
-tracing-subscriber = { version = "0.3.11", features = ["env-filter"] }
+tracing = "0.1.36"
+tracing-subscriber = { version = "0.3.15", features = ["env-filter"] }


Is this dependency on tracing-subscriber intentional? My understanding is that libraries should depend on tracing, and applications should depend on tracing-subscriber.

This dependency is needed here as this crate exposes a functionality used by the codegenerator to setup the subscriber to allow logging to flow through tracing for both Python and Rust.

The user of the Python version does not have easy access to Rust functionalities such has tracing, so the logging.rs file is handling it for them.

In short this runtime crate provide most of the application for the Python server. The rest is code-generated.

github-actions · 2022-08-18T11:34:02Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2022-08-18T12:31:19Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

weihanglo

Amazing patch! Especially the refactor of PyApp trait becomes footgun-proof. Thank you for working on this huge change!

rust-runtime/aws-smithy-http-server-python/src/types.rs

rust-runtime/aws-smithy-http-server-python/examples/pokemon_service.py

rust-runtime/aws-smithy-http-server-python/Cargo.toml

...otlin/software/amazon/smithy/rust/codegen/server/python/smithy/PythonServerSymbolProvider.kt

weihanglo

Just did a simple load testing against the streaming endpoint /radio of pokemon server with 10 workers. It didn't crash and the response time seems reasonable (at least not blocked). I am going to approve this.

Connection Times (ms)
Server Software:
Server Hostname:		127.0.0.1
Server Port:		13734
Document Path:		/radio
Document Length:		Variable
Concurrency Level:		100
Time taken for tests:		421.215 seconds
Complete requests:		1000
Failed requests:		0
Total transferred:		3023157030 bytes
HTML transferred:		3023061030 bytes
Requests per second:		2.37
Transfer rate:		7009.01 kb/s received
	min	avg	max
Connect:	0	3	14
Processing:	3715	38975	96078
Total:	3715	38978	96092

github-actions · 2022-08-18T18:15:33Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

crisidev · 2022-08-19T16:07:42Z

@jdisanti I have fixed all the comments apart from the one about making the TypeConversionGenerator more future-proof, since I don't have a good answer :(

github-actions · 2022-08-19T16:38:59Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)

A new doc preview is ready to view.

crisidev added bug Something isn't working enhancement New feature or request server Rust server SDK labels Aug 12, 2022

Remove leftovers from previous commit

4e48eeb

crisidev mentioned this pull request Aug 12, 2022

[WIP] Generate server implementation for pure Python business logic #1367

Open

34 tasks

crisidev added 3 commits August 12, 2022 18:13

Fix RustDoc

0fbbbce

Enable streaming for Python \0/

b60448b

Add aiohttp pypi dependency for bytestrem Python example

a03f346

crisidev marked this pull request as ready for review August 15, 2022 11:26

crisidev requested review from a team as code owners August 15, 2022 11:26

crisidev changed the title ~~[WIP] Fix bug in coroutine scheduling. Add ByteStream implementation and simplify Python symbol visitor.~~ Fix bug in coroutine scheduling. Add ByteStream implementation and simplify Python symbol visitor. Aug 15, 2022

Merge branch 'main' into crisidev/oxypi-fix-asyncio-and-bytestreams

e22bde7

Merge branch 'main' into crisidev/oxypi-fix-asyncio-and-bytestreams

2096c8f

crisidev added needs-sdk-review needs-server-review labels Aug 17, 2022

jdisanti reviewed Aug 17, 2022

View reviewed changes

crisidev added 2 commits August 18, 2022 11:51

Merge branch 'main' into crisidev/oxypi-fix-asyncio-and-bytestreams

aac9428

Fix suggestion from PR

0fd41f4

Merge branch 'main' into crisidev/oxypi-fix-asyncio-and-bytestreams

4cca9ac

weihanglo reviewed Aug 18, 2022

View reviewed changes

Fix CR comments from Weihang

7f292d8

weihanglo approved these changes Aug 18, 2022

View reviewed changes

Merge branch 'main' into crisidev/oxypi-fix-asyncio-and-bytestreams

ece126c

Merge branch 'main' into crisidev/oxypi-fix-asyncio-and-bytestreams

46ae014

jdisanti approved these changes Aug 19, 2022

View reviewed changes

jdisanti removed the needs-sdk-review label Aug 19, 2022

crisidev merged commit 5455c4e into main Aug 19, 2022

crisidev deleted the crisidev/oxypi-fix-asyncio-and-bytestreams branch August 19, 2022 18:09

Velfi removed the needs-server-review label Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in coroutine scheduling. Add ByteStream implementation and simplify Python symbol visitor. #1633

Fix bug in coroutine scheduling. Add ByteStream implementation and simplify Python symbol visitor. #1633

crisidev commented Aug 12, 2022 •

edited

Loading

github-actions bot commented Aug 12, 2022

github-actions bot commented Aug 16, 2022

github-actions bot commented Aug 17, 2022

jdisanti Aug 17, 2022

crisidev Aug 18, 2022

jdisanti Aug 17, 2022

crisidev Aug 18, 2022 •

edited

Loading

crisidev Aug 18, 2022

github-actions bot commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

weihanglo left a comment

weihanglo left a comment

github-actions bot commented Aug 18, 2022

crisidev commented Aug 19, 2022

github-actions bot commented Aug 19, 2022

Fix bug in coroutine scheduling. Add ByteStream implementation and simplify Python symbol visitor. #1633

Fix bug in coroutine scheduling. Add ByteStream implementation and simplify Python symbol visitor. #1633

Conversation

crisidev commented Aug 12, 2022 • edited Loading

Description

Bug in coroutine scheduling:

Add ByteStream implementation

Implement futures_core::stream::Stream for Python ByteStream wrapper.

Simplify Python symbol visitor

Checklist

github-actions bot commented Aug 12, 2022

github-actions bot commented Aug 16, 2022

github-actions bot commented Aug 17, 2022

jdisanti Aug 17, 2022

Choose a reason for hiding this comment

crisidev Aug 18, 2022

Choose a reason for hiding this comment

jdisanti Aug 17, 2022

Choose a reason for hiding this comment

crisidev Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

crisidev Aug 18, 2022

Choose a reason for hiding this comment

github-actions bot commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 18, 2022

crisidev commented Aug 19, 2022

github-actions bot commented Aug 19, 2022

crisidev commented Aug 12, 2022 •

edited

Loading

crisidev Aug 18, 2022 •

edited

Loading