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

OWPythonScript: Replace console #5228

Draft
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

irgolic
Copy link
Member

@irgolic irgolic commented Jan 31, 2021

OWPythonScript PRs:

#5208 -> here

Issue

The console hangs on execution. Also it could be prettier.

Description of changes
Before

Screenshot 2021-02-01 at 23 46 50

After

Screenshot 2021-02-01 at 23 57 53

The console now launches a python kernel in a separate process.

Some new files:

  • python_serialize.py
    This is variable injection ZMQ stuff between console and kernel. It facilitates a pipe-based IPC (inter-process communication) solution. Makes a great prototype for client/server architecture in Orange. Apparently ipc:// isn't supported on Windows though TODO.
  • python_console.py
    Uses python_serialize.py's OrangeZMQMixin. Also does a lot of work maintaining current state (transferring variables, executing script), also triggering signals that indirectly change the widget's status message.
  • python_kernel.py
    Uses python_serialize.py's OrangeZMQMixin. Simplest of the bunch, just make sure not to import Qt or Orange here or it hangs the kernel on startup (making it timeout).

I tried making the kernel fast, but if the timeout is an issue, I can extend it. From what I've seen, it only causes a small UI glitch.

Includes
  • Code changes
  • Tests
  • Documentation

@irgolic irgolic changed the title Pythonscript console OWPythonScript: Replace console Jan 31, 2021
@irgolic irgolic force-pushed the pythonscript-console branch 2 times, most recently from f857549 to e3de711 Compare February 1, 2021 21:46
Copy link
Member

@markotoplak markotoplak left a comment

Choose a reason for hiding this comment

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

We need to ensure that old workflows keep the same functionality. For old workflows, the in-process kernel needs to be used.

To achieve this, the settings version needs to increase in this PR, and then, if old settings are detected, enable the old kernel for that process. If there are other changes to the behavior (I might have missed something), also consider a compatibility mode as in #3810.

This is a serious blocker, but fortunately easy to fix.

@irgolic irgolic force-pushed the pythonscript-console branch 2 times, most recently from 2b3a8a4 to 7c3cf63 Compare July 16, 2021 19:08
If flicking in-process on and off, before this change,
it could get stuck on 'Injecting Variables'
Orange already does its own patching, and does not expose
file descriptors for the patched out streams. Even if this
functionality is added to orange and canvas-core, many test
runners (JetBrains) do not, and the only case handled in the
ipykernel library is for pytest.
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