-
Notifications
You must be signed in to change notification settings - Fork 121
Refactor kernel management to derive status from runtime session and … #10001
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,8 +29,9 @@ import { IPositronNotebookInstance, KernelStatus } from './IPositronNotebookInst | |
import { NotebookCellTextModel } from '../../notebook/common/model/notebookCellTextModel.js'; | ||
import { ICommandService } from '../../../../platform/commands/common/commands.js'; | ||
import { SELECT_KERNEL_ID_POSITRON, SelectPositronNotebookKernelContext } from './SelectPositronNotebookKernelAction.js'; | ||
import { INotebookKernel, INotebookKernelService } from '../../notebook/common/notebookKernelService.js'; | ||
import { INotebookKernelService } from '../../notebook/common/notebookKernelService.js'; | ||
import { IRuntimeSessionService } from '../../../services/runtimeSession/common/runtimeSessionService.js'; | ||
import { RuntimeState } from '../../../services/languageRuntime/common/languageRuntimeService.js'; | ||
import { isEqual } from '../../../../base/common/resources.js'; | ||
import { IPositronWebviewPreloadService } from '../../../services/positronWebviewPreloads/browser/positronWebviewPreloadService.js'; | ||
import { autorun, observableValue } from '../../../../base/common/observable.js'; | ||
|
@@ -44,6 +45,36 @@ interface IPositronNotebookInstanceRequiredTextModel extends IPositronNotebookIn | |
textModel: NotebookTextModel; | ||
} | ||
|
||
/** | ||
* Maps runtime states to their corresponding kernel status values. This mapping | ||
* defines how the notebook UI interprets runtime session states and displays | ||
* them to the user as kernel connection status. As we expand the states we | ||
* report this map will evolve. | ||
* | ||
* States are grouped by their semantic meaning: | ||
* - Connecting: Runtime is initializing or starting | ||
* - Connected: Runtime is operational and ready to execute code | ||
* - Disconnected: Runtime has ended or is shutting down | ||
*/ | ||
const RUNTIME_STATE_TO_KERNEL_STATUS: Partial<Record<RuntimeState, KernelStatus>> = { | ||
// Runtime is starting up | ||
[RuntimeState.Uninitialized]: KernelStatus.Connecting, | ||
[RuntimeState.Initializing]: KernelStatus.Connecting, | ||
[RuntimeState.Starting]: KernelStatus.Connecting, | ||
[RuntimeState.Restarting]: KernelStatus.Connecting, | ||
|
||
// Runtime is operational | ||
[RuntimeState.Ready]: KernelStatus.Connected, | ||
[RuntimeState.Idle]: KernelStatus.Connected, | ||
[RuntimeState.Busy]: KernelStatus.Connected, | ||
[RuntimeState.Interrupting]: KernelStatus.Connected, | ||
|
||
// Runtime is shutting down or ended | ||
[RuntimeState.Exiting]: KernelStatus.Disconnected, | ||
[RuntimeState.Exited]: KernelStatus.Disconnected, | ||
[RuntimeState.Offline]: KernelStatus.Disconnected, | ||
} as const; | ||
|
||
/** | ||
* Implementation of IPositronNotebookInstance that handles the core notebook functionality | ||
* and state management. This class serves as the bridge between the UI and the underlying | ||
|
@@ -214,11 +245,6 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot | |
selectionStateMachine; | ||
contextManager; | ||
|
||
/** | ||
* Selected kernel for the notebook. | ||
*/ | ||
kernel; | ||
|
||
/** | ||
* Status of kernel for the notebook. | ||
*/ | ||
|
@@ -337,29 +363,29 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot | |
d.dispose(); | ||
this.runtimeSession.set(undefined, undefined); | ||
})); | ||
// Also update the observable when runtime state changes | ||
// This ensures derived observables like kernelStatus stay in sync | ||
this._register(session.onDidChangeRuntimeState(() => { | ||
this.runtimeSession.set(session, undefined); | ||
nstrayer marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
})); | ||
} | ||
})); | ||
|
||
// Track the current selected kernel for this notebook | ||
this.kernel = observableValue<INotebookKernel | undefined>('positronNotebookKernel', undefined); | ||
this._register(this.notebookKernelService.onDidChangeSelectedNotebooks(e => { | ||
if (e && this._isThisNotebook(e.notebook) && this.textModel) { | ||
const matching = this.notebookKernelService.getMatchingKernel(this.textModel); | ||
const kernel = matching.all.find(k => k.id === e.newKernel); | ||
this.kernel.set(kernel, undefined); | ||
// Derive the kernel connection status from the runtime session state | ||
// Uses the RUNTIME_STATE_TO_KERNEL_STATUS map for the conversion | ||
this.kernelStatus = this.runtimeSession.map(session => { | ||
if (!session) { | ||
return KernelStatus.Uninitialized; | ||
} | ||
})); | ||
|
||
// Derive the kernel connection status | ||
this.kernelStatus = this.kernel.map( | ||
this, | ||
kernel => /** @description positronNotebookKernelStatus */ kernel ? KernelStatus.Connected : KernelStatus.Disconnected | ||
); | ||
const runtimeState = session.getRuntimeState(); | ||
// Return the mapped status, or Errored if the runtime state is unknown | ||
return RUNTIME_STATE_TO_KERNEL_STATUS[runtimeState] ?? KernelStatus.Errored; | ||
}); | ||
|
||
|
||
// Derive the notebook language from the selected kernel | ||
this._language = this.kernel.map( | ||
this, | ||
kernel => /** @description positronNotebookLanguage */ kernel?.supportedLanguages[0] ?? 'plaintext' | ||
// Derive the notebook language from the runtime session | ||
this._language = this.runtimeSession.map( | ||
session => /** @description positronNotebookLanguage */ session?.runtimeMetadata.languageId ?? 'plaintext' | ||
); | ||
|
||
this.contextManager = this._register( | ||
|
@@ -800,13 +826,15 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot | |
* fully determine the view we see. | ||
*/ | ||
getEditorViewState(): INotebookEditorViewState { | ||
this._assertTextModel(); | ||
const selectedKernel = this.notebookKernelService.getSelectedOrSuggestedKernel(this.textModel); | ||
return { | ||
editingCells: {}, | ||
cellLineNumberStates: {}, | ||
editorViewStates: {}, | ||
collapsedInputCells: {}, | ||
collapsedOutputCells: {}, | ||
selectedKernelId: this.kernel.get()?.id, | ||
selectedKernelId: selectedKernel?.id, | ||
}; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This subscription is registered on the instance instead of the per-session DisposableStore, so it won't be disposed when the session changes. This can lead to stale callbacks updating runtimeSession from an old session and a potential leak. Add this listener to the same per-session DisposableStore (d) used above.
Copilot uses AI. Check for mistakes.