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

Strict IKernel with notebook document linked in core. #10817

Merged
merged 21 commits into from
Jul 20, 2022

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Jul 14, 2022

This PR tries to separate jupyter kernels created for notebook documents and non notebook documents.

The motivation behind this is having a clear separation between the use of kernels created for notebooks or non-notebooks. Currently kernels created for non-notebooks are only created by 3rd party extensions and breaking the 1 to 1 mapping between IKernel to NotebookDocument. That means the core system (notebook and interactive window) needs to handle the undefined case when running getAssociatedNotebookDocument, even if that should never happen.

As @DonJayamanne and I discussed offline, with changes in this PR, internal components only need to care about Kernels that are attached to a NotebookDocument. The src/kernels component will offer

  • `IKernelProvider
  • IKernel which always ties to NotebookDocument

Kernels created by third party only matters to src/kernels component (e.g., execution related code) and API (this is why we have such kernels).

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for feature-requests.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).

Co-authored-by: Don Jayamanne <don.jayamanne@outlook.com>
@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2022

Codecov Report

Merging #10817 (35aa60e) into main (7308db8) will decrease coverage by 10%.
The diff coverage is 77%.

❗ Current head 35aa60e differs from pull request most recent head 1cbacb1. Consider uploading reports for the commit 1cbacb1 to get more accurate results

@@           Coverage Diff            @@
##            main   #10817     +/-   ##
========================================
- Coverage     63%      52%    -11%     
========================================
  Files        486      482      -4     
  Lines      33679    33758     +79     
  Branches    5496     5505      +9     
========================================
- Hits       21297    17750   -3547     
- Misses     10337    14284   +3947     
+ Partials    2045     1724    -321     
Impacted Files Coverage Δ
src/interactive-window/commands/commandRegistry.ts 29% <0%> (-5%) ⬇️
...ctive-window/editor-integration/codeLensFactory.ts 59% <0%> (-30%) ⬇️
...ractive-window/editor-integration/hoverProvider.ts 37% <0%> (-32%) ⬇️
src/interactive-window/interactiveWindow.ts 9% <0%> (-66%) ⬇️
src/kernels/helpers.node.ts 23% <0%> (-46%) ⬇️
src/kernels/variables/debuggerVariables.ts 17% <0%> (-51%) ⬇️
...books/controllers/remoteKernelConnectionHandler.ts 94% <ø> (ø)
.../notebooks/controllers/vscodeNotebookController.ts 71% <0%> (-10%) ⬇️
src/notebooks/debugger/debuggingManagerBase.ts 29% <0%> (-41%) ⬇️
src/notebooks/notebookCommandListener.ts 30% <0%> (-22%) ⬇️
... and 278 more

@rebornix rebornix marked this pull request as ready for review July 19, 2022 00:14
@rebornix rebornix requested a review from a team as a code owner July 19, 2022 00:14
Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

Pretty neat and complete! From my limited perspective, this looks good!

if (notebook) {
this.notebookData.delete(notebook.uri.toString());
}
this.notebookData.delete(kernel.notebook.uri.toString());
Copy link
Member

Choose a reason for hiding this comment

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

Maybe out of scope for this PR, but I thought we removed almost all cases of using uri.toString(). Seems like this should maybe WeakMap the NotebookDocument instead like we do in other places.

Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

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

I like it, nice to just have a .notebook instead of the helper function all over the place.

Copy link
Contributor

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

I think this is changing the 3rd party API.

Before this change, 3rd parties could get kernels from live notebooks. Now they can only find kernels that they've created.

I think the 3rd party kernel provider has to search in the original kernel provider too.

@rchiodo
Copy link
Contributor

rchiodo commented Jul 19, 2022

Sorry I didn't read all the way down to the API section. It's using both. My initial feeling was that the 3rd party provider should provide both but thinking about it more, it makes more sense for the API to do it in the API layer.

Copy link
Contributor

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

I wonder if there still might be problems though with internal code not seeing 3rd party kernels at all? If a 3rd party creates a kernel for a notebook and then the user runs a cell, it won't use it. So from the 3rd party's point of view they're using the wrong kernel.

Copy link
Contributor

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

No it looks like you've handled that case in the KernelConnector code.

@rebornix rebornix merged commit 4910e9b into main Jul 20, 2022
@rebornix rebornix deleted the rebornix/kernel-with-notebook branch July 20, 2022 03:34
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.

6 participants