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

Trionics #241

Merged
merged 32 commits into from
Oct 27, 2021
Merged

Trionics #241

merged 32 commits into from
Oct 27, 2021

Conversation

goodboy
Copy link
Owner

@goodboy goodboy commented Oct 4, 2021

Introduces a new sub-package to start exposing all our high(er) level trio primitives and goodies.

@goodboy goodboy changed the base branch from fix_kbi_in_ctx_block to optional_msgspec_support October 4, 2021 19:06
@goodboy goodboy force-pushed the trionics branch 2 times, most recently from 4ef9ef6 to ae13a5e Compare October 6, 2021 19:56
Base automatically changed from optional_msgspec_support to master October 6, 2021 21:01
@goodboy goodboy force-pushed the trionics branch 2 times, most recently from c7e03ae to c064d4d Compare October 13, 2021 13:37
@goodboy goodboy force-pushed the trionics branch 3 times, most recently from 9a69756 to e5b3eb1 Compare October 17, 2021 12:56
@goodboy
Copy link
Owner Author

goodboy commented Oct 22, 2021

Oh, right so we'll need to drop 3.8 as well.
I'll put up a PR todo that shortly if noone else wants to.

Since it seems we're building out more and more higher level primitives
in order to support certain parallel style actor trees and messaging
patterns (eg. task broadcast channels), we might as well start a new
sub-package for purely `trio` constructions. We hereby dub this
the realm of `trionics` (like electronics but for trios instead of
electrons).

To kick things off, add an `async_enter_all()` concurrent
exit-stack-like context manager API which will concurrently spawn
a sequence of provided async context managers and deliver their ordered
results but with proper support for `trio` cancellation semantics.
The stdlib's `AsyncExitStack` is not compatible with nurseries not
`trio` tasks (which are cancelled) since as task will be suspended on
the stack after push and does not ever hit a checkpoint until the stack
is closed.
@goodboy goodboy marked this pull request as ready for review October 23, 2021 22:10


@acm
async def open_actor_cluster(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be the answer to the process pool question right? looks great!

Copy link
Owner Author

Choose a reason for hiding this comment

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

More or less yah.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I guess the last question here is how do we export this?

Should it be top level from tractor import open_actor_cluster or maybe should be export via something like tractor.builtin, tractor.extras?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also change async_enter_all to mass_aenter, for example.

Copy link
Owner Author

@goodboy goodboy Oct 24, 2021

Choose a reason for hiding this comment

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

Hmm, <something_aenter() is interesting, though i wonder if we should try to mimic what contextlib stack apis offer? Not that any of them have this kind of interface per say. mass_ seems a bit foreign to me.

I think we need to emphasize that the entering is done concurrently, not just that __aenter__() is being called (which is obviously async). I think in worker pool parlance this is done with something like pool.map() but the difference here is that we're not running functions and collecting output - it's more of a setup/teardown type thing..

Copy link
Owner Author

Choose a reason for hiding this comment

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

what about enter_all_soon()?

Copy link
Owner Author

@goodboy goodboy Oct 24, 2021

Choose a reason for hiding this comment

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

AsyncExitStack api for reference.

if all(unwrapped.values()):
all_entered.set()

await trio.sleep_forever()
Copy link

@richardsheridan richardsheridan Oct 24, 2021

Choose a reason for hiding this comment

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

My one nitpick as a lurker is that with this cancel-to-exit strategy, all managers will exit via an "unclean" path, or in other words, their __aexit__ methods will never be called with (None, None, None) as arguments.

I'm not sure if that is actually important? It just gives me an itch. Could replace it with an event pretty easily.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ahh good point. I don't see any reason not to use an event and you're right about the mngrs themselves then getting expected inputs on different exit conditions.

The api we've made here is actually closer to `asyncio.gather()` but
with opening async context managers instead of funcs. Use another event
to allow for graceful teardown of children on non-cancellation exits
and add a doc string.
Change to `gather_contexts()`, use event for graceful exit
@goodboy
Copy link
Owner Author

goodboy commented Oct 25, 2021

I think we just need a nooz file and this is good to land?

@goodboy
Copy link
Owner Author

goodboy commented Oct 27, 2021

Let this CI run through and we'll land it.

@goodboy goodboy merged commit 5dbe8e4 into master Oct 27, 2021
@goodboy goodboy deleted the trionics branch October 27, 2021 17:09
@goodboy goodboy mentioned this pull request Nov 2, 2021
@goodboy goodboy mentioned this pull request Jan 24, 2022
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.

4 participants