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

Main thread runtime notebook kernels #5926

Merged
merged 78 commits into from
Jan 10, 2025
Merged

Conversation

seeM
Copy link
Contributor

@seeM seeM commented Jan 9, 2025

This PR adds notebook kernels for language runtimes in the main thread. Before this, runtime kernels lived in the positron-notebook-controllers extension. Having kernels in the main thread gives us more control over runtime session management. Addresses #2671.

Should also address #4224 because of the way we manage notebook runtime sessions.

Some changes were required in the runtime session service in order to make this possible.

Release Notes

New Features

  • Positron's notebook kernels now live in the main thread rather than in an extension. They should also be more stable. Since this was a big change, the feature can be disabled via the positron.runtimeNotebookKernel.enable setting.

Bug Fixes

QA Notes

I'll run E2E tests against this branch. Playing around with notebooks should also feel stable.

Should also address #4224.

Copy link

github-actions bot commented Jan 9, 2025

E2E Tests 🚀  ?
This PR will run tests tagged with: @critical

@seeM
Copy link
Contributor Author

seeM commented Jan 9, 2025

E2E test failure should be fixed by posit-dev/qa-example-content#42

@seeM seeM requested a review from jmcphers January 9, 2025 17:56
jmcphers
jmcphers previously approved these changes Jan 10, 2025
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

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

Whew, epic PR! Looks clean and ran well. The optional cell execution info is nice too:

image

properties: {
[POSITRON_RUNTIME_NOTEBOOK_KERNEL_ENABLED_KEY]: {
type: 'boolean',
default: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is CAUTION Experimental, should it default to false for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I ended up spending more time testing + hardening the PR than I initially intended on, so it turned out not to be CAUTION Experimental in the end. I've updated the description to reflect that in eb2315f.

@seeM seeM merged commit 08c9ce4 into main Jan 10, 2025
6 checks passed
@seeM seeM deleted the 2671-remove-notebook-session-service branch January 10, 2025 11:58
@github-actions github-actions bot locked and limited conversation to collaborators Jan 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants