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.

Sign up for GitHub

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

Jump to bottom

Optional msgspec support #214

Merged
merged 28 commits into from
Oct 6, 2021
Merged

Optional msgspec support #214

merged 28 commits into from
Oct 6, 2021

Conversation

goodboy
Copy link
Owner

@goodboy goodboy commented Jul 1, 2021 •
edited
Loading

Supersedes #212 (mostly for git history reasons) but adding msgspec as an optional dependency which is loaded as the transport backend TCPMsgspecStream if importable.

This also reworks TCP transport EOF handling to raise a new internal error type TransportClosed which now handled around the core message loop and inter-actor handshake.

This also add tricycle as a dep since it has a lot useful stuff anyway that we'll probably use and once we go full msgspec after dropping <= 3.8 python, it'll be necessary.

TODO list from #212:

  • fix up the remaining failing tests which are shipping defaultdicts for some reason as well as tuple assertion issues for channel uuids
  • add nested structs for our ipc protocol for Spec out the async-ipc protocol #36 deferring for now..
  • 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?
  • appease mypy stuff
  • potentially offer msgpack-numpy as the optional dep if we end up liking the performance here? was already dropped.
  • 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 goodboy requested a review from guilledk July 1, 2021 13:48
@goodboy
Copy link
Owner Author

goodboy commented Jul 2, 2021

So turns out those last CI runs never had msgspec installed 😂

This demonstrates 2 interesting outcomes

  • msgspec hasn't been tested against our test suite (which, shocker, it's completely broken for a variety of reasons)
  • apparently the master branch is already seeing problems on windows and some linux runs?
    • this is particularly odd because if you look at the runs the tests complete but the jobs are cancelled?
    • this means the CI failures we're seeing as part of Bidir streaming #209 might be false negatives?

Lots more work to do here but quick summary is I'm going to defer this support and pull out the transport stuff that's needed for the bi-dir streaming changes and put it as a separate smaller PR onto mainline.

@goodboy goodboy force-pushed the optional_msgspec_support branch from a25dfc7 to 94784d2 Compare July 2, 2021 15:25
@goodboy goodboy mentioned this pull request Jul 4, 2021
Closed
7 tasks
@goodboy goodboy force-pushed the optional_msgspec_support branch 2 times, most recently from 7f89f9a to c27b006 Compare September 5, 2021 22:28
@goodboy
Copy link
Owner Author

goodboy commented Sep 6, 2021

Welp, now all the non-msgspec tests are borked 😂

Will have to dig into it.

@goodboy goodboy mentioned this pull request Sep 8, 2021
Merged
@goodboy goodboy force-pushed the optional_msgspec_support branch from e8b2828 to 4e61fd1 Compare September 8, 2021 13:53
@goodboy goodboy force-pushed the optional_msgspec_support branch 2 times, most recently from 75eb4dd to fbc6905 Compare October 4, 2021 16:40
goodboy added 19 commits October 5, 2021 13:37
@goodboy
Cast to tuples for all uids explicitly
4079f02
@goodboy
Try out msgspec in our msgpack stream channel
dda0b22 
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
@goodboy
Add tricycle and msgspec deps
3771734
@goodboy
Always cast arbiter addr to tuple
e39ee3a
@goodboy
Add streaming decode support for msgspec
95e35f3 
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
@goodboy
Add our own "transport closed" signal
112117c 
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
Drop happy eyeballs inf delay
ecd8c4b
@goodboy
Accept transport closed error during handshake and msg loop
96b3f94
@goodboy
Pkg msgpec as optional dep, load transport type if importable
b64396f
@goodboy
Fix py version classifier
8375002
@goodboy
Mypy fixes to enforce uid tuple
aa08054
@goodboy
Add msgspec installs, drop py3.7
3eb4c6d
@goodboy
Fix log levels
3facfb6
@goodboy
Convert actor UIDs to hashable tuples
562419c 
`msgspec` sends python lists over the wire
(jcrist/msgspec#30) which is fine and dandy
but we use them as lookup keys so we need to be sure we tuple-cast
first.
@goodboy
Map broken stream errs to transport closed; msgspec seems to be racy
1ab495a
@goodboy
Cast defaultdict to dict for registry get
486e983
@goodboy
Call registry getter method in test
93726f1
@goodboy
Pin to latest and greatest msgspec
dbc4e3d
@goodboy
Ensure tuple for passed in arbiter addr
19d6885
@goodboy goodboy force-pushed the optional_msgspec_support branch from fbc6905 to 135459c Compare October 5, 2021 17:37
goodboy added 2 commits October 6, 2021 15:54
@goodboy
Add "message transport" structured sub-typing
c6dc96b 
In an effort to have some kind of more formal interface around the
transport layer, add a `MsgTransport` protocol type and use with
the channel composition of message streams. Start a little "key map"
of `(<codec>, <protocol>)` to `MsgTransport` types which can be
dynamically loaded. Add a `Channel.from_stream()` constructor thus
cleaning up the mangled logic that was in the constructor based on
inputs. Drop all the "auto reconnect" channel logic for now since
nothing is using it (internally) and it's likely it will need rework
once we bring in a protocol besides TCP.
@goodboy
Use from .from_stream() in TCP handler
b496e79
@goodboy goodboy merged commit 21e6055 into master Oct 6, 2021
@goodboy goodboy deleted the optional_msgspec_support branch October 6, 2021 21:01
@goodboy goodboy mentioned this pull request Nov 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guilledk guilledk Awaiting requested review from guilledk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@goodboy