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

KernelPush #5565

Merged
merged 26 commits into from
Apr 21, 2021
Merged

KernelPush #5565

merged 26 commits into from
Apr 21, 2021

Conversation

IanMatthewHuff
Copy link
Member

For #

  • 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 enhancements.
  • 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).

@IanMatthewHuff
Copy link
Member Author

@DonJayamanne I've had the branch up. But figure that it might be easier to discuss issues with this actually up as a draft PR, so I'm putting this up here so we can discuss changes easier.

@IanMatthewHuff
Copy link
Member Author

IanMatthewHuff commented Apr 20, 2021

Remaining work tracker:

  • Preloads
  • onDidChangeActiveNotebookKernel replacement
  • NotebookCommunication => NotebookController for widgets
  • Interrupt
  • Check dispose pattern? Seeing issue currently
  • PreferredKernel
  • Telemetry for kernel searching / providing back in
  • Cleanup / Naming (remove VSCodeKernelPicker provider and VsCodeNotebookKernelMetadata)
  • Remove usage of ActiveNotebookEditor.kernel
  • Cancellation token when searching for kernels?
  • Supported languages
  • Update remote details
  • Tests - Run locally and fix up what is needed.

@@ -107,6 +102,9 @@ export class VSCodeNotebook implements IVSCodeNotebook {
): Disposable {
return notebook.registerNotebookKernelProvider(selector, provider);
}
public createNotebookController(id: string, selector: NotebookSelector, label: string, handler?: NotebookExecutionHandler, preloads?: NotebookKernelPreload[]): NotebookController {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about making this static, we don't really need an instance method on this (this way we know that there's no state information or the like).
E.g. my immediate assumption was there's something that ties this controller to the instance of this class

}

private onDidChangeExtensions() {
// IANHU: Need to invalidate kernels here?
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious what were we planning on putting here?
Is this for if user installs Python extenison?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I believe that's what it was for before. Back in the old pull provider we would signal onDidChangeKernels when extensions changed. Now we need to instead dispose and regenerate NotebookControllers. Just saving this with a todo: as I think we can do it after most everything else is working (i.e. not a super important scenario, you can just close and open the file to get a new list).

@IanMatthewHuff
Copy link
Member Author

IanMatthewHuff commented Apr 20, 2021

Issues to discuss with VS Code:

  • Make sure that details has been added in the newest version of NotebookController
  • isPreferred is getting set on an item, but it's not being selected, do we set this somehow?
  • supportedLanguages == undefined used to mean all languages, but now undefined is not an option
  • If you don't pick an initial kernel, then the association event is never fired

@IanMatthewHuff IanMatthewHuff marked this pull request as ready for review April 21, 2021 00:53
@IanMatthewHuff IanMatthewHuff requested a review from a team as a code owner April 21, 2021 00:53
@IanMatthewHuff
Copy link
Member Author

IanMatthewHuff commented Apr 21, 2021

For anyone on the team looking. Don and I have been working on this today and I wanted to put it up for others to take a peek if they want. But we're going to have to fix up more of this tomorrow. Widgets for sure will not be ready, as well as a number of other issues. I'm going to keep working on this tonight and see where we can get. At a minimum we'll push an extension in the morning to make sure that the extension doesn't just straight up crash.

KERNELPUSH marks the known spots in code that still need attention

@IanMatthewHuff IanMatthewHuff changed the title DRAFT - KernelPush KernelPush Apr 21, 2021
@IanMatthewHuff IanMatthewHuff merged commit 3c7e93c into main Apr 21, 2021
@IanMatthewHuff IanMatthewHuff deleted the dev/ianhu/kernelPush2 branch April 21, 2021 15:09
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.

2 participants