Skip to content

Add type annotations to most functions#35

Merged
goodboy merged 6 commits intomasterfrom
type_annotations
Sep 1, 2018
Merged

Add type annotations to most functions#35
goodboy merged 6 commits intomasterfrom
type_annotations

Conversation

@goodboy
Copy link
Copy Markdown
Owner

@goodboy goodboy commented Aug 20, 2018

This is purely for documentation purposes for now as it should be
obvious a bunch of the signatures aren't using the correct "generics"
syntax (i.e. the use of (str, int) instead of typing.Tuple[str, int]))
in a bunch of places. We're also not using a type checker yet and besides,
trio doesn't really expose a lot of its internal types very well.

cc @vodik

Tyler Goodlet added 2 commits August 22, 2018 11:50
This is purely for documentation purposes for now as it should be
obvious a bunch of the signatures aren't using the correct "generics"
syntax (i.e. the use of `(str, int)` instead of `typing.Tuple[str, int])`)
in a bunch of places. We're also not using a type checker yet and besides,
`trio` doesn't really expose a lot of its internal types very well.

2SQASH
@vodik
Copy link
Copy Markdown

vodik commented Aug 26, 2018

I wouldn't do it like this, imho. Stick to what mypy/typing module wants, even if its more verbose.

Using dict and tuple directly as annotations throws away useful information you could be encoding about the structure of said types: (e.g. Tuple[int], Tuple[int, str], Tuple[int, ...] and Tuple[...] all mean different things: tuple of a single int, tuple of a integer and string, tuple that has at least one element of type int, and tuple of any length)

But the fact that this design not only deviates away from community's approach to this problem, but is actively hostile to using mypy as a development aid (I run mypy as a background checker in my editor), I have zero interest in supporting it.

I'd personally reject this in any project under my control, so not interested in reviewing it.

@goodboy
Copy link
Copy Markdown
Owner Author

goodboy commented Aug 26, 2018

@vodik changed it all.

I'm just gonna go ahead and try to get mypy going in CI in this PR. Not quite there yet but maybe I'll get it running so we can look through the problems together.

@goodboy
Copy link
Copy Markdown
Owner Author

goodboy commented Aug 31, 2018

ping @vodik. Should be running mypy clean in CI!

Copy link
Copy Markdown
Owner Author

@goodboy goodboy left a comment

Choose a reason for hiding this comment

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

mpy for the winnnn!

@goodboy goodboy merged commit 22ac567 into master Sep 1, 2018
msgpack.dumps(data, use_bin_type=True))

async def get(self):
async def get(self) -> Any:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to look and see if you can define a TypeVar for this instead: https://mypy.readthedocs.io/en/latest/generics.html

T = TypeVar('T')

class StreamQueue(Generic[T]):
    async def get(self) -> Any:
        pass

You can get a little bit more type safety this way if you know what the return type will be situationaly:

queue = StreamQueue[str](steam)

For a StreamQueue that operates over strings.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Hmm yeah I was thinking I might want to more strictly define the underlying IPC protocol being used. So maybe this can tie in with that. Do you think it's necessary to type the msgpack blobs as well?

self._expect_result = None
self._exc: Optional[RemoteActorError] = None
self._expect_result: Optional[
Tuple[str, Any, str, Dict[str, Any]]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can do something like this:

Result = Tuple[str, Any, str, Dict[str, Any]

self._expect_result: Optional[Result]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

You think this is necessary though?

fn: typing.Callable,
bind_addr: Tuple[str, int] = ('127.0.0.1', 0),
rpc_module_paths: List[str] = None,
statespace: dict = None,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Probably meant to fix this one?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yup.


@asynccontextmanager
async def open_nursery(supervisor=None):
async def open_nursery() -> typing.AsyncGenerator[None, ActorNursery]:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Got the arguments backwards? Isn't the second argument the send type? I see you yielding a nursery.

Copy link
Copy Markdown
Owner Author

@goodboy goodboy Sep 1, 2018

Choose a reason for hiding this comment

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

yupp I remember forgetting about this now..
weird that mpy didn't catch it? I guess asynccontextmanager isn't exactly compatible yet.

This was referenced Sep 1, 2018
@goodboy goodboy deleted the type_annotations branch August 19, 2021 22:14
goodboy added a commit that referenced this pull request Mar 27, 2025
Fitting in line with the issues outstanding:
- #36: (msg)spec-ing out our SCIPP (structured-con-inter-proc-prot).
  (#36)

- #196: adding strictly typed IPC msg dialog schemas, more or less
  better described as "dialog/transaction scoped message specs"
  using `msgspec`'s tagged unions and custom codecs.
  (#196)

- #365: using modern static type-annots to drive capability based
  messaging and RPC.
  (statically #365)

This is a first draft of a new API for dynamically overriding IPC msg
codecs for a given interchange lib from any task in the runtime. Right
now we obviously only support `msgspec` but ideally this API holds
general enough to be used for other backends eventually (like
`capnproto`, and apache arrow).

Impl is in a new `tractor.msg._codec` with:
- a new `MsgCodec` type for encapsing `msgspec.msgpack.Encoder/Decoder`
  pairs and configuring any custom enc/dec_hooks or typed decoding.
- factory `mk_codec()` for creating new codecs ad-hoc from a task.
- `contextvars` support for a new `trio.Task` scoped
  `_ctxvar_MsgCodec: ContextVar[MsgCodec]` named 'msgspec_codec'.
- `apply_codec()` for temporarily modifying the above per task
  as needed around `.open_context()` / `.open_stream()` operation.

A new test (suite) in `test_caps_msging.py`:
- verify a parent and its child can enable the same custom codec (in
  this case to transmit `NamespacePath`s) with tons of pedantic ctx-vars
  checks.
- ToDo: still need to implement #36 msg types in order to be able to get
  decodes working (as in `MsgStream.receive()` will deliver an already
  created `NamespacePath` obj) since currently all msgs come packed in `dict`-msg
  wrapper packets..
  -> use the proto from PR #35 to get nested `msgspec.Raw` processing up
  and running Bo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants