Support trio-run-in-process as process spawning backend #95

goodboy · 2020-01-20T18:51:50Z

Resolves #93.

For those interested in support for alternatives to multiprocessing this PR bring in the first new choice: trio-run-in-process. I had to rejig the actor nursery machinery but I think it's overall resulted in more trionic, easier to understand code.

trip (trio-run-in-process) seems to not suffer the ailments of the spawning in mp namely infinitely recursive, structured concurrent, actor pools seem to be quite reliable. The spawning is definitely slower then the forkserver method still but I think this is a decent first step toward trio-izing process launching entirely. I'm interested now to see how hard it would be to roll our own forkserver method using just trio APIs.

The test suite is likely to fail at first since I haven't integrated trip into the test generation system quite yet. Failure on Windows is expected here out of the box as well.

I would appreciate any code reviews or criticisms from lurkers ;)
ping @ryanhiebert

Get a few more things working: - fail reliably when remote module loading goes awry - do a real hacky job of module loading using `sys.path` stuffsies - we're still totally borked when trying to spin up and quickly cancel a bunch of subactors... It's a small move forward I guess.

This took a ton of tinkering and a rework of the actor nursery tear down logic. The main changes include: - each subprocess is now spawned from inside a trio task from one of two containing nurseries created in the body of `tractor.open_nursery()`: one for `run_in_actor()` processes and one for `start_actor()` "daemons". This is to address the need for `trio-run-in_process.open_in_process()` opening a nursery which must be closed from the same task that opened it. Using this same approach for `multiprocessing` seems to work well. The nurseries are waited in order (rip actors then daemon actors) during tear down which allows for avoiding the recursive re-entry of `ActorNursery.wait()` handled prior. - pull out all the nested functions / closures that were in `ActorNursery.wait()` and move into the `_spawn` module such that that process shutdown logic takes place in each containing task's code path. This allows for vastly simplifying `.wait()` to just contain an event trigger which initiates process waiting / result collection. Likely `.wait()` should just be removed since it can no longer be used to synchronously wait on the actor nursery. - drop `ActorNursery.__aenter__()` / `.__atexit__()` and move this "supervisor" tear down logic into the closing block of `open_nursery()`. This not only cleans makes the code more comprehensible it also makes our nursery implementation look more like the one in `trio`. Resolves #93

ryanhiebert

This is neat work! It looks like trip make things conceptually simpler.

ryanhiebert · 2020-01-23T14:14:13Z

tests/test_rpc.py

@@ -60,7 +60,7 @@ def test_rpc_errors(arb_addr, to_call, testdir):
    if exposed_mods == ['tmp_mod']:
        # create an importable module with a bad import
        testdir.syspathinsert()
-        # module should cause raise a ModuleNotFoundError at import
+        # module should cause a raise of a ModuleNotFoundError at import


Warning, I'm pedantic. I'd have worded this as module should raise a ModuleNotFoundError at import. Shorter and grammatically correct.

ryanhiebert · 2020-01-23T14:17:22Z

tractor/__init__.py

    # https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods
-    start_method: str = 'forkserver',
+    # OR `trio-run-in-process` (the new default).
+    start_method: str = 'trip',


This comment is confusing, since it seems like its saying that trio-run-in-process is the new default, but it's actually the shortened version trip that is the default. I'd pick one or the other as the "correct" spelling in the code.

Do you think the long or short name is better?

I like the short name better, but I think it would be best to be consistent with the package name. Maybe we'll eventually get a package with a shorter, more user friendly name. Then I'll be very happy to see us using that name instead.

@ryanhiebert went with the explicit name as asked.

It seems that mixing the two backends in the test suite results in hangs due to lingering forkservers and resource managers from `multiprocessing`? Likely we'll need either 2 separate CI runs to work or someway to be sure that these lingering servers are killed in between tests.

Set `trio-run-in-process` as the default on *nix systems and `multiprocessing`'s spawn method on Windows. Enable overriding the default choice using `tractor._spawn.try_set_start_method()`. Allows for easy runs of the test suite using a user chosen backend.

Add a `--spawn-backend` option which can be set to one of {'mp', 'trio_run_in_process'} which will either run the test suite using the `multiprocessing` or `trio-run-in-process` backend respectively. Currently trying to run both in the same session can result in hangs seemingly due to a lack of cleanup of forkservers / resource trackers from `multiprocessing` which cause broken pipe errors on occasion (no idea on the details). For `test_cancellation.py::test_nested_multierrors`, use less nesting when mp is used since it breaks if we push it too hard with the whole recursive subprocess spawning thing...

goodboy · 2020-01-28T04:45:41Z

If no one is going to review or complain about this I might just merge it and hope for the best.
There's only a few more things I want to get done before pushing an alpha version onto pypi so speak now or never 😉

ryanhiebert · 2020-01-28T13:43:08Z

Fine by me. As long as you're happy and it's still running, don't let your momentum be stalled... Sometimes you're the only one looking.

goodboy · 2020-01-28T19:43:29Z

tractor/_spawn.py

    # passed through to actor main
    bind_addr: Tuple[str, int],
    parent_addr: Tuple[str, int],
-) -> mp.Process:
+    begin_wait_phase: trio.Event,


Ahh dang do we need this?

Nope. Was left over from an old approach.

@salotz

Thanks to @salotz for pointing out that the first example in the docs was broken. Though it's somewhat embarrassing this might also explain the problem in #79 and certain issues in #59... The solution here is to import the target RPC module using the its unique basename and absolute filepath in the sub-actor that requires it. Special handling for `__main__` and `__mp_main__` is needed since the spawned subprocess will have no knowledge about these parent- -state-specific module variables. Solution: map the modules name to the respective module file basename in the child process since the module variables will of course have different values in children.

goodboy · 2020-01-29T17:23:20Z

Big props to @salotz for running the first example in the docs and finding it was totally borked...

2a43079 fixes this and should probably fix issues others have seen on Windows to do with __mp_main__ not found errors. For now that commit introduces more manual module loading in sub-actors, something multiprocessing was doing for us behind the scenes before.

Instead of hackery trying to map modules manually from the filesystem let Python do all the work by simply copying what ``multiprocessing`` does to "fixup the __main__ module" in spawned subprocesses. The new private module ``_mp_fixup_main.py`` is simply cherry picked code from ``multiprocessing.spawn`` which does just that. We only need these "fixups" when using a backend other then ``multiprocessing``; for now just when using ``trio_run_in_process``.

goodboy · 2020-01-30T02:40:45Z

Just so it's noted, I actually instead just copied the __main__ fixups stuff multiprocessing does into a new private module (see the commit message for deats). This technique seems to be the right way to make subprocesses behave in the expected way in terms of module loading. We only use it when not using the mp backend (i.e. for now when using trio_run_in_process).

salotz

Its going to take me some more time with the code base before I really understand the whole multiprocessing module load thing, so it was mostly pig latin for me. The one thing I did comment on is not really related to this PR, and is just a request to refactor some stuff which will have an impact on APIs.

salotz · 2020-01-30T03:38:37Z

tractor/__init__.py

    # https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods
-    start_method: str = 'forkserver',
+    # OR `trio_run_in_process` (the new default).
+    start_method: Optional[str] = None,


This should just default to the string for 'trip'. I think you set it to None here because that is what the try_set_start_method returns for trip. I don't think that's how that function should behave, so once that gets refactored this should just be a string type with whatever the string repr for that mode is.

understand the whole multiprocessing module load thing

If you're talking about the forkserver hack stuff, yeah don't worry about it. I'm starting to think we'll drop all that to write our own anyway.

@salotz so as you can see from the build triggered after fb93336 which failed, on windows we need to have a different default. If 'trio_run_in_process' is passed to try_set_start_method() we're going to get an error on windows (I think this is correct since we want that function to explicitly fail when an incorrect method is chosen on the platform).

This is the small dilemma, on different systems we require different default start methods. We can either leave it as I had it -> None is default and implies the correct actual default for the system is chosen internally or we move that same logic into tractor.run() so that's it's more clear that's what's happening?

Interested in your opinion here :)

Yeah and the other reason to leave that default as None and not have the default-for-the-system logic here would be that when we get alternatives for windows then we'll want to pass through the user chosen value and validate it.

salotz · 2020-01-30T03:39:42Z

tractor/__init__.py

    **kwargs,
 ) -> Any:
    """Run a trio-actor async function in process.

    This is tractor's main entry and the start point for any async actor.
    """
-    _spawn.try_set_start_method(start_method)
+    if start_method is not None:


relatedly wouldn't need this check here with above refactoring.

salotz · 2020-01-30T03:41:19Z

tractor/_actor.py

+    # marked by the process spawning backend at startup
+    # will be None for the parent most process started manually
+    # by the user (currently called the "arbiter")
+    _spawn_method: Optional[str] = None


ditto. Unless this has different semantics which is to just choose whichever was chosen in run, in which case the Optional would be correct.

@salotz for the parent-most (called arbiter) right now this will actually stay as None since there was no spawn method that created the top most process.

salotz · 2020-01-30T03:43:07Z

.travis.yml

+          python: 3.7  # this works for Linux but is ignored on macOS or Windows
+          env: SPAWN_BACKEND="trio_run_in_process"
+
+        - name: "Pytron 3.8: multiprocessing"


salotz · 2020-01-30T03:47:57Z

tractor/_spawn.py



-def try_set_start_method(name: str) -> mp.context.BaseContext:
+def try_set_start_method(name: str) -> Optional[mp.context.BaseContext]:


ditto with above. Optional and Union return values is a code smell anyways

The thing is for trip there is no context possible to return, nor is this function called in that case.
I'm not sure how else to specify the output if it's not this?

Either make a wrapper function that does produce a context or this function simply decides on the start method given the platform yadayada and then outputs a string/enum. I don't think it blocks this PR btw

KK we can discuss further in #101

goodboy · 2020-01-30T07:24:28Z

tractor/_actor.py

+            mod = importlib.import_module(name)
+            mods[name] = _get_mod_abspath(mod)
+
+        self.rpc_module_paths = mods
        self._mods: dict = {}


Should probably type annotate this.

goodboy · 2020-01-30T07:25:09Z

tractor/_actor.py

+                # XXX append the allowed module to the python path which
+                # should allow for relative (at least downward) imports.
+                sys.path.append(os.path.dirname(filepath))
+                # XXX leaving this in for now incase we decide to swap


Maybe this isn't worth keeping in for now.
I can always dig up how to do it again if someone complains about the path mutating approach.

goodboy · 2020-01-30T07:28:01Z

tractor/_spawn.py

@@ -3,7 +3,14 @@

 Mostly just wrapping around ``multiprocessing``.


comment is wrong now..

goodboy · 2020-01-30T07:29:25Z

tractor/_spawn.py



-def try_set_start_method(name: str) -> mp.context.BaseContext:
+def try_set_start_method(name: str) -> Optional[mp.context.BaseContext]:
    """Attempt to set the start method for ``multiprocess.Process`` spawning.


Doc is also out of date - doesn't include trip deats.

goodboy · 2020-01-30T07:33:56Z

tractor/_spawn.py

+            # unblock parent task
+            task_status.started(portal)
+
+            # wait for ActorNursery.wait() to be called


Hmm is this comment right still?

goodboy · 2020-01-30T07:35:57Z

tractor/_trionics.py

+    # an `@asynccontextmanager` which has an internal nursery *and* the
+    # task that opens a nursery **must also close it**.
+    errors: Dict[Tuple[str, str], Exception] = {}
+    async with trio.open_nursery() as da_nursery:


should probably comment on the difference between da_ (daemon actor) and ria_ (run in actor) nurseries..

goodboy · 2020-01-30T07:38:13Z

tractor/_trionics.py

+                # last bit before first nursery block ends
+                log.debug(f"Waiting on all subactors to complete")
+                anursery._join_procs.set()
+            # ria_nursery scope


should probably mark the same for the da_nursery scope

The logic in the `ActorNursery` block is critical to cancellation semantics and in particular, understanding how supervisor strategies are invoked. Stick in a bunch of explanatory comments to clear up these details and also prepare to introduce more supervisor strats besides the current one-cancels-all approach.

goodboy added 8 commits January 21, 2020 10:27

WIP trying out trio_run_in_process

1b7cdfe

Do module abspath loading in actor init

91c3716

Drop ActorNusery.wait(); it's no longer necessary really

6c45416

Add trio-run-in-process` as dep

3c86aa2

Importing happens once locally now so expect a local error

e1a55a6

Type checker fixes

4b0554b

goodboy force-pushed the try_trip branch from 4d4133c to 4b0554b Compare January 21, 2020 15:29

Add mypy.ini lel

f1a96c1

goodboy mentioned this pull request Jan 23, 2020

Windows support? ethereum/trio-run-in-process#3

Open

goodboy added 4 commits January 23, 2020 01:15

Try out trip as the default spawn_method on unix for now

ddbf557

Add trip to start_method parametrizations

44996fe

Don't import trip on Windows

4c5a60d

Fake out mypy again

4837595

ryanhiebert reviewed Jan 23, 2020

View reviewed changes

goodboy added 7 commits January 24, 2020 00:47

Be explicit with the real name for trip

d9803ca

Use trip as default in all tests for now

bc259b7

Add per backend test runs for each Python version

87948bd

Always force mp backend on Windows

e18fec9

goodboy force-pushed the try_trip branch from ce023ae to b92b1c3 Compare January 27, 2020 03:15

Make windows job names explicit

7c1bc1f

goodboy force-pushed the try_trip branch from b92b1c3 to 7c1bc1f Compare January 27, 2020 03:17

goodboy added 4 commits January 26, 2020 22:35

Fork isn't present on windows...

e57811a

Drop useless fork error branch

b4cb743

Force mp backend if option is blank?

5fd38d4

Forkserver just can't seem to cut it...

a6b249c

goodboy force-pushed the try_trip branch 2 times, most recently from aa26e9a to 1d9d15a Compare January 27, 2020 21:02

Document available process spawning backends

7feef44

goodboy force-pushed the try_trip branch from 1d9d15a to 7feef44 Compare January 27, 2020 21:04

goodboy commented Jan 28, 2020

View reviewed changes

goodboy force-pushed the try_trip branch from 2b021d7 to c479b86 Compare January 29, 2020 16:54

goodboy force-pushed the try_trip branch from c479b86 to 2a43079 Compare January 29, 2020 17:16

salotz reviewed Jan 30, 2020

View reviewed changes

goodboy mentioned this pull request Jan 30, 2020

doctests for examples #98

Closed

goodboy commented Jan 30, 2020

View reviewed changes

tractor/_spawn.py Outdated

@@ -3,7 +3,14 @@

Mostly just wrapping around ``multiprocessing``.

Copy link

Owner Author

goodboy Jan 30, 2020

Choose a reason for hiding this comment

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

comment is wrong now..

goodboy commented Jan 30, 2020

View reviewed changes

goodboy added 4 commits January 31, 2020 09:50

Fix typo

ee4b014

Drop old module loading from abspath cruft

8264b7d

Fixup _spawn.py comments to incorporate trip

e671cb4

goodboy force-pushed the try_trip branch from fb93336 to e671cb4 Compare January 31, 2020 18:24

goodboy mentioned this pull request Jan 31, 2020

_spawn.try_set_start_method() should have consistent return value? #101

Open

goodboy merged commit 5741bd5 into master Jan 31, 2020

goodboy mentioned this pull request May 27, 2020

Let's roll our own subproc spawner ("the way the experts would") #117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support trio-run-in-process as process spawning backend #95

Support trio-run-in-process as process spawning backend #95

goodboy commented Jan 20, 2020 •

edited

Loading

ryanhiebert left a comment

ryanhiebert Jan 23, 2020

ryanhiebert Jan 23, 2020

goodboy Jan 23, 2020

ryanhiebert Jan 23, 2020

goodboy Jan 27, 2020

goodboy commented Jan 28, 2020

ryanhiebert commented Jan 28, 2020

goodboy Jan 28, 2020

goodboy Jan 28, 2020

goodboy commented Jan 29, 2020

goodboy commented Jan 30, 2020

salotz left a comment

salotz Jan 30, 2020

goodboy Jan 30, 2020

goodboy Jan 31, 2020

goodboy Jan 31, 2020

salotz Jan 30, 2020

salotz Jan 30, 2020

goodboy Jan 30, 2020

salotz Jan 30, 2020

salotz Jan 30, 2020

goodboy Jan 30, 2020

salotz Jan 31, 2020

goodboy Jan 31, 2020

goodboy Jan 30, 2020

goodboy Jan 30, 2020

goodboy Jan 30, 2020

goodboy Jan 30, 2020

goodboy Jan 30, 2020

goodboy Jan 30, 2020 •

edited

Loading

goodboy Jan 30, 2020



		def try_set_start_method(name: str) -> mp.context.BaseContext:
		def try_set_start_method(name: str) -> Optional[mp.context.BaseContext]:

		@@ -3,7 +3,14 @@

		Mostly just wrapping around ``multiprocessing``.

Support trio-run-in-process as process spawning backend #95

Support trio-run-in-process as process spawning backend #95

Conversation

goodboy commented Jan 20, 2020 • edited Loading

ryanhiebert left a 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 commented Jan 28, 2020

ryanhiebert commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodboy commented Jan 29, 2020

goodboy commented Jan 30, 2020

salotz left a 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

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 Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodboy commented Jan 20, 2020 •

edited

Loading

goodboy Jan 30, 2020 •

edited

Loading