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

First attempt at removing trip & updating hazmat -> lowlevel #127

Closed
wants to merge 7 commits into from
Closed

First attempt at removing trip & updating hazmat -> lowlevel #127

wants to merge 7 commits into from

Conversation

guilledk
Copy link
Collaborator

This branch started as an attempt to remove all RuntimeWarnings about trio.hazmat deprecation but also turned into dropping ethereum/trio-run-in-process.

I'm getting a bunch of fails on tests and hang, I'm pretty sure it has to do with the tractor machinery not being setup right in the newly spawned processes.

@goodboy
Copy link
Owner

goodboy commented Jul 20, 2020

@guilledk amazing 🥳, thanks for making an attempt at this.

I cloned your branch and tried to run the suite with pytest -svvv tests --pdb and see a bunch of interesting errors:

Task <bound method Actor.cancel of <tractor._actor.Actor object at 0x7fc8307a6880>> was likely cancelled before it was started
Traceback (most recent call last):
  File "/home/goodboy/repos/tractor/tractor/_child.py", line 10, in <module>
    cloudpickle.dump(sys.stdout.detach(), result)
  File "/home/goodboy/repos/tractor/env/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py", line 47, in dump
    CloudPickler(file, protocol=protocol, buffer_callback=buffer_callback).dump(obj)
  File "/home/goodboy/repos/tractor/env/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py", line 429, in __init__
    Pickler.__init__(self, file, protocol=protocol, buffer_callback=buffer_callback)
TypeError: file must have a 'write' attribute
Traceback (most recent call last):
  File "/usr/lib64/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib64/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/home/goodboy/repos/tractor/tractor/_child.py", line 13, in <module>
    cloudpickle.dump(sys.stdout.detach(), err)
ValueError: underlying buffer has been detached
/usr/lib64/python3.8/runpy.py:127: RuntimeWarning: 'tractor._child' found in sys.modules after import of package 'tractor', but prior to execution of 'tractor._child'; this may result in unpredictable behaviour
  warn(RuntimeWarning(msg))

I think the last one might be fixed by changing the check for the spawn method in Actor.load_modules().

The other ones I'm note entirely sure what's going on yet; looks like stdio getting torn down too early?

@goodboy
Copy link
Owner

goodboy commented Jul 20, 2020

@guilledk the other thing is you dropped all the logging setup code in the child entrypoint which makes it so you can't see child logging which is really useful for debugging.

@goodboy
Copy link
Owner

goodboy commented Jul 20, 2020

@guilledk see my change set appended to yours:
guilledk/tractor@drop-trip-update-trio...goodboy:drop-trip-update-trio

I think this should make the tests run clean 🏄‍♀️

Just a quick summary of what I changed:

  • added back logging to subactors (config for which is passed down from the parent)
  • joined trio.Process instances on teardown (I think this fixed most of the tests)

@goodboy
Copy link
Owner

goodboy commented Jul 21, 2020

@guilledk i made a PR to your branch so I think if you merge that and update then it should run the CI here?

Also as discussed some cool things that we could add:

  • pass the actor name and uuid to _child.py so that you can see it from pstree
  • we need to get the test I have in 28799a3 (git cherry-pick 28799a3 I think should work?) running against this branch to make sure we're solving the ctl-c bug from trip doesn't do control-C right... #115
  • figure out if we want to merge the asyncio compat stuff with this branch or pull it out for later? 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

@goodboy
Copy link
Owner

goodboy commented Jul 21, 2020

@guilledk woot!

Looks like tests passed but mypy run is dying; check the .travis.yml to see how to run the mypy checks.

Also we're still seeing the errors from my earlier comment 😿
I think to get rid of 1. & 2. maybe the .detach() calls have to be somewhere else?

@goodboy goodboy self-requested a review July 21, 2020 01:14
@guilledk
Copy link
Collaborator Author

Hey so this commit is to check if the way I'm getting the actor from the passed arguments in run_in_process is correct.

Right now I just search the passed *args for the first instance of Actor and assume that's the right one, so I might as well use a named argument for that, but I don't know if you need this function to be generic or not.

Next commit I'll also check the passed arguments in the _async_main

@@ -153,6 +156,38 @@ async def cancel_on_completion(
await portal.cancel_actor()


@asynccontextmanager
async def run_in_process(async_fn, *args, **kwargs):
Copy link
Owner

@goodboy goodboy Jul 21, 2020

Choose a reason for hiding this comment

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

To make this simpler why don't we just make the signature of run_in_proc() something like:

async def run_in_process(subactor, async_fn, *args, **kwargs):
    debug_args = list(subactor.uid)
    ...

I do think it would be neat to accept these over sys.argv in _child.py and verify just as a sanity check.
It will for sure help with debugging using pstree (or whatever).

Heck, maybe it actually makes more sense to do this without passing the Actor type at all?
In theory we can avoid passing any complex types whatsoever as long as Actor._async_main() receives the appropriate data that Actor.__init__() normally does, 🤔.

Not sure if that's overthinking it.
I wonder if that means we could avoid cloudpickle entirely?

We can maybe just use msgpack in that case and load the module / func just like Actor._get_rpc_fun() does (note it requires .load_modules() to have been run first).

Copy link
Owner

@goodboy goodboy Jul 21, 2020

Choose a reason for hiding this comment

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

I think at the least in terms of dropping cloudpickle we should make an issue and tackle it later.
The multiprocessing.Process call will need to be re-configured for this as well.

@goodboy goodboy mentioned this pull request Jul 21, 2020
16 tasks
@goodboy
Copy link
Owner

goodboy commented Jul 21, 2020

@guilledk try rebasing onto the latest infect_asyncio head (git rebase upstream infect_asyncio, or whatever you've got this repo reffed as instead of upstream) to get some fixes I put up for mypy.

Ideally we can factor out the commonalities into your branch but I've got to look through it.

@goodboy
Copy link
Owner

goodboy commented Jul 22, 2020

Closing in favor of the new #128.

@goodboy goodboy closed this Jul 22, 2020
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