Python: map Python middlewares to Tower layers #1871

unexge · 2022-10-18T15:19:01Z

Map pure Python middlewares to Tower layers so they can have same flexibility as pure Rust middlewares.

Pure Python middlewares will look like this:

@app.middleware
async def middleware(
    request: Request, 
    next: Callable[[Request], Awaitable[Response]]
) -> Response:
    # Do something with the `request`
    request.headers["X-Request-Id"] = ...

    # Call the handler or the next middleware in the stack
    response = await next(request)

    # Do something with the `response`
    response.headers["X-Server"] = ...

    return response

so they will have total control over the execution flow. They can modify the request, return a response without calling the handler, apply timeout to the handler, modify the response returned by the handler etc.

TODOs

~~Provide a way to access to the Context~~
Modifiers for the Request
Modifiers for the Response

/cc @chadac @teruokun @PC-LoadLetter

github-actions · 2022-10-18T15:32:04Z

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-10-21T15:05:51Z

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-10-21T15:51:28Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-10-24T15:41: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-10-24T17:18:57Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-10-25T12:42:49Z

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-10-25T15:18:36Z

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-10-25T17:08:10Z

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.

...are/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonApplicationGenerator.kt

LukeMathWalker · 2022-10-26T10:14:58Z

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

-def main():
+def main() -> None:
    app.run(workers=1)


Do we need a separate main function? 🤔

I think it is not required but having a explicitly defined entry point is kinda useful when you want to setup your service via a supervisor or something

LukeMathWalker · 2022-10-26T10:16:22Z

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

+        let middleware_err = Python::with_gil(|py| other.to_object(py).extract::<Self>(py));
+        match middleware_err {
+            Ok(err) => err,
+            Err(_) => Self::newpy(other.to_string(), None),


Here we probably want a log statement capturing the details of the exception that we failed to extract since it didn't match the one we expected.

The error on the Err(_) variant will always be 'TypeError' object cannot be converted to 'MiddlewareException' here because it is a conversion error and the real error will be logged at https://github.com/awslabs/smithy-rs/blob/e12136563d734e3c1ea40d96a1705d3b1f5d9cbe/rust-runtime/aws-smithy-http-server-python/src/middleware/layer.rs#L124

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

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

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

rust-runtime/aws-smithy-http-server-python/src/middleware/pytests/layer.rs

github-actions · 2022-10-27T13:03:33Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-10-27T17:10:12Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-10-28T12:00:56Z

A new generated diff is ready to view.

A new doc preview is ready to view.

LukeMathWalker · 2022-11-09T16:11:03Z

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

+    fn keys(&self) -> PyResult<Vec<Self::Key>>;
+    fn values(&self) -> PyResult<Vec<Self::Value>>;


I'm wondering if this is a footgun.
In Python you'd expect iterating over an existing collection to be "free", while our implementation triggers a deep clone of the entire map.

I.e. as a Python developer, I wouldn't expect this:

for elem in map: assert elem in elems

to trigger a deep clone of all elements in the map.

I think this is a good first step, but me and Burak already talked about understanding what is needed to prevent the deep clone and borrow the map so that it becomes cheaper. I am not even sure if this is possible with the current PyO3 support.

I don't we can if we are stuck with using HeaderMap as is.
This would be less of an issue If both the header names and their values happened to be behind smart pointers.

Btw, if you are already tracking this issue feel free to merge and we can revisit it later. But perhaps worth calling it out in the docs for those methods.

👍 I've added comments to the related methods: c8bd368.

This would be less of an issue If both the header names and their values happened to be behind smart pointers.

We are converting them to Strings while crossing to Python: https://github.com/awslabs/smithy-rs/blob/c8bd36852b3be76914b5637f855a1e9e7a972571/rust-runtime/aws-smithy-http-server-python/src/middleware/header_map.rs#L56-L66

… function

…and Python

…dlewares

Co-authored-by: Luca Palmieri <[email protected]>

…ur CI version)

Co-authored-by: Matteo Bigoi <[email protected]>

github-actions · 2022-11-10T12:03:30Z

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.

…es clones

github-actions · 2022-11-10T15:39:43Z

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 the python-server Python server SDK label Oct 19, 2022

unexge force-pushed the unexge/map-python-middlewares-to-tower-layers branch 2 times, most recently from 1b08bf5 to 29fe37d Compare October 21, 2022 14:53

unexge force-pushed the unexge/map-python-middlewares-to-tower-layers branch from a1cab3e to 70c1826 Compare October 24, 2022 15:29

unexge force-pushed the unexge/map-python-middlewares-to-tower-layers branch from c3a8df5 to 6154bc2 Compare October 25, 2022 12:30

unexge force-pushed the unexge/map-python-middlewares-to-tower-layers branch from 7340d64 to a81def9 Compare October 25, 2022 16:55

unexge marked this pull request as ready for review October 25, 2022 16:55

unexge requested a review from a team as a code owner October 25, 2022 16:55

LukeMathWalker reviewed Oct 26, 2022

View reviewed changes

...are/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonApplicationGenerator.kt Outdated Show resolved Hide resolved

LukeMathWalker reviewed Oct 26, 2022

View reviewed changes

...are/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonApplicationGenerator.kt Outdated Show resolved Hide resolved

LukeMathWalker reviewed Oct 26, 2022

View reviewed changes

...are/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonApplicationGenerator.kt Outdated Show resolved Hide resolved

LukeMathWalker reviewed Oct 26, 2022

View reviewed changes

rust-runtime/aws-smithy-http-server-python/src/middleware/error.rs Outdated Show resolved Hide resolved

LukeMathWalker reviewed Oct 26, 2022

View reviewed changes

rust-runtime/aws-smithy-http-server-python/src/middleware/handler.rs Outdated Show resolved Hide resolved

LukeMathWalker reviewed Oct 26, 2022

View reviewed changes

rust-runtime/aws-smithy-http-server-python/src/middleware/handler.rs Outdated Show resolved Hide resolved

LukeMathWalker reviewed Oct 26, 2022

View reviewed changes

rust-runtime/aws-smithy-http-server-python/src/middleware/layer.rs Outdated Show resolved Hide resolved

LukeMathWalker reviewed Oct 26, 2022

View reviewed changes

rust-runtime/aws-smithy-http-server-python/src/middleware/pytests/layer.rs Show resolved Hide resolved

unexge force-pushed the unexge/map-python-middlewares-to-tower-layers branch from 9047f46 to e121365 Compare October 28, 2022 16:23

LukeMathWalker reviewed Nov 9, 2022

View reviewed changes

unexge and others added 22 commits November 10, 2022 11:51

Python: map Python middlewares to Tower layers

e0be364

Make middleware layer infallible

58f18ae

Use message and status code from PyMiddlewareException

67dafeb

Introduce FuncMetadata to represent some information about a Python…

5437e73

… function

Improve middleware errors

2b0d4f6

Add missing copyright headers

ec638e6

Allow accessing and changing request body

b084ba2

Allow changing response

b19e6f3

Add some documentation about moving data back-and-forth between Rust …

ed09a19

…and Python

Add mypy to Pokemon service and update typings and comments for mid…

8e4cc14

…dlewares

Add or update comments on the important types

1f92ddf

Add Rust equivalent of collections.abc.MutableMapping

5700cef

Add PyHeaderMap to make HeaderMap accessible from Python

57b7c69

Apply suggestions from code review

330e222

Co-authored-by: Luca Palmieri <[email protected]>

Improve logging

c0208b8

Add RichPyErr to have a better output for PyErrs

63d522b

Better error messages for PyMiddlewareError variants

c1cdad5

Factor out repeating patterns in tests

02b5164

Preserve __builtins__ in globals to fix tests in Python 3.7.10 (o…

4e1cd23

…ur CI version)

Export RichPyErr to fix cargo doc error

f5c9a56

Apply suggestions from code review

cef64c4

Co-authored-by: Matteo Bigoi <[email protected]>

Add missing SPDX headers

385f133

unexge force-pushed the unexge/map-python-middlewares-to-tower-layers branch from 76b2e07 to 385f133 Compare November 10, 2022 11:51

Document that keys, values and items on PyMutableMapping caus…

c8bd368

…es clones

unexge merged commit 4f76e35 into main Nov 10, 2022

unexge deleted the unexge/map-python-middlewares-to-tower-layers branch November 10, 2022 16:22

unexge mentioned this pull request Nov 15, 2022

[Python] Support pure Python implementation of response middleware #1754

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: map Python middlewares to Tower layers #1871

Python: map Python middlewares to Tower layers #1871

unexge commented Oct 18, 2022 •

edited

Loading

github-actions bot commented Oct 18, 2022

github-actions bot commented Oct 21, 2022

github-actions bot commented Oct 21, 2022

github-actions bot commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

github-actions bot commented Oct 25, 2022

github-actions bot commented Oct 25, 2022

github-actions bot commented Oct 25, 2022

LukeMathWalker Oct 26, 2022

unexge Oct 28, 2022

LukeMathWalker Oct 26, 2022

unexge Oct 28, 2022

github-actions bot commented Oct 27, 2022

github-actions bot commented Oct 27, 2022

github-actions bot commented Oct 28, 2022

LukeMathWalker Nov 9, 2022

LukeMathWalker Nov 9, 2022

crisidev Nov 9, 2022

LukeMathWalker Nov 10, 2022

LukeMathWalker Nov 10, 2022

unexge Nov 10, 2022

github-actions bot commented Nov 10, 2022

github-actions bot commented Nov 10, 2022

		fn keys(&self) -> PyResult<Vec<Self::Key>>;
		fn values(&self) -> PyResult<Vec<Self::Value>>;

Python: map Python middlewares to Tower layers #1871

Python: map Python middlewares to Tower layers #1871

Conversation

unexge commented Oct 18, 2022 • edited Loading

TODOs

github-actions bot commented Oct 18, 2022

github-actions bot commented Oct 21, 2022

github-actions bot commented Oct 21, 2022

github-actions bot commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

github-actions bot commented Oct 25, 2022

github-actions bot commented Oct 25, 2022

github-actions bot commented Oct 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 27, 2022

github-actions bot commented Oct 27, 2022

github-actions bot commented Oct 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 10, 2022

github-actions bot commented Nov 10, 2022

unexge commented Oct 18, 2022 •

edited

Loading