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

Integrate notebook session management from positron-notebook-controllers extension into runtime session service #2671

Closed
seeM opened this issue Apr 5, 2024 · 4 comments
Closed

Integrate notebook session management from positron-notebook-controllers extension into runtime session service #2671

seeM opened this issue Apr 5, 2024 · 4 comments
Assignees
@seeM
Labels
area: api Issues related to API category. area: notebooks Issues related to Notebooks category. area: runtimes Issues related to Language Runtimes
Milestone
2025.02.0 Pre-Rel...

Comments

@seeM
Copy link
Contributor

seeM commented Apr 5, 2024 •
edited
Loading

The NotebookSessionService class duplicates a bunch of code from the runtime session service, and really doesn't need to exist. We should bring what's required into the RuntimeSessionService and expose the rest via the API.

Ultimately, we may want the entirety of the positron-notebook-controllers extension to live in Positron. It might be worth trying to tackle that at the same time.

In order to satisfy the requirements for restoring/starting/shutting down notebook sessions in the runtime session service, we may want to split the current startNewRuntimeSession function into separate functions for console sessions vs notebook sessions, since (a) they require different sets of arguments (console takes runtimeId, notebook takes languageId and notebookUri), and (b) they require different functionality (notebook gets the preferred runtime for the language, and keys on notebookUri instead of runtimeId).

We may also do the same for selectLanguageRuntime.
@seeM seeM added area: api Issues related to API category. area: core Issues related to Core category. area: notebooks Issues related to Notebooks category. labels Apr 5, 2024
@seeM seeM added this to the Release Candidate milestone Apr 5, 2024
@seeM
Copy link
Contributor Author

seeM commented Apr 5, 2024

A big difference between the notebook UI and console UI is that in the notebook UI there are far fewer guarantees around when a user attempts to start a session, shutdown a session, restart a session, or change runtimes.

A user can:

  • Click a restart button – thankfully this is the one case where we can disable the restart button when it isn't safe to restart the session (when the session is starting, shutting down, or exited).
  • Open a notebook, starting a session.
  • Close a notebook, ending a session.
  • Switch kernels, ending one session and starting another.

Each of these can happen at any moment, meaning that we need safety between each possible state of: starting, shutting down, and restarting.

This is what I'd expect to happen in each of case:

  1. Open a notebook:
    1. Previous session is starting: Do nothing.
    2. Previous session is shutting down: Finish the shutdown, then start the new session.
    3. Previous session is restarting: Shouldn't be possible.
  2. Close a notebook:
    1. Previous session is starting: Finish the start, then shutdown the started session.
    2. Previous session is shutting down: Do nothing.
    3. Previous session is restarting: Finish the restart, then shutdown the restarted session.
  3. Switch kernels:
    1. Previous session is starting: Finish the start, shutdown the previous session, then start the new session.
    2. Previous session is shutting down: Finish the shutdown, then start the new session.
    3. Previous session is restarting: Finish the restart, shutdown the restarted session, then start the new session.
  4. Restart:
    1. Previous session is starting: Do nothing.
    2. Previous session is shutting down: Finish the shutdown, then start the new session.
    3. Previous session is restarting: Do nothing.

@seeM seeM added area: runtimes Issues related to Language Runtimes and removed area: core Issues related to Core category. labels Apr 9, 2024
@seeM seeM mentioned this issue May 29, 2024
Merged
@seeM seeM self-assigned this Oct 15, 2024
@seeM
Copy link
Contributor Author

seeM commented Oct 15, 2024

I'm going to be tackling this as a step toward improving notebook runtime stability, including the following issues:

@seeM seeM mentioned this issue Nov 12, 2024
Merged
seeM added a commit that referenced this issue Nov 13, 2024
@seeM
Add Positron instantiation service test helpers; refactor tests to us…
fbd8b46 
…e runtime services (#5335)

This is in preparation for writing unit tests for the runtime session
service since I intend on making changes to it for #2671.

The main addition is the `positronWorkbenchInstantiationService`
function, which sets up a test instantiation service with a bunch of
test dummies, following upstream's `workbenchInstantiationService`
function. I also refactored our existing unit tests to use the new
function.

#### QA Notes

1. Positron unit tests should pass. They can be run locally with:

    ```sh
    ./scripts/test-positron.sh
    ```
2. There are a few minor code changes to runtime-related code, mainly to
fix leaked disposables, which could use a review.
@seeM seeM mentioned this issue Nov 15, 2024
Merged
@petetronic petetronic mentioned this issue Nov 18, 2024
Closed
@seeM seeM mentioned this issue Nov 26, 2024
Open
seeM added a commit that referenced this issue Nov 28, 2024
@seeM
Improve runtime session service stability (#5380)
a36f4d7 
The aim of this PR is to improve the stability of the runtime session
service, with a slight focus on notebooks.

This is another step toward
#2671.

I've also added an extensive suite of unit tests for the runtime session
service, for two reasons:

1. Some of the issues we're seeing with notebooks are intermittent and
therefore tricky to reproduce without tests.
2. I wanted to ensure that I didn't break any existing behavior with
these changes as well as more planned changes to add notebook-runtime
methods to the extension API.
This was referenced Dec 5, 2024
Merged
Merged
seeM added a commit that referenced this issue Dec 10, 2024
@seeM
Retrieve notebook sessions via the positron.runtime API (#5632)
0a51167 
The goal of this PR is to stop relying on the
positron-notebook-controller extension's notebook session service
tracking the running notebook sessions and instead use the existing
`positron.runtime.getNotebookSession` method. It's another step toward
removing the notebook session service altogether.

Related to #2671.

### QA Notes

Tests should continue to pass and opening/closing/restarting/executing
notebooks should feel stable in general.
@seeM seeM mentioned this issue Dec 10, 2024
Merged
seeM added a commit that referenced this issue Dec 11, 2024
@seeM
Fix inconsistency when concurrently starting a session and `session.s…
9e85741 
…tart()` rejects (#5628)

Now both the original call and any concurrent calls will reject. Related to #2671.
seeM added a commit that referenced this issue Dec 11, 2024
@seeM
Move the hasRunningNotebookSession context out of the notebook sess…
70ed8a3 
…ion service (#5688)

Another step toward removing the notebook session service entirely
(#2671).

### QA Notes

This shouldn't change any behavior, although I did fix some edge cases
where the context was not set correctly.

Opening a notebook, restarting it, closing and reopening it should all
work. The restart button should be deactivated when the session is
exited.
seeM added a commit that referenced this issue Dec 13, 2024
@seeM
Fix inconsistency when concurrently starting a session and `session.s…
0522b95 
…tart()` rejects (#5628)

Now both the original call and any concurrent calls will reject. Related to #2671.
@petetronic petetronic removed this from the 2025.01.0 Pre-Release milestone Jan 7, 2025
@petetronic petetronic added this to the 2025.02.0 Pre-Release milestone Jan 7, 2025
@seeM seeM mentioned this issue Jan 9, 2025
Merged
seeM added a commit that referenced this issue Jan 10, 2025
@seeM
Main thread runtime notebook kernels (#5926)
08c9ce4 
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

- Cancelling the ipykernel installation modal no longer breaks kernel
selection (#4224).

### QA Notes

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

Should also address #4224.
@seeM
Copy link
Contributor Author

seeM commented Jan 14, 2025

For QA: Main thread notebook kernels should be active by default in the latest release. We should try a bunch of different operations in notebooks and ensure that they all work as before.

@testlabauto
Copy link
Contributor

Verified Fixed

Positron Version(s) : 2025.02.0-59
OS Version          : OSX

Test scenario(s)

Unable to detect any differences in notebook functionality with this release.

Link(s) to TestRail test cases run or created:

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@seeM seeM

Labels
area: api Issues related to API category. area: notebooks Issues related to Notebooks category. area: runtimes Issues related to Language Runtimes
Projects
None yet
Milestone
2025.02.0 Pre-Release
Development

No branches or pull requests
3 participants
@seeM @petetronic @testlabauto