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

Drop trio-run-in-process, use pure trio process spawner, test out of channel ctrl-c subactor cancellation #128

Merged
merged 20 commits into from
Jul 27, 2020

Conversation

goodboy
Copy link
Owner

@goodboy goodboy commented Jul 22, 2020

This continues from #127 since some history was reworked to get some upstream changes in before this to do with #121.

Much thanks to @guilledk for starting this effort!

The main things we have accomplished thus far:

Things still on the todo:

  • (solved via hand-crafted rebasing) figure out if we can make this land before Infect asyncio #121 (which still has a bunch of outstandings)
    • Techincally, there's no documentation of it so it might not hurt to just merge?
      • note to self: the only commits we probably want to hold off on are found with git log --follow tractor/to_asyncio.py
  • (probably can defer for now, though we need a new issue) determine if we want to drop cloudpickle and instead pass all Actor.__init__() data to Actor._async_main() allowing us to just use msgpack and the Actor.load_modules()/Actor._get_rpc_func() way of invoking the target entrypoint.
  • (should have been resolved in Wen? Multiprocessing-native debugger now! #129) discover if there's anything else related to remote debugging (REPL-driven, multi-process-native debugger features #113) that can be addressed now as part of subproc spawning before we move on

Closes #117, #115, #112

@goodboy
Copy link
Owner Author

goodboy commented Jul 22, 2020

grrr,

tractor/to_asyncio.py:47: error: Missing return statement

This mypy stuff is killin me.

goodboy and others added 15 commits July 24, 2020 16:48
This is an initial solution for #120.

Allow spawning `asyncio` based actors which run `trio` in guest
mode. This enables spawning `tractor` actors on top of the `asyncio`
event loop whilst still leveraging the SC focused internal actor
supervision machinery. Add a `tractor.to_syncio.run()` api to allow
spawning tasks on the `asyncio` loop from an embedded (remote) `trio`
task and return or stream results all the way back through the `tractor`
IPC system using a very similar api to portals.

One outstanding problem is getting SC around calls to
`asyncio.create_task()`. Currently a task that crashes isn't able to
easily relay the error to the embedded `trio` task without us fully
enforcing the portals based message protocol (which seems superfluous
given the error ref is in process). Further experiments using `anyio`
task groups may alleviate this.
This is an edit to factor out changes needed for the `asyncio` in guest mode
integration (which currently isn't tested well) so that later more pertinent
changes (which are tested well) can be rebased off of this branch and
merged into mainline sooner. The *infect_asyncio* branch will need to be
rebased onto this branch as well before merge to mainline.
An initial attempt to discover an issue with trio-run-inprocess.
This is a good test to have regardless.
Verify ctrl-c, as a user would trigger it, properly cancels the actor
tree. This was an issue with `trio-run-in-process` that clearly wasn't
being handled correctly but for sure is now with the plain old
`trio` process spawner.

Resolves #115
Using the context manager interface does some extra teardown beyond simply
calling `.wait()`. Pass the subactor's "uid" on the exec line for
debugging purposes when monitoring the process tree from the OS.
Hard code the child script module path to avoid a double import warning.
We don't really need stdin for anything but passing the entry point and
detaching it seemed to just cause errors on cancellation teardown.
@goodboy goodboy changed the base branch from infect_asyncio to master July 24, 2020 21:09
@goodboy
Copy link
Owner Author

goodboy commented Jul 24, 2020

@guilledk @carlosplanchon this is now rebased onto the changes hand-pulled from the infect_asyncio branch and is ready for merge with master (I think).

Please take a look at this if you have the time 🥺

@goodboy
Copy link
Owner Author

goodboy commented Jul 25, 2020

Ok somewhow we've borked windows?
It looks like pytest is collecting but not running?
Gonna try pushing up a .travis.yml tweak.

The multiprocessing run on linux is shaky (as usual) so I wonder if we should go further with not pushing the nested subactors so much in those tests even if just for making the CI process a little smoother.

@goodboy goodboy force-pushed the flaky_tests branch 4 times, most recently from 7b38faa to cf8a980 Compare July 25, 2020 17:36
The new pure trio spawning backend uses `subprocess` internally which is
also supported on windows so let's run it in CI.
@goodboy goodboy mentioned this pull request Jul 25, 2020
@goodboy goodboy force-pushed the flaky_tests branch 10 times, most recently from 4c4a6d3 to df5e702 Compare July 27, 2020 02:36
- ease up on first stream test run deadline
- skip streaming tests in CI for mp backend, period
- give up on > 1 depth nested spawning with mp
- completely give up on slow spawning on windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let's roll our own subproc spawner ("the way the experts would")
1 participant