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.

Sign up for GitHub

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

Jump to bottom

Support for arbitrary notebook kernel execution #179258

Merged
merged 8 commits into from
Apr 6, 2023
Merged

Support for arbitrary notebook kernel execution #179258

merged 8 commits into from
Apr 6, 2023

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented Apr 5, 2023 •
edited
Loading

@DonJayamanne DonJayamanne requested a review from roblourens April 5, 2023 12:53
@DonJayamanne DonJayamanne marked this pull request as ready for review April 5, 2023 12:54
@vscodenpa vscodenpa added this to the April 2023 milestone Apr 5, 2023
@DonJayamanne
Copy link
Contributor Author

Will need to add some tests, but I would like to get some early feedback before I spend too much time on this approach.
Don't feel too happy as we're now duplicating the events, events for cell execution and another for notebook execution.

Perhaps it could be merged into a single event argument, not sure that increases readability or makes it more complicated.

roblourens
roblourens reviewed Apr 5, 2023
View reviewed changes
Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

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

Of the internal stuff (NotebookExecutionStateService and stuff that references it) do you think some of that could be made to be shared between "cell executions" and "notebook executions", rather than duplicating everything into separate events and methods? Or do you think that would make it more awkward to deal with?

It would make sense to me to basically have a single concept of execution overall, a single listener that you have to listen to, which fires updates about executions, and the execution might be for a cell or it might be for the notebook as a whole.

src/vs/workbench/services/extensions/common/extensionsApiProposals.ts Outdated Show resolved Hide resolved
@DonJayamanne
Copy link
Contributor Author

, and the execution might be for a cell or it might be for the notebook as a whole.

I agree, thats exactly what I wanted to do, but wasn't sure about it.

@DonJayamanne DonJayamanne force-pushed the donjayamanne/kernelExecution branch from 5775bf9 to 262f98e Compare April 5, 2023 19:35
@DonJayamanne
Copy link
Contributor Author

@roblourens merged into a single event, thats all.
Didn't want to unify anything else, else having too many conditions could just complicate the code.
Let me know your thoughts. I like what we have now, specially the fact that internally we have a single event.

roblourens
roblourens previously approved these changes Apr 5, 2023
View reviewed changes
src/vs/workbench/api/common/extHostNotebookKernels.ts Show resolved Hide resolved
src/vs/workbench/api/common/extHostNotebookKernels.ts Outdated Show resolved Hide resolved
src/vscode-dts/vscode.proposed.notebookExecution.d.ts
* When {@linkcode NotebookExecution.start start()} is called on the execution task, it causes the Notebook to enter a executing state .
* When {@linkcode NotebookExecution.end end()} is called, it enters the idle state.
*/
export interface NotebookExecution {
Copy link
Member

Choose a reason for hiding this comment

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

I suppose normally I'd say this should start automatically when you create it, and then be disposable, but we should probably use start/end just to match the cell execution pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, thats the pattern is tried to stick with,
I thought of just returning a IDisposable instead of NotebookExecution, that was much simpler, but didn't seem to line up with the cell exec code.

@roblourens
Copy link
Member

Good stuff, thanks for writing all of this!

rebornix
rebornix previously approved these changes Apr 5, 2023
View reviewed changes
Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

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

Looks great. It's a good fix and worth bringing to the next API sync meeting

@DonJayamanne DonJayamanne dismissed stale reviews from rebornix and roblourens via e85c23c April 6, 2023 00:03
@DonJayamanne DonJayamanne force-pushed the donjayamanne/kernelExecution branch from e85c23c to a81c11e Compare April 6, 2023 00:07
@DonJayamanne DonJayamanne enabled auto-merge (squash) April 6, 2023 00:08
@DonJayamanne
Copy link
Contributor Author

@roblourens @rebornix Please re-review, added testes, and had to get the latest stuff from main

@DonJayamanne DonJayamanne merged commit ab33543 into main Apr 6, 2023
@DonJayamanne DonJayamanne deleted the donjayamanne/kernelExecution branch April 6, 2023 00:21
@github-actions github-actions bot locked and limited conversation to collaborators May 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rebornix rebornix rebornix approved these changes

@roblourens roblourens Awaiting requested review from roblourens

Assignees

@DonJayamanne DonJayamanne

Labels
None yet
Projects
None yet
Milestone
April 2023
Development

Successfully merging this pull request may close these issues.
4 participants
@DonJayamanne @roblourens @rebornix @vscodenpa