Skip to content

Commit f617a4e

Browse files
committed
feat: Add clearControllerForEnvironment method and disposeKernelsUsingEnvironment logic
- Introduced clearControllerForEnvironment method in IDeepnoteKernelAutoSelector to unselect the controller for a notebook when an environment is deleted. - Implemented disposeKernelsUsingEnvironment method in DeepnoteEnvironmentsView to dispose of kernels from open notebooks using the deleted environment. - Enhanced unit tests for DeepnoteEnvironmentsView to verify kernel disposal behavior when an environment is deleted. Signed-off-by: Tomas Kislan <tomas@kislan.sk>
1 parent 4582eee commit f617a4e

File tree

5 files changed

+196
-5
lines changed

5 files changed

+196
-5
lines changed

src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,15 @@ import { IDeepnoteEnvironmentManager } from '../types';
33
import { EnvironmentTreeItemType, DeepnoteEnvironmentTreeItem } from './deepnoteEnvironmentTreeItem.node';
44
import { EnvironmentStatus } from './deepnoteEnvironment';
55
import { inject, injectable } from 'inversify';
6+
import { IExtensionSyncActivationService } from '../../../platform/activation/types';
67

78
/**
89
* Tree data provider for the Deepnote kernel environments view
910
*/
1011
@injectable()
11-
export class DeepnoteEnvironmentTreeDataProvider implements TreeDataProvider<DeepnoteEnvironmentTreeItem>, Disposable {
12+
export class DeepnoteEnvironmentTreeDataProvider
13+
implements TreeDataProvider<DeepnoteEnvironmentTreeItem>, IExtensionSyncActivationService, Disposable
14+
{
1215
private readonly _onDidChangeTreeData = new EventEmitter<DeepnoteEnvironmentTreeItem | undefined | void>();
1316
private readonly disposables: Disposable[] = [];
1417

@@ -21,6 +24,10 @@ export class DeepnoteEnvironmentTreeDataProvider implements TreeDataProvider<Dee
2124
);
2225
}
2326

27+
public activate(): void {
28+
this.refresh();
29+
}
30+
2431
public get onDidChangeTreeData(): Event<DeepnoteEnvironmentTreeItem | undefined | void> {
2532
return this._onDidChangeTreeData.event;
2633
}

src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts

Lines changed: 57 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,14 @@
11
import { inject, injectable } from 'inversify';
2-
import { commands, Disposable, l10n, ProgressLocation, QuickPickItem, TreeView, window } from 'vscode';
2+
import { commands, Disposable, l10n, ProgressLocation, QuickPickItem, TreeView, window, workspace } from 'vscode';
33
import { IDisposableRegistry } from '../../../platform/common/types';
44
import { logger } from '../../../platform/logging';
55
import { IPythonApiProvider } from '../../../platform/api/types';
6-
import { IDeepnoteEnvironmentManager, IDeepnoteKernelAutoSelector, IDeepnoteNotebookEnvironmentMapper } from '../types';
6+
import {
7+
DeepnoteKernelConnectionMetadata,
8+
IDeepnoteEnvironmentManager,
9+
IDeepnoteKernelAutoSelector,
10+
IDeepnoteNotebookEnvironmentMapper
11+
} from '../types';
712
import { DeepnoteEnvironmentTreeDataProvider } from './deepnoteEnvironmentTreeDataProvider.node';
813
import { DeepnoteEnvironmentTreeItem } from './deepnoteEnvironmentTreeItem.node';
914
import { CreateDeepnoteEnvironmentOptions, EnvironmentStatus } from './deepnoteEnvironment';
@@ -303,6 +308,9 @@ export class DeepnoteEnvironmentsView implements Disposable {
303308
await this.notebookEnvironmentMapper.removeEnvironmentForNotebook(nb);
304309
}
305310

311+
// Dispose kernels from any open notebooks using this environment
312+
await this.disposeKernelsUsingEnvironment(environmentId);
313+
306314
await this.environmentManager.deleteEnvironment(environmentId, token);
307315
logger.info(`Deleted environment: ${environmentId}`);
308316
}
@@ -315,6 +323,52 @@ export class DeepnoteEnvironmentsView implements Disposable {
315323
}
316324
}
317325

326+
/**
327+
* Dispose kernels from any open notebooks that are using the specified environment.
328+
* This ensures the UI reflects that the kernel is no longer available.
329+
*/
330+
private async disposeKernelsUsingEnvironment(environmentId: string): Promise<void> {
331+
const openNotebooks = workspace.notebookDocuments;
332+
333+
for (const notebook of openNotebooks) {
334+
// Only check Deepnote notebooks
335+
if (notebook.notebookType !== 'deepnote') {
336+
continue;
337+
}
338+
339+
// Get the kernel for this notebook
340+
const kernel = this.kernelProvider.get(notebook);
341+
if (!kernel) {
342+
continue;
343+
}
344+
345+
// Check if this kernel is using the environment being deleted
346+
const connectionMetadata = kernel.kernelConnectionMetadata;
347+
if (connectionMetadata.kind === 'startUsingDeepnoteKernel') {
348+
const deepnoteMetadata = connectionMetadata as DeepnoteKernelConnectionMetadata;
349+
const expectedHandle = `deepnote-config-server-${environmentId}`;
350+
351+
if (deepnoteMetadata.serverProviderHandle.handle === expectedHandle) {
352+
logger.info(
353+
`Disposing kernel for notebook ${getDisplayPath(
354+
notebook.uri
355+
)} as it uses deleted environment ${environmentId}`
356+
);
357+
358+
try {
359+
// First, unselect the controller from the notebook UI
360+
this.kernelAutoSelector.clearControllerForEnvironment(notebook, environmentId);
361+
362+
// Then dispose the kernel
363+
await kernel.dispose();
364+
} catch (error) {
365+
logger.error(`Failed to dispose kernel for ${getDisplayPath(notebook.uri)}`, error);
366+
}
367+
}
368+
}
369+
}
370+
}
371+
318372
public async selectEnvironmentForNotebook(): Promise<void> {
319373
// Get the active notebook
320374
const activeNotebook = window.activeNotebookEditor?.notebook;
@@ -359,7 +413,7 @@ export class DeepnoteEnvironmentsView implements Disposable {
359413
const isCurrent = currentEnvironment?.id === env.id;
360414

361415
return {
362-
label: `${icon} ${env.name} [${text}]${isCurrent ? ' $(check)' : ''}`,
416+
label: `$(${icon}) ${env.name} [${text}]${isCurrent ? ' $(check)' : ''}`,
363417
description: getDisplayPath(env.pythonInterpreter.uri),
364418
detail: env.packages?.length
365419
? l10n.t('Packages: {0}', env.packages.join(', '))

src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,107 @@ suite('DeepnoteEnvironmentsView', () => {
456456
// Verify success message was shown
457457
verify(mockedVSCodeNamespaces.window.showInformationMessage(anything())).once();
458458
});
459+
460+
test('should dispose kernels from open notebooks using the deleted environment', async () => {
461+
// Mock environment exists
462+
when(mockConfigManager.getEnvironment(testEnvironmentId)).thenReturn(testEnvironment);
463+
464+
// Mock user confirmation
465+
when(mockedVSCodeNamespaces.window.showWarningMessage(anything(), anything(), anything())).thenReturn(
466+
Promise.resolve('Delete')
467+
);
468+
469+
// Mock notebooks using this environment
470+
when(mockNotebookEnvironmentMapper.getNotebooksUsingEnvironment(testEnvironmentId)).thenReturn([]);
471+
when(mockNotebookEnvironmentMapper.removeEnvironmentForNotebook(anything())).thenResolve();
472+
473+
// Mock open notebooks with kernels
474+
const openNotebook1 = {
475+
uri: Uri.file('/workspace/open-notebook1.deepnote'),
476+
notebookType: 'deepnote',
477+
isClosed: false
478+
} as any;
479+
480+
const openNotebook2 = {
481+
uri: Uri.file('/workspace/open-notebook2.deepnote'),
482+
notebookType: 'jupyter-notebook',
483+
isClosed: false
484+
} as any;
485+
486+
const openNotebook3 = {
487+
uri: Uri.file('/workspace/open-notebook3.deepnote'),
488+
notebookType: 'deepnote',
489+
isClosed: false
490+
} as any;
491+
492+
// Mock workspace.notebookDocuments
493+
when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([
494+
openNotebook1,
495+
openNotebook2,
496+
openNotebook3
497+
]);
498+
499+
// Mock kernels
500+
const mockKernel1 = {
501+
kernelConnectionMetadata: {
502+
kind: 'startUsingDeepnoteKernel',
503+
serverProviderHandle: {
504+
handle: `deepnote-config-server-${testEnvironmentId}`
505+
}
506+
},
507+
dispose: sinon.stub().resolves()
508+
};
509+
510+
const mockKernel3 = {
511+
kernelConnectionMetadata: {
512+
kind: 'startUsingDeepnoteKernel',
513+
serverProviderHandle: {
514+
handle: 'deepnote-config-server-different-env'
515+
}
516+
},
517+
dispose: sinon.stub().resolves()
518+
};
519+
520+
// Mock kernelProvider.get()
521+
when(mockKernelProvider.get(openNotebook1)).thenReturn(mockKernel1 as any);
522+
when(mockKernelProvider.get(openNotebook2)).thenReturn(undefined); // No kernel for jupyter notebook
523+
when(mockKernelProvider.get(openNotebook3)).thenReturn(mockKernel3 as any);
524+
525+
// Mock window.withProgress
526+
when(mockedVSCodeNamespaces.window.withProgress(anything(), anything())).thenCall(
527+
(_options: ProgressOptions, callback: Function) => {
528+
const mockProgress = {
529+
report: () => {
530+
// Mock progress reporting
531+
}
532+
};
533+
const mockToken: CancellationToken = {
534+
isCancellationRequested: false,
535+
onCancellationRequested: () => ({
536+
dispose: () => {
537+
// Mock disposable
538+
}
539+
})
540+
};
541+
return callback(mockProgress, mockToken);
542+
}
543+
);
544+
545+
// Mock environment deletion
546+
when(mockConfigManager.deleteEnvironment(testEnvironmentId, anything())).thenResolve();
547+
when(mockedVSCodeNamespaces.window.showInformationMessage(anything())).thenResolve(undefined);
548+
549+
// Execute the command
550+
await view.deleteEnvironmentCommand(testEnvironmentId);
551+
552+
// Verify that only kernel1 (using the deleted environment) was disposed
553+
assert.strictEqual(mockKernel1.dispose.callCount, 1, 'Kernel using deleted environment should be disposed');
554+
assert.strictEqual(
555+
mockKernel3.dispose.callCount,
556+
0,
557+
'Kernel using different environment should not be disposed'
558+
);
559+
});
459560
});
460561

461562
suite('selectEnvironmentForNotebook', () => {

src/kernels/deepnote/types.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,14 @@ export interface IDeepnoteKernelAutoSelector {
194194
* @param token Cancellation token to cancel the operation
195195
*/
196196
rebuildController(notebook: vscode.NotebookDocument, token?: vscode.CancellationToken): Promise<void>;
197+
198+
/**
199+
* Clear the controller selection for a notebook using a specific environment.
200+
* This is used when deleting an environment to unselect its controller from any open notebooks.
201+
* @param notebook The notebook document
202+
* @param environmentId The environment ID
203+
*/
204+
clearControllerForEnvironment(notebook: vscode.NotebookDocument, environmentId: string): void;
197205
}
198206

199207
export const IDeepnoteEnvironmentManager = Symbol('IDeepnoteEnvironmentManager');

src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,6 @@ export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector,
419419

420420
private async ensureKernelSelectedWithConfiguration(
421421
notebook: NotebookDocument,
422-
// configuration: import('./../../kernels/deepnote/environments/deepnoteEnvironment').DeepnoteEnvironment,
423422
configuration: DeepnoteEnvironment,
424423
baseFileUri: Uri,
425424
notebookKey: string,
@@ -653,6 +652,28 @@ export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector,
653652
logger.info(`Created loading controller for ${notebookKey}`);
654653
}
655654

655+
/**
656+
* Clear the controller selection for a notebook using a specific environment.
657+
* This is used when deleting an environment to unselect its controller from any open notebooks.
658+
*/
659+
public clearControllerForEnvironment(notebook: NotebookDocument, environmentId: string): void {
660+
const selectedController = this.controllerRegistration.getSelected(notebook);
661+
if (!selectedController || selectedController.connection.kind !== 'startUsingDeepnoteKernel') {
662+
return;
663+
}
664+
665+
const deepnoteMetadata = selectedController.connection as DeepnoteKernelConnectionMetadata;
666+
const expectedHandle = `deepnote-config-server-${environmentId}`;
667+
668+
if (deepnoteMetadata.serverProviderHandle.handle === expectedHandle) {
669+
// Unselect the controller by setting affinity to Default
670+
selectedController.controller.updateNotebookAffinity(notebook, NotebookControllerAffinity.Default);
671+
logger.info(
672+
`Cleared controller for notebook ${getDisplayPath(notebook.uri)} (environment ${environmentId})`
673+
);
674+
}
675+
}
676+
656677
/**
657678
* Handle kernel selection errors with user-friendly messages and actions
658679
*/

0 commit comments

Comments
 (0)