Skip to content

Support ipywidgets in the kernel work#11444

Merged
rchiodo merged 33 commits into
masterfrom
rchiodo/raw_ipywidgets_2
Apr 28, 2020
Merged

Support ipywidgets in the kernel work#11444
rchiodo merged 33 commits into
masterfrom
rchiodo/raw_ipywidgets_2

Conversation

@rchiodo

@rchiodo rchiodo commented Apr 27, 2020

Copy link
Copy Markdown

For #10771

Rework how we create a kernel for the raw kernel support. Instead of implementing Kernel.IKernel interface ourselves, just derive from the default one but with using a special websocket.

This websocket (called RawSocket) talks directly to the ZMQ channels (which also eliminates our need to use the enchannel stuff).

However to prevent unnecessary serialization (as the default kernel serializes/deserializes messages from the websocket), the default kernel js file is changed to eliminate serialization.

Modified the notebook.functional test and ipywidget functional test to also test with the raw kernel.

@rchiodo rchiodo self-assigned this Apr 27, 2020
@rchiodo rchiodo added the no-changelog No news entry required label Apr 27, 2020
Comment thread package.json Outdated
Comment thread src/test/datascience/notebook.functional.test.ts
} else if (message === InteractiveWindowMessages.LoadAllCellsComplete) {
const notebook = this.getNotebook();
if (!notebook) {
traceError(

@rchiodo rchiodo Apr 27, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

THis was firing during tests when running the interactive window. #Resolved

return;
}
this.ipyWidgetMessageDispatcher = this.widgetMessageDispatcherFactory.create(this.notebookIdentity);
if (!this.ipyWidgetMessageDispatcher) {

@rchiodo rchiodo Apr 27, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Seemed odd that we'd recreate this object every time we asked for a dispatcher. #Resolved

// Then create the real kernel. We will remap its serialize/deserialize functions
// to do nothing so that we can control serialization at our socket layer.
if (!nonSerializingKernel) {
nonSerializingKernel = require('@jupyterlab/services/lib/kernel/nonSerializingKernel') as typeof import('@jupyterlab/services/lib/kernel/default'); // NOSONAR

@IanMatthewHuff IanMatthewHuff Apr 27, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A comment pointing to the post install step where we make this would be helpful, especially for something this funky. #Resolved

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure will do.


In reply to: 416055782 [](ancestors = 416055782)

Comment thread build/ci/postInstall.js
Comment thread src/client/datascience/interactive-ipynb/nativeEditorStorage.ts Outdated
public async asWebviewUri(localResource: Uri): Promise<Uri> {
// Make a copy of the local file if not already in the correct location
if (!localResource.fsPath.startsWith(this._rootScriptFolder)) {
if (!localResource.fsPath.toLocaleLowerCase().startsWith(this._rootScriptFolder.toLocaleLowerCase())) {

@DonJayamanne DonJayamanne Apr 27, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This isn't true on unix.
Did you come across an instance where you had issues with case on windows?
#Resolved

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah during running functional tests.


In reply to: 416040444 [](ancestors = 416040444)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I need an areSubPathsSame or something like it.


In reply to: 416060240 [](ancestors = 416060240,416040444)

Comment thread src/client/datascience/jupyter/jupyterExporter.ts Outdated
Comment thread src/client/datascience/raw-kernel/rawKernel.ts Outdated
Comment thread src/client/datascience/raw-kernel/rawKernel.ts Outdated
throw new Error('No session available?');
public shutdown(): Promise<void> {
suppressShutdownErrors(this.realKernel);
return this.realKernel.shutdown().catch((_exc) => {

@DonJayamanne DonJayamanne Apr 27, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd log the errors here, else it will never get logged anywhere. #WontFix

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Don't care about shutdown errors. They're irrelevant.


In reply to: 416047279 [](ancestors = 416047279)

Comment thread src/client/datascience/raw-kernel/rawSocket.ts
sendMessage(message: KernelMessage.IMessage): void;
subscribe(handlerFunc: (message: KernelMessage.IMessage) => void): void;
// tslint:disable-next-line: no-any
subscribe(handlerFunc: (message: KernelMessage.IMessage) => void, errorHandler?: (exc: any) => void): void;

@DonJayamanne DonJayamanne Apr 27, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please could you add comments.
Is this a subscription for outgoing or incoming messages? #WontFix

@DonJayamanne DonJayamanne Apr 27, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not sure how error handler works here?
Is this when one of the other handlers for incoming messages bombs or when there's an error in sending messages. #WontFix

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not going to do that. We're not using this anymore. A future submission will remove this type entirely.


In reply to: 416056786 [](ancestors = 416056786)

Comment thread src/client/datascience/raw-kernel/rawSocket.ts
@codecov-io

codecov-io commented Apr 27, 2020

Copy link
Copy Markdown

Codecov Report

Merging #11444 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11444   +/-   ##
=======================================
  Coverage    0.00%    0.00%           
=======================================
  Files          11       11           
  Lines          36       36           
  Branches        4        4           
=======================================
  Misses         36       36           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c2d0a3...7c2d0a3. Read the comment docs.

@rchiodo
rchiodo requested a review from DonJayamanne April 27, 2020 21:39
@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 1 Security Hotspot to review)
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rchiodo
rchiodo merged commit dd82fc3 into master Apr 28, 2020
@rchiodo
rchiodo deleted the rchiodo/raw_ipywidgets_2 branch April 28, 2020 00:56
@lock lock Bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants