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

Add python webserver handlers and clients for starlette (fastapi) and aiohttp #1828

Merged
merged 6 commits into from
Jun 18, 2022

Conversation

timkpaine
Copy link
Member

@timkpaine timkpaine commented May 30, 2022

(Moving over from #1573)

This PR decouples perspective's python server code from tornado, and adds PerspectiveManager integration, examples, and client libraries for AIOHTTP and Starlette/FastAPI.

The existing perspective webserver integration was limited to tornado. Though it was possible to connect PerspectiveManager to other webservers (with relative ease), these integrations were not "out of the box" and required end users to be familiar with the perspective client/server architecture. Additionally, because support was limited to tornado, certain aspects of both the server integration, and the websocket client integration would require extensive copy-paste for a user to integrate with a different library.

Websocket Server Changes

We abstract away the PerspectiveManager integration from the PerspectiveTornadoHandler class into the new PerspectiveHandlerBase class. This class is essentially the non tornado-specific aspects of the original PerspectiveTornadoHandler class. Minor modifications are made:

  • Resolve an async race condition when sending a pre message before a binary message
  • Allow for binary messages to not have to immediately follow a pre message
    • pre messages are stored in a queue and coupled with binary messages when (and in the order) the binary messages are received

We then add two additional handlers implementing the PerspectiveHandlerBase class:

  • PerspectiveAIOHTTPHandler, which implements compatibility with aiohttp web servers
  • PerspectiveStarletteHandler, which implements compatibility with starlette/FastAPI web servers

Tests mirroring the tornado web server tests are added for both these handlers

Websocket Client Changes

On the client side, we decouple the tornado integration by adding three base classes:

  • PerspectiveWebsocketClient to handle the integration between an arbitrary read/write websocket interface and the PerspectiveClient class
  • PerspectiveWebsocketConnection, an abstract class to be implemented by the low level websocket connection details
  • Periodic, an abstract class to to be implemented according to the underlying event loop (asyncio / tornado) to handle keep alive (ping/pong) messages

The PerspectiveTornadoClient class is then modified to implement these interfaces, alongside the new PerspectiveTornadoWebsocketConnection and PerspectiveTornadoPeriodic classes.

We then introduce support for two new libraries:

  • PerspectiveAIOHTTPClient for integration with aiohttp websocket client library (and the corresponding PerspectiveAIOHTTPWebsocketConnection and AIOHTTPPeriodic
  • a test-only _PerspectiveStarletteTestClient for integration with the starlette testing websocket client (starlette does not provide a standalone non-test websocket client)

The former websocket function export is renamed to tornado_websocket, and aiohttp_websocket is added.

Examples

Leveraging the same frontend code as the existing tornado example, we add examples for FastAPI and aiohttp servers. The existing tornado libraries are renamed for consistency and to appear next to their sibling examples for discoverability.


@nickpholden feel free to test the included examples for starlette/fastapi and aiohttp

@tiangolo about to get an insanely fast datagrid 🚀

@finos finos deleted a comment from finos-cla-bot bot May 30, 2022
@finos finos deleted a comment from finos-cla-bot bot May 30, 2022
@finos finos locked as spam and limited conversation to collaborators May 30, 2022
@timkpaine timkpaine requested a review from sc1f May 30, 2022 20:45
@timkpaine timkpaine added this to the 2.0 milestone Jun 6, 2022
starlette server

Asyncify the perspective client, abstract away the websocket handler into its own class and utility classes

Implement starlette test client websocket wrapper, for testing purposes only. Websocket wrapper is synchronous so won't play nicely with perspective outside of a testing context

fix lint
@timkpaine timkpaine changed the title Add manager and examples for perspective hosted in starlette Add python webserver handlers and clients for starlette (fastapi) and aiohttp Jun 7, 2022
@timkpaine timkpaine marked this pull request as ready for review June 7, 2022 03:26
@timkpaine timkpaine requested a review from texodus June 7, 2022 03:26
@timkpaine timkpaine force-pushed the starlette-ws branch 8 times, most recently from 47f02dd to d241f50 Compare June 8, 2022 16:08
@texodus texodus removed this from the 2.0 milestone Jun 9, 2022
@timkpaine timkpaine force-pushed the starlette-ws branch 6 times, most recently from 2d8345e to bc731f8 Compare June 13, 2022 15:49
@timkpaine timkpaine force-pushed the starlette-ws branch 2 times, most recently from ce51302 to 67fb17e Compare June 15, 2022 19:33
example, add aiohttp handler tests, migrate tornado tests to
full async/await
@timkpaine
Copy link
Member Author

Going to do a pass through now that all the tests are working (and cover some corner cases like locking) to see if we can eliminate the need for web server-specific handlers and just export 1, potentially with some type checks or flags

Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

This is a cool addition, thanks especially for updating the tests and the special attention paid to the chunking/locking/streaming use cases we discussed. A few comments I had -

  • It's worth IMO adding explicit docs on installing the optional dependencies, even though this may be basic Python developer knowledge.
  • As we add more framework integrations (?), I'm interested in any strategy we can come up with to minimize an explosion of "glue" code that just translates Perspective concepts to framework ones. PerspectiveManager was meant to be a core abstraction we could use to "invert" this problem - e.g., make a simple API for dealing with Perspective server and session state, and then integration with specific frameworks should be "obvious"; however, we still ended up with a TornadoHandler which we justified "as an example", then it just seemed pedantic to not export this also ... and now we export 3 such glue modules, all nearly identical.
  • We want to avoid an explosion of examples also - the ones in this PR are fine but if we end up adding more I'd like to see a consolidation of the python examples to show novel/interesting/exemplary use of the API, rather than just all possible permutations of framework/perspective.

"superstore-arrow": "^1.0.0"
},
"devDependencies": {
"@finos/perspective-webpack-plugin": "^1.4.0",
Copy link
Member

Choose a reason for hiding this comment

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

This entire devDependencies is superfluous I think - same goes for the other example projects added in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

oh nvm you already merged

Copy link
Member

Choose a reason for hiding this comment

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

"I copied something that was wrong" does not merit indemnity from review comments

@texodus texodus merged commit eae7dce into master Jun 18, 2022
@texodus texodus deleted the starlette-ws branch June 18, 2022 19:38
@texodus
Copy link
Member

texodus commented Jun 18, 2022

@timkpaine also please remember to drop merge commits before un-drafting (as per CONTRIBUTING.md), especially for a PR like this that was underdevelopment across many versions, it makes it very complex to bisect when problems arise as the bisect-er must keep multiple dev streams changes in mind at once; rebasing makes the committer resolve these before they are merged so they don't have to be done again post-mortem when there is an issue discovered.

@texodus texodus added the enhancement Feature requests or improvements label Jun 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants