Ensure kernel points to live kernel connection when starting remote kernels #10573

DonJayamanne · 2022-06-26T18:12:13Z

codecov-commenter · 2022-06-26T19:14:06Z

Codecov Report

Merging #10573 (b37d24c) into main (1d727e8) will increase coverage by 0%.
The diff coverage is 70%.

@@           Coverage Diff           @@
##            main   #10573    +/-   ##
=======================================
  Coverage     63%      63%            
=======================================
  Files        477      479     +2     
  Lines      34162    34378   +216     
  Branches    5546     5576    +30     
=======================================
+ Hits       21597    21812   +215     
+ Misses     10506    10503     -3     
- Partials    2059     2063     +4

Impacted Files	Coverage Δ
src/kernels/types.ts	`100% <ø> (ø)`
src/notebooks/controllers/types.ts	`100% <ø> (ø)`
src/kernels/notebookControllerWrapper.ts	`34% <34%> (ø)`
src/kernels/common/delayedFutureExecute.ts	`5% <50%> (+1%)`	⬆️
src/kernels/kernelProvider.base.ts	`93% <68%> (-3%)`	⬇️
src/notebooks/controllers/kernelConnector.ts	`82% <71%> (-1%)`	⬇️
...rc/notebooks/controllers/controllerRegistration.ts	`85% <80%> (-2%)`	⬇️
src/kernels/kernelConnectionMetadataWrapper.ts	`80% <80%> (ø)`
src/interactive-window/interactiveWindow.ts	`75% <85%> (+<1%)`	⬆️
...ebooks/controllers/notebookIPyWidgetCoordinator.ts	`89% <88%> (+2%)`	⬆️
... and 41 more

IanMatthewHuff · 2022-06-27T09:49:58Z

Looks like there is a new failed test: 3rd Party Kernel Service API Start Kernel

src/kernels/notebookControllerWrapper.ts

src/interactive-window/interactiveWindow.ts

src/notebooks/controllers/controllerRegistration.ts

src/notebooks/controllers/vscodeNotebookController.ts

src/kernels/ipywidgets/commonMessageCoordinator.ts

sadasant · 2022-07-01T21:24:13Z

src/kernels/ipywidgets/commonMessageCoordinator.ts

+                        // However its possble the kernel is not yet ready at that point in time.
+                        // We could solve this issue easily by not executing cells, until the webview is ready,
+                        // however that would have significant performance implications.
+                        // Hence for ipywidgets, once the webview has completely been initialized, we can attempt to re-render the widgets.


I love this!! Thank you 🙏

src/kernels/ipywidgets/commonMessageCoordinator.ts

src/kernels/kernelProvider.base.ts

src/notebooks/controllers/controllerRegistration.ts

src/kernels/kernelProvider.web.ts

src/kernels/types.ts

src/notebooks/controllers/controllerRegistration.ts

rchiodo · 2022-07-01T23:31:27Z

src/standalone/api/kernelApi.ts

-            const translatedConnection = Object.freeze(readWriteConnection) as KernelConnectionMetadata;
+
+            const translatedConnection = Object.freeze(
+                JSON.parse(JSON.stringify(connection))


rchiodo · 2022-07-01T23:35:48Z

I wonder if we could tag all of this code with 'Unnecessary if we could get a new API for creating controllers'? Maybe a comment in some key places?

DonJayamanne force-pushed the issue9865 branch from 8817f9a to 85e0344 Compare June 26, 2022 22:00

DonJayamanne marked this pull request as ready for review June 26, 2022 22:34

DonJayamanne requested a review from a team as a code owner June 26, 2022 22:34

rchiodo reviewed Jun 27, 2022

View reviewed changes

src/kernels/notebookControllerWrapper.ts Show resolved Hide resolved

rchiodo approved these changes Jun 27, 2022

View reviewed changes

DonJayamanne force-pushed the issue9865 branch 2 times, most recently from 8d88724 to 0d4a271 Compare June 28, 2022 18:08

amunger reviewed Jun 29, 2022

View reviewed changes

src/interactive-window/interactiveWindow.ts Outdated Show resolved Hide resolved

amunger reviewed Jun 29, 2022

View reviewed changes

src/notebooks/controllers/controllerRegistration.ts Show resolved Hide resolved

amunger reviewed Jun 29, 2022

View reviewed changes

src/notebooks/controllers/vscodeNotebookController.ts Outdated Show resolved Hide resolved

DonJayamanne force-pushed the issue9865 branch 4 times, most recently from 00b7111 to 77657b1 Compare July 1, 2022 07:19