Skip to content

Commit 52d8926

Browse files
tkislantkislan
committed
refactor: Update deepnote toolkit installation to return toolkit version and improve logging
1 parent e20afc1 commit 52d8926

7 files changed

+80
-69
lines changed

‎src/kernels/deepnote/deepnoteServerStarter.node.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -166,10 +166,11 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension
166166

167167
// Ensure toolkit is installed in venv and get venv's Python interpreter
168168
logger.info(`Ensuring deepnote-toolkit is installed in venv for environment ${environmentId}...`);
169-
const venvInterpreter = await this.toolkitInstaller.ensureVenvAndToolkit(interpreter, venvPath, token);
170-
if (!venvInterpreter) {
171-
throw new Error('Failed to install deepnote-toolkit. Please check the output for details.');
172-
}
169+
const { pythonInterpreter: venvInterpreter } = await this.toolkitInstaller.ensureVenvAndToolkit(
170+
interpreter,
171+
venvPath,
172+
token
173+
);
173174

174175
Cancellation.throwIfCanceled(token);
175176

‎src/kernels/deepnote/deepnoteToolkitInstaller.node.ts‎

Lines changed: 37 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,20 @@
33

44
import { inject, injectable, named } from 'inversify';
55
import { CancellationToken, Uri, workspace } from 'vscode';
6-
import { PythonEnvironment } from '../../platform/pythonEnvironments/info';
7-
import { IDeepnoteToolkitInstaller, DEEPNOTE_TOOLKIT_WHEEL_URL, DEEPNOTE_TOOLKIT_VERSION } from './types';
8-
import { IProcessServiceFactory } from '../../platform/common/process/types.node';
9-
import { logger } from '../../platform/logging';
10-
import { IOutputChannel, IExtensionContext } from '../../platform/common/types';
6+
import { Cancellation } from '../../platform/common/cancellation';
117
import { STANDARD_OUTPUT_CHANNEL } from '../../platform/common/constants';
128
import { IFileSystem } from '../../platform/common/platform/types';
13-
import { Cancellation } from '../../platform/common/cancellation';
14-
import { DeepnoteVenvCreationError, DeepnoteToolkitInstallError } from '../../platform/errors/deepnoteKernelErrors';
9+
import { IProcessServiceFactory } from '../../platform/common/process/types.node';
10+
import { IExtensionContext, IOutputChannel } from '../../platform/common/types';
11+
import { DeepnoteToolkitInstallError, DeepnoteVenvCreationError } from '../../platform/errors/deepnoteKernelErrors';
12+
import { logger } from '../../platform/logging';
13+
import { PythonEnvironment } from '../../platform/pythonEnvironments/info';
14+
import {
15+
DEEPNOTE_TOOLKIT_VERSION,
16+
DEEPNOTE_TOOLKIT_WHEEL_URL,
17+
IDeepnoteToolkitInstaller,
18+
VenvAndToolkitInstallation
19+
} from './types';
1520

1621
/**
1722
* Handles installation of the deepnote-toolkit Python package.
@@ -20,7 +25,7 @@ import { DeepnoteVenvCreationError, DeepnoteToolkitInstallError } from '../../pl
2025
export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
2126
private readonly venvPythonPaths: Map<string, Uri> = new Map();
2227
// Track in-flight installations per venv path to prevent concurrent installs
23-
private readonly pendingInstallations: Map<string, Promise<PythonEnvironment>> = new Map();
28+
private readonly pendingInstallations: Map<string, Promise<VenvAndToolkitInstallation>> = new Map();
2429

2530
constructor(
2631
@inject(IProcessServiceFactory) private readonly processServiceFactory: IProcessServiceFactory,
@@ -77,7 +82,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
7782
baseInterpreter: PythonEnvironment,
7883
venvPath: Uri,
7984
token?: CancellationToken
80-
): Promise<PythonEnvironment> {
85+
): Promise<VenvAndToolkitInstallation> {
8186
const venvKey = venvPath.fsPath;
8287

8388
logger.info(`Ensuring virtual environment at ${venvKey}`);
@@ -96,19 +101,22 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
96101

97102
// Check if venv already exists with toolkit installed
98103
const existingVenv = await this.getVenvInterpreterByPath(venvPath);
99-
if (existingVenv && (await this.isToolkitInstalled(existingVenv))) {
100-
logger.info(`deepnote-toolkit venv already exists at ${venvPath.fsPath}`);
104+
if (existingVenv) {
105+
const toolkitVersion = await this.isToolkitInstalled(existingVenv);
106+
if (toolkitVersion != null) {
107+
logger.info(`deepnote-toolkit venv already exists at ${venvPath.fsPath}`);
101108

102-
// Ensure kernel spec is installed (may have been deleted or never installed)
103-
try {
104-
await this.installKernelSpec(existingVenv, venvPath);
105-
} catch (ex) {
106-
logger.warn(`Failed to ensure kernel spec installed: ${ex}`);
107-
// Don't fail - continue with existing venv
108-
}
109+
// Ensure kernel spec is installed (may have been deleted or never installed)
110+
try {
111+
await this.installKernelSpec(existingVenv, venvPath);
112+
} catch (ex) {
113+
logger.warn(`Failed to ensure kernel spec installed: ${ex}`);
114+
// Don't fail - continue with existing venv
115+
}
109116

110-
logger.info(`Venv ready at ${venvPath.fsPath}`);
111-
return existingVenv;
117+
logger.info(`Venv ready at ${venvPath.fsPath}`);
118+
return { pythonInterpreter: existingVenv, toolkitVersion };
119+
}
112120
}
113121

114122
// Double-check for race condition
@@ -193,7 +201,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
193201
baseInterpreter: PythonEnvironment,
194202
venvPath: Uri,
195203
token?: CancellationToken
196-
): Promise<PythonEnvironment> {
204+
): Promise<VenvAndToolkitInstallation> {
197205
try {
198206
Cancellation.throwIfCanceled(token);
199207

@@ -288,7 +296,8 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
288296
}
289297

290298
// Verify installation
291-
if (await this.isToolkitInstalled(venvInterpreter)) {
299+
const installedToolkitVersion = await this.isToolkitInstalled(venvInterpreter);
300+
if (installedToolkitVersion != null) {
292301
logger.info('deepnote-toolkit installed successfully in venv');
293302

294303
// Install kernel spec so the kernel uses this venv's Python
@@ -300,7 +309,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
300309
}
301310

302311
this.outputChannel.appendLine('✓ Deepnote toolkit ready');
303-
return venvInterpreter;
312+
return { pythonInterpreter: venvInterpreter, toolkitVersion: installedToolkitVersion };
304313
} else {
305314
logger.error('deepnote-toolkit installation failed');
306315
this.outputChannel.appendLine('✗ deepnote-toolkit installation failed');
@@ -334,18 +343,19 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
334343
}
335344
}
336345

337-
private async isToolkitInstalled(interpreter: PythonEnvironment): Promise<boolean> {
346+
private async isToolkitInstalled(interpreter: PythonEnvironment): Promise<string | undefined> {
338347
try {
339348
// Use undefined as resource to get full system environment
340349
const processService = await this.processServiceFactory.create(undefined);
341350
const result = await processService.exec(interpreter.uri.fsPath, [
342351
'-c',
343-
"import deepnote_toolkit; print('installed')"
352+
'import deepnote_toolkit; print(deepnote_toolkit.__version__)'
344353
]);
345-
return result.stdout.toLowerCase().includes('installed');
354+
logger.info(`isToolkitInstalled result: ${result.stdout}`);
355+
return result.stdout.trim();
346356
} catch (ex) {
347357
logger.debug(`deepnote-toolkit not found: ${ex}`);
348-
return false;
358+
return undefined;
349359
}
350360
}
351361

‎src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,11 @@ export class DeepnoteEnvironmentManager implements IExtensionSyncActivationServi
195195
logger.info(`Ensuring server is running for environment: ${config.name} (${id})`);
196196

197197
// First ensure venv is created and toolkit is installed
198-
await this.toolkitInstaller.ensureVenvAndToolkit(config.pythonInterpreter, config.venvPath, undefined);
198+
const { pythonInterpreter, toolkitVersion } = await this.toolkitInstaller.ensureVenvAndToolkit(
199+
config.pythonInterpreter,
200+
config.venvPath,
201+
undefined
202+
);
199203

200204
// Install additional packages if specified
201205
if (config.packages && config.packages.length > 0) {
@@ -205,13 +209,10 @@ export class DeepnoteEnvironmentManager implements IExtensionSyncActivationServi
205209
// Start the Jupyter server (serverStarter is idempotent - returns existing if running)
206210
// IMPORTANT: Always call this to ensure we get the current server info
207211
// Don't return early based on config.serverInfo - it may be stale!
208-
const serverInfo = await this.serverStarter.startServer(
209-
config.pythonInterpreter,
210-
config.venvPath,
211-
id,
212-
undefined
213-
);
212+
const serverInfo = await this.serverStarter.startServer(pythonInterpreter, config.venvPath, id, undefined);
214213

214+
config.pythonInterpreter = pythonInterpreter;
215+
config.toolkitVersion = toolkitVersion;
215216
config.serverInfo = serverInfo;
216217
config.lastUsedAt = new Date();
217218

‎src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,13 @@ import { Uri } from 'vscode';
44
import { DeepnoteEnvironmentManager } from './deepnoteEnvironmentManager.node';
55
import { DeepnoteEnvironmentStorage } from './deepnoteEnvironmentStorage.node';
66
import { IExtensionContext } from '../../../platform/common/types';
7-
import { IDeepnoteServerStarter, IDeepnoteToolkitInstaller, DeepnoteServerInfo } from '../types';
7+
import {
8+
IDeepnoteServerStarter,
9+
IDeepnoteToolkitInstaller,
10+
DeepnoteServerInfo,
11+
VenvAndToolkitInstallation,
12+
DEEPNOTE_TOOLKIT_VERSION
13+
} from '../types';
814
import { PythonEnvironment } from '../../../platform/pythonEnvironments/info';
915
import { EnvironmentStatus } from './deepnoteEnvironment';
1016

@@ -28,6 +34,11 @@ suite('DeepnoteEnvironmentManager', () => {
2834
token: 'test-token'
2935
};
3036

37+
const testVenvAndToolkit: VenvAndToolkitInstallation = {
38+
pythonInterpreter: testInterpreter,
39+
toolkitVersion: DEEPNOTE_TOOLKIT_VERSION
40+
};
41+
3142
setup(() => {
3243
mockContext = mock<IExtensionContext>();
3344
mockStorage = mock<DeepnoteEnvironmentStorage>();
@@ -179,7 +190,7 @@ suite('DeepnoteEnvironmentManager', () => {
179190
test('should return environment with running status when server is running', async () => {
180191
when(mockStorage.saveEnvironments(anything())).thenResolve();
181192
when(mockToolkitInstaller.ensureVenvAndToolkit(anything(), anything(), anything())).thenResolve(
182-
testInterpreter
193+
testVenvAndToolkit
183194
);
184195
when(mockServerStarter.startServer(anything(), anything(), anything(), anything())).thenResolve(
185196
testServerInfo
@@ -271,7 +282,7 @@ suite('DeepnoteEnvironmentManager', () => {
271282
test('should stop server before deleting if running', async () => {
272283
when(mockStorage.saveEnvironments(anything())).thenResolve();
273284
when(mockToolkitInstaller.ensureVenvAndToolkit(anything(), anything(), anything())).thenResolve(
274-
testInterpreter
285+
testVenvAndToolkit
275286
);
276287
when(mockServerStarter.startServer(anything(), anything(), anything(), anything())).thenResolve(
277288
testServerInfo
@@ -298,7 +309,7 @@ suite('DeepnoteEnvironmentManager', () => {
298309
test('should start server for environment', async () => {
299310
when(mockStorage.saveEnvironments(anything())).thenResolve();
300311
when(mockToolkitInstaller.ensureVenvAndToolkit(anything(), anything(), anything())).thenResolve(
301-
testInterpreter
312+
testVenvAndToolkit
302313
);
303314
when(mockServerStarter.startServer(anything(), anything(), anything(), anything())).thenResolve(
304315
testServerInfo
@@ -321,7 +332,7 @@ suite('DeepnoteEnvironmentManager', () => {
321332
test('should install additional packages when specified', async () => {
322333
when(mockStorage.saveEnvironments(anything())).thenResolve();
323334
when(mockToolkitInstaller.ensureVenvAndToolkit(anything(), anything(), anything())).thenResolve(
324-
testInterpreter
335+
testVenvAndToolkit
325336
);
326337
when(mockToolkitInstaller.installAdditionalPackages(anything(), anything(), anything())).thenResolve();
327338
when(mockServerStarter.startServer(anything(), anything(), anything(), anything())).thenResolve(
@@ -344,7 +355,7 @@ suite('DeepnoteEnvironmentManager', () => {
344355
test('should always call serverStarter.startServer to ensure fresh serverInfo (UT-6)', async () => {
345356
when(mockStorage.saveEnvironments(anything())).thenResolve();
346357
when(mockToolkitInstaller.ensureVenvAndToolkit(anything(), anything(), anything())).thenResolve(
347-
testInterpreter
358+
testVenvAndToolkit
348359
);
349360
when(mockServerStarter.startServer(anything(), anything(), anything(), anything())).thenResolve(
350361
testServerInfo
@@ -370,7 +381,7 @@ suite('DeepnoteEnvironmentManager', () => {
370381

371382
when(mockStorage.saveEnvironments(anything())).thenResolve();
372383
when(mockToolkitInstaller.ensureVenvAndToolkit(anything(), anything(), anything())).thenResolve(
373-
testInterpreter
384+
testVenvAndToolkit
374385
);
375386

376387
// First call returns initial serverInfo
@@ -415,7 +426,7 @@ suite('DeepnoteEnvironmentManager', () => {
415426
test('should update lastUsedAt timestamp', async () => {
416427
when(mockStorage.saveEnvironments(anything())).thenResolve();
417428
when(mockToolkitInstaller.ensureVenvAndToolkit(anything(), anything(), anything())).thenResolve(
418-
testInterpreter
429+
testVenvAndToolkit
419430
);
420431
when(mockServerStarter.startServer(anything(), anything(), anything(), anything())).thenResolve(
421432
testServerInfo
@@ -443,7 +454,7 @@ suite('DeepnoteEnvironmentManager', () => {
443454
test('should stop running server', async () => {
444455
when(mockStorage.saveEnvironments(anything())).thenResolve();
445456
when(mockToolkitInstaller.ensureVenvAndToolkit(anything(), anything(), anything())).thenResolve(
446-
testInterpreter
457+
testVenvAndToolkit
447458
);
448459
when(mockServerStarter.startServer(anything(), anything(), anything(), anything())).thenResolve(
449460
testServerInfo
@@ -486,7 +497,7 @@ suite('DeepnoteEnvironmentManager', () => {
486497
test('should stop and start server', async () => {
487498
when(mockStorage.saveEnvironments(anything())).thenResolve();
488499
when(mockToolkitInstaller.ensureVenvAndToolkit(anything(), anything(), anything())).thenResolve(
489-
testInterpreter
500+
testVenvAndToolkit
490501
);
491502
when(mockServerStarter.startServer(anything(), anything(), anything(), anything())).thenResolve(
492503
testServerInfo

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

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -96,15 +96,13 @@ export class DeepnoteEnvironmentTreeDataProvider implements TreeDataProvider<Dee
9696
// Python interpreter
9797
items.push(
9898
DeepnoteEnvironmentTreeItem.createInfoItem(
99-
`Python: ${this.getShortPath(config.pythonInterpreter.uri.fsPath)}`,
99+
`Python: ${config.pythonInterpreter.uri.fsPath}`,
100100
'symbol-namespace'
101101
)
102102
);
103103

104104
// Venv path
105-
items.push(
106-
DeepnoteEnvironmentTreeItem.createInfoItem(`Venv: ${this.getShortPath(config.venvPath.fsPath)}`, 'folder')
107-
);
105+
items.push(DeepnoteEnvironmentTreeItem.createInfoItem(`Venv: ${config.venvPath.fsPath}`, 'folder'));
108106

109107
// Packages
110108
if (config.packages && config.packages.length > 0) {
@@ -132,19 +130,6 @@ export class DeepnoteEnvironmentTreeDataProvider implements TreeDataProvider<Dee
132130
return items;
133131
}
134132

135-
/**
136-
* Shorten a file path for display (show last 2-3 segments)
137-
*/
138-
private getShortPath(fullPath: string): string {
139-
const parts = fullPath.split(/[/\\]/);
140-
if (parts.length <= 3) {
141-
return fullPath;
142-
}
143-
144-
// Show last 3 segments with ellipsis
145-
return `.../${parts.slice(-3).join('/')}`;
146-
}
147-
148133
public dispose(): void {
149134
this._onDidChangeTreeData.dispose();
150135
this.disposables.forEach((d) => d.dispose());

‎src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,6 @@ export class DeepnoteEnvironmentTreeItem extends TreeItem {
8383
lines.push(`Status: ${this.status}`);
8484
lines.push(`Python: ${this.environment.pythonInterpreter.uri.fsPath}`);
8585
lines.push(`Venv: ${this.environment.venvPath.fsPath}`);
86-
// lines.push(`Python: ${this.environment.pythonInterpreter.uriFsPath}`);
87-
// lines.push(`Venv: ${this.environment.venvPathFsPath}`);
8886

8987
if (this.environment.packages && this.environment.packages.length > 0) {
9088
lines.push(`Packages: ${this.environment.packages.join(', ')}`);

‎src/kernels/deepnote/types.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,11 @@ import {
1313
DeepnoteEnvironmentWithStatus
1414
} from './environments/deepnoteEnvironment';
1515

16+
export interface VenvAndToolkitInstallation {
17+
pythonInterpreter: PythonEnvironment;
18+
toolkitVersion: string;
19+
}
20+
1621
/**
1722
* Connection metadata for Deepnote Toolkit Kernels.
1823
* This kernel connects to a Jupyter server started by deepnote-toolkit.
@@ -82,15 +87,15 @@ export interface IDeepnoteToolkitInstaller {
8287
* @param baseInterpreter The base Python interpreter to use for creating the venv
8388
* @param venvPath The path where the venv should be created
8489
* @param token Cancellation token to cancel the operation
85-
* @returns The Python interpreter from the venv
90+
* @returns The Python interpreter from the venv and the toolkit version
8691
* @throws {DeepnoteVenvCreationError} If venv creation fails
8792
* @throws {DeepnoteToolkitInstallError} If toolkit installation fails
8893
*/
8994
ensureVenvAndToolkit(
9095
baseInterpreter: PythonEnvironment,
9196
venvPath: vscode.Uri,
9297
token?: vscode.CancellationToken
93-
): Promise<PythonEnvironment>;
98+
): Promise<VenvAndToolkitInstallation>;
9499

95100
/**
96101
* Install additional packages in the venv.

0 commit comments

Comments
 (0)