Fixes related to loading widget scripts #10459

DonJayamanne · 2022-06-14T21:54:46Z

Fixes #10434, #10060, #8834, #10319, #8241

DonJayamanne · 2022-06-14T21:55:19Z

src/kernels/common/baseJupyterSession.ts

@@ -379,7 +379,9 @@ export abstract class BaseJupyterSession implements IJupyterSession {
                            const jupyterLabSerialize =
                                require('@jupyterlab/services/lib/kernel/serialize') as typeof import('@jupyterlab/services/lib/kernel/serialize'); // NOSONAR
                            const message =
-                                typeof msg.msg === 'string' ? jupyterLabSerialize.deserialize(msg.msg) : msg.msg;
+                                typeof msg.msg === 'string' || msg.msg instanceof ArrayBuffer


This was incorrect, when we were firing this event we weren't deserializing buffers

src/kernels/ipywidgets-message-coordination/cdnWidgetScriptSourceProvider.ts

DonJayamanne · 2022-06-14T22:00:34Z

src/kernels/ipywidgets-message-coordination/cdnWidgetScriptSourceProvider.ts

+    }
+
+    private async configureWidgets(): Promise<void> {
+        if (this.configuredScriptSources.length !== 0) {


Moved code related to displaying messages for CDN into the CDN class, instaead of having that elsewhere.

DonJayamanne · 2022-06-14T22:01:11Z

src/kernels/ipywidgets-message-coordination/ipyWidgetScriptSource.ts

+                });
+        } else if (message === IPyWidgetMessages.IPyWidgets_IsOnline) {
+            const isOnline = (payload as { isOnline: boolean }).isOnline;
+            this.isWebViewOnline.resolve(isOnline);


Helps us determine from exension host whether the webview has access to the internet (i.e. whether we can access CDN resources from webview)

DonJayamanne · 2022-06-14T22:01:39Z

src/kernels/ipywidgets-message-coordination/ipyWidgetScriptSource.ts

@@ -127,7 +118,7 @@ export class IPyWidgetScriptSource {
        }

        if (!this.kernel) {
-            this.kernel = await this.kernelProvider.get(this.document.uri);
+            this.kernel = this.kernelProvider.get(this.document.uri);


unnecessary await

codecov-commenter · 2022-06-15T01:41:19Z

Codecov Report

Merging #10459 (c3730db) into main (94de3cd) will increase coverage by 0%.
The diff coverage is 80%.

❗ Current head c3730db differs from pull request most recent head 9aba5b1. Consider uploading reports for the commit 9aba5b1 to get more accurate results

@@           Coverage Diff           @@
##            main   #10459    +/-   ##
=======================================
  Coverage     71%      71%            
=======================================
  Files        480      486     +6     
  Lines      29003    29157   +154     
  Branches    4885     4902    +17     
=======================================
+ Hits       20640    20756   +116     
- Misses      6421     6449    +28     
- Partials    1942     1952    +10

Impacted Files	Coverage Δ
...tive-window/debugger/jupyter/kernelDebugAdapter.ts	`58% <ø> (-2%)`	⬇️
src/kernels/common/baseJupyterSession.ts	`78% <0%> (ø)`
src/platform/common/platform/types.node.ts	`100% <ø> (ø)`
src/telemetry/types.ts	`100% <ø> (ø)`
...ssage-coordination/remoteIPyWidgetScriptManager.ts	`63% <63%> (ø)`
...sage-coordination/cdnWidgetScriptSourceProvider.ts	`71% <71%> (ø)`
...message-coordination/baseIPyWidgetScriptManager.ts	`76% <76%> (ø)`
...gets-message-coordination/ipyWidgetScriptSource.ts	`78% <76%> (+<1%)`	⬆️
...sage-coordination/ipyWidgetScriptSourceProvider.ts	`75% <81%> (-10%)`	⬇️
src/platform/common/platform/fileSystem.node.ts	`82% <84%> (+2%)`	⬆️
... and 39 more

DonJayamanne · 2022-06-16T02:17:53Z

src/test/datascience/ipywidgets/samples/beakerx/LICENSE

@@ -0,0 +1,201 @@
+Apache License


I wanted to use the actual extension.js files for tests instead of crafting our own content.
Included the license files along with the extesnsion.js files.

Wondering if some automated check tool is going to flag us for having a different license in the repo. But I guess we'll see if anything like that crops up moving forward.

DonJayamanne · 2022-06-16T02:18:29Z

src/kernels/common/baseJupyterSession.ts

@@ -379,7 +379,9 @@ export abstract class BaseJupyterSession implements IJupyterSession {
                            const jupyterLabSerialize =
                                require('@jupyterlab/services/lib/kernel/serialize') as typeof import('@jupyterlab/services/lib/kernel/serialize'); // NOSONAR
                            const message =
-                                typeof msg.msg === 'string' ? jupyterLabSerialize.deserialize(msg.msg) : msg.msg;
+                                typeof msg.msg === 'string' || msg.msg instanceof ArrayBuffer


Bug i found while testing, we weren't deserializing buffers

DonJayamanne · 2022-06-16T02:20:20Z

src/kernels/ipywidgets-message-coordination/cdnWidgetScriptSourceProvider.ts

+ * Given an widget module name & version, this will attempt to find the Url on a CDN.
+ * We'll need to stick to the order of preference prescribed by the user.
+ */
+export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvider {


Merged the web and node files into one, we don't need to download the cdn files into the local file system anymore.
If we do that then things will not work, e.g. ipyvolume and a few other widgets don't work with that approach.

If user doesn't have access to the internet from the webview, then we'll not use CDN, this way we're consistent. I.e. we always use CDN when there's network connection, else fallback.
If here's no network access we're now displaying a warning message.

DonJayamanne · 2022-06-16T02:21:50Z

src/platform/common/platform/fileSystem.node.ts

-        const uri = vscode.Uri.file(filename);
-        const data = await this.vscfs.readFile(uri);
-        return Buffer.from(data);
+        return fs.readFile(filename);


Using VSC API here is totally unnecessary its very slow.
Since these are local only (the methods are local , hence easier & more efficient to just use native node modules.

DonJayamanne · 2022-06-16T02:23:33Z

news/2 Fixes/10060.md

@@ -0,0 +1 @@
+Ensure IPyWidgets get loaded correctly when loading resources from the CDN, Remote Jupyter or local the Python Environment.


Still requires some more testing, but this is ready for review.
Will look into adding more tests or at least re-enabling some of the widget tests tomorrow.

src/interactive-window/debugger/jupyter/kernelDebugAdapter.ts

src/kernels/ipywidgets-message-coordination/localIPyWidgetScriptManager.node.ts

src/kernels/ipywidgets-message-coordination/nbExtensionsPathProvider.web.ts

rchiodo

Awesome job. Looks like we're going to have ALL widgets working now.

IanMatthewHuff · 2022-06-16T16:52:06Z

src/kernels/ipywidgets-message-coordination/remoteIPyWidgetScriptManager.ts

+        }
+        try {
+            // Value will be an array of the form `['xyz', 'abc']`
+            const items = (output.text as string)


Language conversions like this always feel a bit scary. Like I think that , is a valid file path character on linux. I think since you have the logging in the catch case this should be ok.

Agreed,
Also I don't see why it will work not work as we own this code along with the output generated (I could optionally send a JSON output back but that felt a little overkill for me).

Come to think of it, that might be better.
Let me see if I can add the JSON output, at least we know the output is JSON and dpoesn't need to be parsed in this manner.

Doesn't work as expected, not going to spend more time on that.

amunger · 2022-06-16T20:37:43Z

empty news file

DonJayamanne commented Jun 14, 2022

View reviewed changes

src/kernels/ipywidgets-message-coordination/cdnWidgetScriptSourceProvider.ts Show resolved Hide resolved

DonJayamanne commented Jun 14, 2022

View reviewed changes

DonJayamanne force-pushed the issue10434DataUrl branch from fc173a3 to ff42172 Compare June 15, 2022 01:41

DonJayamanne force-pushed the issue10434DataUrl branch from 4431fd5 to eb4aad2 Compare June 16, 2022 02:15

DonJayamanne changed the title ~~WIP~~ Fixes related to loading widget scripts Jun 16, 2022

DonJayamanne marked this pull request as ready for review June 16, 2022 02:16

DonJayamanne requested a review from a team as a code owner June 16, 2022 02:16

DonJayamanne commented Jun 16, 2022

View reviewed changes

DonJayamanne requested review from IanMatthewHuff, rchiodo, amunger and sadasant June 16, 2022 02:23

rchiodo reviewed Jun 16, 2022

View reviewed changes

src/interactive-window/debugger/jupyter/kernelDebugAdapter.ts Show resolved Hide resolved

rchiodo reviewed Jun 16, 2022

View reviewed changes

src/kernels/ipywidgets-message-coordination/localIPyWidgetScriptManager.node.ts Show resolved Hide resolved

rchiodo reviewed Jun 16, 2022

View reviewed changes

src/kernels/ipywidgets-message-coordination/nbExtensionsPathProvider.web.ts Show resolved Hide resolved

rchiodo approved these changes Jun 16, 2022

View reviewed changes

IanMatthewHuff reviewed Jun 16, 2022

View reviewed changes

IanMatthewHuff approved these changes Jun 16, 2022

View reviewed changes

DonJayamanne force-pushed the issue10434DataUrl branch from 9ff9753 to c3730db Compare June 16, 2022 21:13

Fix loading IPyWidgets from CDN, Remote & Local

9aba5b1

DonJayamanne force-pushed the issue10434DataUrl branch from c3730db to 9aba5b1 Compare June 16, 2022 21:44

DonJayamanne merged commit 4ed91b2 into main Jun 16, 2022

DonJayamanne deleted the issue10434DataUrl branch June 16, 2022 23:24

DonJayamanne mentioned this pull request Jun 16, 2022

Scripts sources related to custom IPyWidgets not loaded #10319

Closed

		@@ -0,0 +1 @@
		Ensure IPyWidgets get loaded correctly when loading resources from the CDN, Remote Jupyter or local the Python Environment.

Fixes related to loading widget scripts #10459

Fixes related to loading widget scripts #10459

Uh oh!

Conversation

DonJayamanne commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amunger commented Jun 16, 2022

Uh oh!

Uh oh!

DonJayamanne commented Jun 14, 2022 •

edited

Loading

codecov-commenter commented Jun 15, 2022 •

edited

Loading