Patch async enter all (addresses #242) #246

overclockworked64 · 2021-10-17T05:41:40Z

Ready for review.

There was an issue with getting current actor's UID because at the time we try to get it, no actor is currently spawned (at least that's what the error said), so I left a random UID as a temporary workaround - this should probably be addressed. Another thing is that I thought that it's more appropriate for async_enter_all to take a sequence of managers. A couple of mypy errors were fixed as well. Finally, a test that demonstrates new capabilities was added.

Minor nitpick: we should enforce some kind of consistent neutral code formatting style across the entire codebase.

goodboy · 2021-10-17T12:23:42Z

@overclockworked64 thanks for these fixes!

Glad to see you got something working for wrath 😎

I left a random UID as a temporary workaround - this should probably be addressed.

Where does this issue stem from? Are we trying to do something before the runtime comes up a la open_root_actor()?

more appropriate for async_enter_all to take a sequence of managers

Yeah, fine by me. Sequence in, sequence out would be more ergonomic I'd think.

Finally, a test that demonstrates new capabilities was added.

🏄🏼

Minor nitpick: we should enforce some kind of consistent neutral code formatting style across the entire codebase.

Yeah, I've changed my position on style a few times since this code base was started - which is probably what you're seeing 😂.

Thoughts from you and lurkerz are (mostly) welcome.

goodboy · 2021-10-17T12:24:50Z

tractor/_clustering.py

@@ -2,6 +2,8 @@
 Actor cluster helpers.


I've been wondering how to start exposing "high level" helpers like this. @guilledk and I had though about the names:

tractor.extras

tractor.builtin

overclockworked64 · 2021-10-18T11:47:08Z

I left a random UID as a temporary workaround - this should probably be addressed.

Where does this issue stem from? Are we trying to do something before the runtime comes up a la open_root_actor()?

As is, this code gives a RuntimeError, I'm assuming probably because we're trying to do something with the current actor before actually opening a nursery and spawning some actors.

…' will wait on

goodboy · 2021-10-21T22:39:03Z

tests/test_clustering.py

+            mngrs=[p.open_context(worker) for p in portals.values()],
+            teardown_trigger=teardown_trigger,
+        ) as contexts,
+        async_enter_all(


hehe i love this, but we still have 3.8 support atm.

I wonder though maybe we'll just drop since 3.10 is out?
I kinda like the idea of just rolling with latest 2 majors.

goodboy · 2021-10-21T22:40:45Z

tests/test_clustering.py

+        with trio.move_on_after(1):
+            for stream in itertools.cycle(streams):
+                await stream.send(MESSAGE)
+        teardown_trigger.set()


@overclockworked64 so this teardown_trigger we need because of teardown errors yah?

I wonder do you see the same issues that originally justified this now that #245 has landed?

I've tested on my use case and it seems that #245 resolves the problem.

goodboy · 2021-10-21T22:52:22Z

tractor/_clustering.py


 ) -> AsyncGenerator[
    list[str],
    dict[str, tractor.Portal]
 ]:

    portals: dict[str, tractor.Portal] = {}
-    uid = tractor.current_actor().uid
+    uid = str(__import__('random').randint(0, 2 ** 16))
+    # uid = tractor.current_actor().uid


Ok right, so you can put this back as it was as long as you move this line under the ractor.open_nursery(start_method=start_method) as an: line.

The runtime has to be up (which is done implicitly inside the first opened nursery in the root actor) to avoid the error you were probably getting.

I had to refactor this a bit, let me know what you think.

goodboy · 2021-10-21T22:53:17Z

tractor/_clustering.py

@@ -33,7 +38,7 @@ async def open_actor_cluster(
        raise ValueError(
            'Number of names is {len(names)} but count it {count}')

-    async with tractor.open_nursery() as an:
+    async with tractor.open_nursery(start_method=start_method) as an:
        async with trio.open_nursery() as n:


So just stick the uid = line underneath here but before you do the zip loop and it should all work nicely.

goodboy · 2021-10-21T22:54:06Z

tractor/trionics/_mngrs.py

    mngr: AsyncContextManager[T],
-    to_yield: dict[int, T],
+    unwrapped: dict[int, T],


woa someone has the functional parlance going on 😎

IMO that's what this does, it unwraps the thing so you can consume what's inside, so I thought the name was appropriate.

name is great; trio uses the same one internally for wrapping coroutines.

goodboy · 2021-10-21T23:07:11Z

tractor/trionics/_mngrs.py

-
-    to_yield = {}.fromkeys(id(mngr) for mngr in mngrs)
+    mngrs: Sequence[AsyncContextManager[T]],
+    teardown_trigger: trio.Event,


Ahh interesting. I wonder if this answers my question from above?

If we don't use a trigger passed in by the user then we rely on the user code eventually relaying in a trio.Cancelled (or other error) from the wither. So is there a difference here between putting a teardown_trigger: trio.Event in this scope (and hiding it from the user) vs. letting them pass it in? For eg. if we do a try: / finally: around the yield and always do the teardown_trigger.set() in the finally do we need to let the user pass it in?

It seems like #245 completely resolves the problem I encountered.

goodboy

@overclockworked64 super great work on this!

I have a couple questions mostly to do with whether we can make the teardown_trigger an implementation detail of async_enter_all(). Pretty sure it can be wrapped in a try:, finally block and we'll get the same result without the user needing to pass an event in.

The only other thing is that you can still use the uid = tractor.current_actor().uid stuff if you move the line below the nursery open.

Lmk what you think 🏄🏼

… problem

overclockworked64 · 2021-10-22T11:40:47Z

One thought I had is that maybe we should think of a better name for async_enter_all. FWIW, it could be shortened to aenter_all, but we could also think of something to replace all with.

goodboy · 2021-10-22T14:21:11Z

FWIW, it could be shortened to aenter_all, but we could also think of something to replace all with.

I'm totally down for a better name. Maybe one that emphasizes that the managers are being concurrently entered via tasks?

Lurkerz unite!

goodboy

Yeah this looks great to me.

@guilledk you mind giving this a peek?

I'll probably merge into the trionics dev branch before mid day.

goodboy · 2021-10-22T14:27:20Z

@overclockworked64 i think we can probably discuss re-naming in #241 since likely that branch is going to get a few more helpers stuck in it.

I think this is more then good enough to land on the dev branch.

goodboy reviewed Oct 17, 2021

View reviewed changes

goodboy force-pushed the trionics branch from 9a69756 to e5b3eb1 Compare October 17, 2021 12:56

overclockworked64 added 10 commits October 21, 2021 22:14

Add 'trio.trionics' to setup.py

f1c24c7

Add 'open_actor_cluster' to __all__

4974e3e

Postpone evaluation of annotations

21042ef

Make 'async_enter_all' take a teardown trigger which '_enter_and_wait…

dc8ccca

…' will wait on

Avoid RuntimeError by not using current_actor's uid

0f61305

Make sure the ID is a str

010a994

Cancel nursery

c8e7eb8

Rename a variable and fix type annotations

5f7802d

Add a clustering test

b17bdbf

Allow specifying start_method and hard_kill

f4af279

overclockworked64 force-pushed the patch-async-enter-all branch from 715970a to f4af279 Compare October 21, 2021 20:15

goodboy reviewed Oct 21, 2021

View reviewed changes

overclockworked64 added 3 commits October 22, 2021 02:59

Get rid of dumb random uid and use current actor's uid

14a7a12

Get rid of external teardown trigger because goodboy#245 resolves the…

3020793

… problem

Get rid of external teardown trigger

8d9ad6b

goodboy approved these changes Oct 22, 2021

View reviewed changes

goodboy merged commit f4ba805 into goodboy:trionics Oct 22, 2021

overclockworked64 deleted the patch-async-enter-all branch October 22, 2021 20:46

goodboy mentioned this pull request Oct 23, 2021

246 facepalm backup #252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patch async enter all (addresses #242) #246

Patch async enter all (addresses #242) #246

overclockworked64 commented Oct 17, 2021

goodboy commented Oct 17, 2021 •

edited

Loading

goodboy Oct 17, 2021

overclockworked64 commented Oct 18, 2021

goodboy Oct 21, 2021

goodboy Oct 21, 2021

overclockworked64 Oct 22, 2021

goodboy Oct 22, 2021

goodboy Oct 21, 2021

overclockworked64 Oct 22, 2021

goodboy Oct 21, 2021

goodboy Oct 21, 2021

overclockworked64 Oct 22, 2021

goodboy Oct 22, 2021

goodboy Oct 21, 2021

overclockworked64 Oct 22, 2021

goodboy left a comment

overclockworked64 commented Oct 22, 2021

goodboy commented Oct 22, 2021

goodboy left a comment

goodboy commented Oct 22, 2021

Patch async enter all (addresses #242) #246

Patch async enter all (addresses #242) #246

Conversation

overclockworked64 commented Oct 17, 2021

goodboy commented Oct 17, 2021 • edited Loading

Choose a reason for hiding this comment

overclockworked64 commented Oct 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodboy left a comment

Choose a reason for hiding this comment

overclockworked64 commented Oct 22, 2021

goodboy commented Oct 22, 2021

goodboy left a comment

Choose a reason for hiding this comment

goodboy commented Oct 22, 2021

goodboy commented Oct 17, 2021 •

edited

Loading