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

Try msgspec #212

Closed
wants to merge 8 commits into from
Closed

Try msgspec #212

wants to merge 8 commits into from

Conversation

goodboy
Copy link
Owner

@goodboy goodboy commented Jun 2, 2021

First draft trying out msgspec over msgpack-python.

We currently can't easily decode streams since there's no api support, see:
jcrist/msgspec#27, see the updates; we just need to implement our own framing with a bytes len prefix 🥳

The bulk of speed improvements are supposed to come from decoding + validation so we're not really seeing benefits from either of those yet 😂

TODO:

  • add nested structs for our ipc protocol for Spec out the async-ipc protocol #36
  • offer a way to specify which serialization lib is used at runtime startup (likely much like the start_method kwarg to open_root_actor()), maybe serialization_method?
  • fix up the remaining failing tests which are shipping defaultdicts for some reason
  • appease mypy stuff
  • potentially offer msgpack-numpy as the optional dep if we end up liking the performance here?
  • oh right, pin to python 3.8+ (since msgspec requires it) and we might as well sine we're onto a new alpha release anyway
  • document msgspec use in the readme

@goodboy
Copy link
Owner Author

goodboy commented Jun 11, 2021

Gah next hassle thing is not supporting native tuple serialization.

Opened another issue: jcrist/msgspec#30.

@goodboy goodboy changed the base branch from wip_fix_asyncio_gen_streaming to stdstream_clobber_fix June 11, 2021 20:58
@goodboy
Copy link
Owner Author

goodboy commented Jun 11, 2021

Aight, this finally gets us working msgspec serialization 🥳.

There's a small legnth-prefix framing as part of the new MsgspecStream type.

Also had to force cast a few more spots since there's no tuple-by-default support.

Can only really use an encoder currently since there is no streaming api
in `msgspec` as of currently. See jcrist/msgspec#27.

Not sure if any encoding speedups are currently noticeable especially
without any validation going on yet XD.

First experiments toward #196
Add a `tractor._ipc.MsgspecStream` type which can be swapped in for
`msgspec` serialization transparently. A small msg-length-prefix framing
is implemented as part of the type and we use
`tricycle.BufferedReceieveStream` to handle buffering logic for the
underlying transport.

Notes:
- had to force cast a few more list  -> tuple spots due to no native
  `tuple`decode-by-default in `msgspec`: jcrist/msgspec#30
- the framing can be understood by this protobuf walkthrough:
  https://eli.thegreenplace.net/2011/08/02/length-prefix-framing-for-protocol-buffers
- `tricycle` becomes a new dependency
This change some super old (and bad) code from the project's very early
days. For some redic reason i must have thought masking `trio`'s
internal stream / transport errors and a TCP EOF as `StopAsyncIteration`
somehow a good idea. The reality is you probably
want to know the difference between an unexpected transport error
and a simple EOF lol. This begins to resolve that by adding our own
special `TransportClosed` error to signal the "graceful" termination of
a channel's underlying transport. Oh, and this builds on the `msgspec`
integration which helped shed light on the core issues here B)
@goodboy goodboy mentioned this pull request Jul 1, 2021
7 tasks
@goodboy
Copy link
Owner Author

goodboy commented Jul 4, 2021

Dropping this in favor of #214 which branches from master and includes a simpler git history.

@goodboy goodboy closed this Jul 4, 2021
@goodboy goodboy deleted the try_msgspec branch July 4, 2021 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IPC and transport messaging messaging patterns and protocols streaming
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant