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

Wen? Multiprocessing-native debugger now! #129

Merged
merged 61 commits into from Oct 14, 2020
Merged

Wen? Multiprocessing-native debugger now! #129

merged 61 commits into from Oct 14, 2020

Conversation

goodboy
Copy link
Owner

@goodboy goodboy commented Jul 23, 2020

This is a first attempt at accomplishing #113 ๐Ÿ˜ฎ

Surprisingly it all works without using any threads, greenback, or other dark magic; it's just tractor bby ๐Ÿ’‹.

Hopefully we can get some feedback from lurkers ๐Ÿ•ต๏ธ

This first edition definitely works as well as other alternatives but there's a bunch of stuff we can do to make it better.

Caveats:

Immediate TODOs:

  • (we'll need to defer this until Fancy remote debugging (aka tab completion and editor controls)ย #130 is adressed) 1. disable the "cooked" mode when processing stdin (you'll see some commented code where I already started on this) such that each character is relayed immediately to the child stdin pipe.
    • This should get us out of the box completions and line editor control from the remote pdbpp ๐ŸŽ‰
    • I'm working off this example code; if there are terminal nerds about please feel free to yell about anything wrong with this approach :bowing_man
  • 2. lock access to the parent's stdin such that no more then one subactor can hijack it at a time; this of course is required in order to avoid all sorts of console mayhem
    • I've got locking working but not switching between two subactors requesting the parents stdio..
  • 3. somehow test all this? I think taking at look at how pytest does it is probably a good start.
  • 4. get feedback from some real users and figure out if debug_mode is cool or not.
    • thinking this will come with .dev0 usage so marking complete for now.

For the future:

@goodboy goodboy added enhancement New feature or request help wanted Extra attention is needed discussion experiment Exploratory design and testing labels Jul 23, 2020
@goodboy
Copy link
Owner Author

goodboy commented Jul 23, 2020

@guilledk If you get a chance try to play with this as well :)

@goodboy
Copy link
Owner Author

goodboy commented Jul 24, 2020

Alright, so after much frustration trying to get tab completion to just work I went back to trip and found that it did just work and I finally got the ๐Ÿคฆ that of course it's not going to work with our code since the child process isn't attached the parent's (nor any) tty ๐Ÿ˜ฟ

Of course rlcompleter (and thus pdbp and pdbp++) rely on a tty at startup for any readline support which is what gives all the fancy tab completion and control character functionality. I'm pretty sure even somehow hacking a pty around breakpoint() calls isn't going to work either especially since there's a whole package (ptyprocess) built to solve the problem of launching subprocs under ptys, which, btw does work with pdb when you enter from the subproc.

I think instead of doing a big writeup here I'm going to make a new issue and discuss my findings.
2. - 4. should still get complete before this lands.

Oh and one further approach we may want to take to get the fancy stuff for now is just don't hook up subproc stdin to a pipe and instead let it have the parent's tty and then just use a trio.FIFOLock around access (including overriding breakpoint() access by the root process). This might get us 90% of the way I'd like things to work without being super correct and it should work for nested subprocs I think.

@guilledk

This comment has been minimized.

@goodboy
Copy link
Owner Author

goodboy commented Jul 26, 2020

@guilledk yeah, I'm not following that change?

The debugging stuff I don't think should have an effect here?
Maybe i'm missing something.

@goodboy goodboy force-pushed the flaky_tests branch 9 times, most recently from df5e702 to 5a27065 Compare July 27, 2020 02:53
@goodboy goodboy marked this pull request as ready for review October 13, 2020 18:21
@goodboy
Copy link
Owner Author

goodboy commented Oct 13, 2020

Worth mentioning is that I have not idea if this stuff works no windows since there doesn't seem to be any pexpect support?

Might have to dig into this so that likely means a deferral issue is coming.

@goodboy goodboy force-pushed the multiproc_debug branch 3 times, most recently from bf90705 to 0510ab2 Compare October 14, 2020 01:43
portal = await tn.run_in_actor('sleeper', spin_for)


@no_windows
Copy link
Owner Author

Choose a reason for hiding this comment

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

Probably need an issue for this as well.

@@ -171,13 +176,16 @@ class Actor:
_root_n: Optional[trio.Nursery] = None
_service_n: Optional[trio.Nursery] = None
_server_n: Optional[trio.Nursery] = None
_lifetime_stack: ExitStack = ExitStack()
Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh right, we might want tests for this ๐Ÿคฃ..

Maybe it's ok to just slip it in for now and worry later?

@goodboy goodboy mentioned this pull request Oct 14, 2020


# TODO:
# - recurrent entry to breakpoint() from single actor *after* and an
Copy link
Owner Author

Choose a reason for hiding this comment

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

Actually, I think most of these are covered now but wouldn't hurt to do an audit in the future.

I don't have much juice left to keep pushing here ๐Ÿ˜ฟ

at each level up the tree.
"""

# TODO: inside this script there's still a bug where if the parent
Copy link
Owner Author

@goodboy goodboy Oct 14, 2020

Choose a reason for hiding this comment

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

This is actually fixed now and is addressed by solving #156.

The test below actually demonstrates this is now solved.

@goodboy goodboy merged commit 7115d6c into master Oct 14, 2020
@goodboy goodboy deleted the multiproc_debug branch October 14, 2020 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request experiment Exploratory design and testing help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants