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

[WIP] OWPythonScript: Rewrite #4402

Closed
wants to merge 45 commits into from

Conversation

irgolic
Copy link
Member

@irgolic irgolic commented Feb 7, 2020

Description of changes

The code editor (a QPlainTextEdit) is replaced with Qutepart (a QPlainTextEdit subclass). The source code for this was copied into Orange.
The console is replaced with a jupyter qtconsole, which runs a customized IPython kernel.

The main process and kernel process communicate in JSON, so variables are pickled, base64 encoded, sent, decoded, unpickled. This is very space inefficient. Perhaps something like https://jsonpickle.github.io/ could alleviate the issue?

The library stores files in a real folder, allowing users to import from their other scripts. Scripts aren't saved by default.

Includes
  • Code changes
  • Tests
  • Documentation

@codecov
Copy link

codecov bot commented Feb 7, 2020

Codecov Report

Merging #4402 into master will increase coverage by 3.20%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #4402      +/-   ##
==========================================
+ Coverage   84.24%   87.45%   +3.20%     
==========================================
  Files         283      405     +122     
  Lines       57873    74084   +16211     
==========================================
+ Hits        48756    64787   +16031     
- Misses       9117     9297     +180     

@irgolic irgolic force-pushed the owpythonscript-spruce-up branch 2 times, most recently from 96dc73d to aaa8a20 Compare February 13, 2020 15:05
@irgolic irgolic force-pushed the owpythonscript-spruce-up branch from aaa8a20 to 74ccd9a Compare March 16, 2020 20:32
@irgolic irgolic changed the title [RFC][WIP] OWPythonScript: Spruce up [RFC][WIP] OWPythonScript: Rewrite Mar 16, 2020
@irgolic irgolic changed the title [RFC][WIP] OWPythonScript: Rewrite [WIP] OWPythonScript: Rewrite Mar 16, 2020
@irgolic irgolic force-pushed the owpythonscript-spruce-up branch 2 times, most recently from b1b3876 to 2151845 Compare March 17, 2020 22:21
@irgolic irgolic force-pushed the owpythonscript-spruce-up branch 3 times, most recently from cf7fce8 to aaf31c8 Compare March 19, 2020 17:32
@irgolic irgolic force-pushed the owpythonscript-spruce-up branch 2 times, most recently from 6c7ca4b to a1ebfca Compare May 20, 2020 18:05
@irgolic irgolic force-pushed the owpythonscript-spruce-up branch from a1ebfca to a203c2b Compare July 13, 2020 17:40
@irgolic irgolic force-pushed the owpythonscript-spruce-up branch from a203c2b to c97d377 Compare July 31, 2020 23:10
@irgolic irgolic force-pushed the owpythonscript-spruce-up branch from c97d377 to 2e18284 Compare August 16, 2020 11:22
irgolic added 12 commits August 16, 2020 13:31
…on qtconsole

- Introduce OrangeIPythonKernel for script running functionality
- Modelled after Jupyter lab
- Editor is styled with Qutepart's built-in highlighter
- Console is styled with pygments
The subclassed widget uses a lot of super protected methods, mostly in
manipulating prompt appearance -- any bugs which arise are likely to be purely
cosmetic. Still, I'm constraining the required version to the current latest version.

- Implement kernel interrupt on new run_script request or ctrl+C
by adding a fake function signature and return statement,
whose variables are bolded on in_ signal connection/out_ variable collection.

- Removed info box
- TODO add tooltips to parameters
@irgolic irgolic force-pushed the owpythonscript-spruce-up branch 11 times, most recently from d5f2b5a to 8b61ad5 Compare August 17, 2020 19:40
@irgolic irgolic force-pushed the owpythonscript-spruce-up branch 3 times, most recently from 9f1ca66 to 3f4ad36 Compare August 17, 2020 21:44
@irgolic
Copy link
Member Author

irgolic commented Aug 17, 2020

This is pretty much finished, just documentation left.

... Except, tests show [IPKernelApp] WARNING | Parent appears to have exited, shutting down.. I've not found a way to gracefully shut down IPYKernels (see OWPythonScript.onDeleteWidget), it seems that it's linked to a heartbeat pinging the parent process to check if it's exited. This isn't really a problem, except I've seen tests timeout sometimes on waiting for script execution, which might be related.
Although looking at ipykernel.tests.test_embed_kernel.py, they run some tests as @flaky(max_runs=3). If tests timing out remains a problem, this will most likely solve it.

@ales-erjavec, could you take a look to see if I didn't royally screw something up?
@ajdapretnar, I've fixed the comments from our discussion earlier today.
@janezd, could you play around with the widget a bit, tell me what you think about the UX?

Also, if anyone wants any specific keyboard shortcuts in the widget, let me know! The editor already supports stuff like moving lines (Alt+Up/Down), see the WIP documentation for more.

@irgolic irgolic requested review from janezd and ales-erjavec August 17, 2020 22:39
@irgolic irgolic force-pushed the owpythonscript-spruce-up branch from ae55728 to 703f65a Compare August 18, 2020 20:34
@irgolic
Copy link
Member Author

irgolic commented Aug 22, 2020

I've gone ahead and added completions in the editor as well, modeled after spyder-ide. What's left is tooltips, like in the console, but I'm leaving that for some other time.

The only issue left is performance. The kernel starts up slowly; it takes at least a couple of seconds each time. Launching qtconsole as python -m qtconsole is instant, so I must be doing something wrong. Note, it exhibited this behavior from the very beginning. If this isn't solved it's not that big a deal, but if someone figures it out, I'd be immensely grateful.

@irgolic
Copy link
Member Author

irgolic commented Sep 25, 2020

Closed in favor of biolab/orange3-prototypes#219.

@irgolic irgolic closed this Sep 25, 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.

1 participant