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

Run control channel in separate thread #585

Merged
merged 12 commits into from
Feb 24, 2021

Conversation

SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Jan 27, 2021

This makes the control channel run in a separate thread from the shell channel, so that control messages can be processed concurrently to shell requests.

  • this is now the recomended approach in the Jupyter protocol.

    For a smoother user experience, we recommend running the control channel in a separate thread from the shell channel, so that e.g. shutdown or debug messages can be processed immediately without waiting for a long-running shell message to be finished processing (such
    as an expensive execute request).

  • this is required for ipykernel to support the Jupyter Debug Protocol in ipykernel: we need to be able to remove and add breakpoints while code is running (or even more importantly, process messages such as "continue", "step into", which are by nature to be called during execution.

Fixes #447

@SylvainCorlay SylvainCorlay changed the title WIP - Run control channel in separate thread Run control channel in separate thread Jan 29, 2021
@SylvainCorlay SylvainCorlay force-pushed the control-thread branch 2 times, most recently from 4bcbe02 to bf48c69 Compare January 29, 2021 11:55
@minrk
Copy link
Member

minrk commented Jan 29, 2021

Nice! How do you handle maintaining the parent_header state? Processing a control request needs to set parent header for its own side effects (only status?), but must not conflict with the parent header set by the now concurrently outstanding shell request.

Make sure to add a test for this, like:

  1. shell request produces multiple outputs (e.g. for i in range(5): print(i); sys.stdout.flush(); time.sleep(1))
  2. control request begins and ends in the middle of shell request, ideally ensuring it overlaps at least one or two shell output messages
  3. verify that all shell output is properly routed and not captured by control, status messages are handled correctly, etc.

@SylvainCorlay
Copy link
Member Author

Nice! How do you handle maintaining the parent_header state?

Yes, I think we need to have two parent_header attributes depending on the channel like in xeus-python.

now concurrently outstanding shell request.

The debugger tests will essentially be of that nature. Since we are working on plugging the debugger (porting code from xeus-python) on top of this PR, we should probably keep this open until the debugger works in practice.

@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Jan 29, 2021

Updates:

  • I split the control and shell parent headers
  • We now expose a shell_streams property with a deprecation warning
  • All tests are passing but somehow, the CI hangs forever in the end. Something must not be shutting down properly.

@SylvainCorlay SylvainCorlay force-pushed the control-thread branch 14 times, most recently from 285a939 to ff50c68 Compare February 6, 2021 16:45
@blink1073 blink1073 added this to the 5.6 milestone Feb 23, 2021
@blink1073 blink1073 modified the milestones: 5.6, 6.0 Feb 23, 2021
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Nice work, thank you!

@blink1073 blink1073 merged commit b4b295f into ipython:master Feb 24, 2021
@SylvainCorlay SylvainCorlay deleted the control-thread branch February 24, 2021 10:35
@Carreau
Copy link
Member

Carreau commented Mar 9, 2021

I think this has broken Jupyter_client ci; there are a bunch of errors wrt control channel.

'Property shell_streams is deprecated in favor of shell_stream',
DeprecationWarning
)
return [shell_stream]
Copy link
Member

Choose a reason for hiding this comment

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

missing self.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle the Control channel in a separate thread
5 participants