Skip to content

Commit f9376b5

Browse files
saltenaslsaltenasl
committed
chore: code review comments
1 parent 263f04e commit f9376b5

File tree

2 files changed

+257
-1
lines changed

2 files changed

+257
-1
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter {
153153

154154
const serverReady = await this.waitForServer(serverInfo, 30000, token);
155155
if (!serverReady) {
156-
await this.stopServer(deepnoteFileUri);
156+
await this.stopServerImpl(deepnoteFileUri);
157157
throw new Error('Deepnote server failed to start within timeout period');
158158
}
159159

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

Lines changed: 256 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,256 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
4+
import { inject, injectable, named } from 'inversify';
5+
import { CancellationToken, Uri } from 'vscode';
6+
import { PythonEnvironment } from '../../platform/pythonEnvironments/info';
7+
import { IProcessServiceFactory } from '../../platform/common/process/types.node';
8+
import { logger } from '../../platform/logging';
9+
import { IOutputChannel, IExtensionContext } from '../../platform/common/types';
10+
import { STANDARD_OUTPUT_CHANNEL } from '../../platform/common/constants';
11+
import { IFileSystem } from '../../platform/common/platform/types';
12+
import { Cancellation } from '../../platform/common/cancellation';
13+
import { DEEPNOTE_TOOLKIT_WHEEL_URL, DEEPNOTE_TOOLKIT_VERSION } from './types';
14+
15+
/**
16+
* Manages a shared installation of deepnote-toolkit in a versioned extension directory.
17+
* This avoids installing the heavy wheel package in every virtual environment.
18+
*/
19+
@injectable()
20+
export class DeepnoteSharedToolkitInstaller {
21+
private readonly sharedInstallationPath: Uri;
22+
private readonly versionFilePath: Uri;
23+
private readonly toolkitVersion: string;
24+
private installationPromise: Promise<boolean> | undefined;
25+
26+
constructor(
27+
@inject(IProcessServiceFactory) private readonly processServiceFactory: IProcessServiceFactory,
28+
@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel,
29+
@inject(IExtensionContext) private readonly context: IExtensionContext,
30+
@inject(IFileSystem) private readonly fs: IFileSystem
31+
) {
32+
// Create versioned directory for shared toolkit installation
33+
this.toolkitVersion = DEEPNOTE_TOOLKIT_VERSION;
34+
this.sharedInstallationPath = Uri.joinPath(
35+
this.context.globalStorageUri,
36+
'deepnote-shared-toolkit',
37+
this.toolkitVersion
38+
);
39+
this.versionFilePath = Uri.joinPath(this.sharedInstallationPath, 'version.txt');
40+
}
41+
42+
/**
43+
* Ensures the shared deepnote-toolkit installation is available.
44+
* @param baseInterpreter The base Python interpreter to use for installation
45+
* @param token Cancellation token
46+
* @returns True if installation is ready, false if failed
47+
*/
48+
public async ensureSharedInstallation(
49+
baseInterpreter: PythonEnvironment,
50+
token?: CancellationToken
51+
): Promise<boolean> {
52+
// Check if already installed and up to date
53+
if (await this.isInstalled()) {
54+
logger.info(`Shared deepnote-toolkit v${this.toolkitVersion} is already installed`);
55+
return true;
56+
}
57+
58+
// Prevent concurrent installations
59+
if (this.installationPromise) {
60+
logger.info('Waiting for existing shared toolkit installation to complete...');
61+
return await this.installationPromise;
62+
}
63+
64+
this.installationPromise = this.installSharedToolkit(baseInterpreter, token);
65+
try {
66+
const result = await this.installationPromise;
67+
return result;
68+
} finally {
69+
this.installationPromise = undefined;
70+
}
71+
}
72+
73+
/**
74+
* Gets the path to the shared toolkit installation.
75+
*/
76+
public getSharedInstallationPath(): Uri {
77+
return this.sharedInstallationPath;
78+
}
79+
80+
/**
81+
* Tests if the shared installation can be imported by a Python interpreter.
82+
* Useful for debugging import issues.
83+
*/
84+
public async testSharedInstallation(interpreter: PythonEnvironment): Promise<boolean> {
85+
try {
86+
const processService = await this.processServiceFactory.create(interpreter.uri);
87+
88+
// Test import with explicit path
89+
const result = await processService.exec(
90+
interpreter.uri.fsPath,
91+
[
92+
'-c',
93+
`import sys; sys.path.insert(0, '${this.sharedInstallationPath.fsPath}'); import deepnote_toolkit; print('shared import successful')`
94+
],
95+
{ throwOnStdErr: false }
96+
);
97+
98+
const success = result.stdout.toLowerCase().includes('shared import successful');
99+
logger.info(`Shared installation test result: ${success ? 'SUCCESS' : 'FAILED'}`);
100+
if (!success) {
101+
logger.warn(`Shared installation test failed: stdout=${result.stdout}, stderr=${result.stderr}`);
102+
}
103+
return success;
104+
} catch (ex) {
105+
logger.error(`Shared installation test error: ${ex}`);
106+
return false;
107+
}
108+
}
109+
110+
/**
111+
* Creates a .pth file in the given venv that points to the shared toolkit installation.
112+
* @param venvInterpreter The venv Python interpreter
113+
* @param token Cancellation token
114+
*/
115+
public async createPthFile(venvInterpreter: PythonEnvironment, token?: CancellationToken): Promise<void> {
116+
Cancellation.throwIfCanceled(token);
117+
118+
// Ensure shared installation is available first
119+
const isInstalled = await this.ensureSharedInstallation(venvInterpreter, token);
120+
if (!isInstalled) {
121+
throw new Error('Failed to ensure shared deepnote-toolkit installation');
122+
}
123+
124+
// Find the correct site-packages directory by querying Python
125+
const processService = await this.processServiceFactory.create(venvInterpreter.uri);
126+
const sitePackagesResult = await processService.exec(
127+
venvInterpreter.uri.fsPath,
128+
['-c', 'import site; print(site.getsitepackages()[0])'],
129+
{ throwOnStdErr: false }
130+
);
131+
132+
if (!sitePackagesResult.stdout) {
133+
throw new Error('Failed to determine site-packages directory');
134+
}
135+
136+
const sitePackagesPath = Uri.file(sitePackagesResult.stdout.trim());
137+
138+
// Create site-packages directory if it doesn't exist
139+
if (!(await this.fs.exists(sitePackagesPath))) {
140+
await this.fs.createDirectory(sitePackagesPath);
141+
}
142+
143+
// Create .pth file pointing to shared installation
144+
const pthFilePath = Uri.joinPath(sitePackagesPath, 'deepnote-toolkit.pth');
145+
const pthContent = `${this.sharedInstallationPath.fsPath}\n`;
146+
147+
await this.fs.writeFile(pthFilePath, Buffer.from(pthContent, 'utf8'));
148+
logger.info(
149+
`Created .pth file at ${pthFilePath.fsPath} pointing to shared installation ${this.sharedInstallationPath.fsPath}`
150+
);
151+
152+
// Verify the .pth file is working by testing import
153+
const testResult = await processService.exec(
154+
venvInterpreter.uri.fsPath,
155+
['-c', 'import sys; print("\\n".join(sys.path))'],
156+
{ throwOnStdErr: false }
157+
);
158+
logger.info(`Python sys.path after .pth file creation: ${testResult.stdout}`);
159+
}
160+
161+
/**
162+
* Checks if the shared installation exists and is up to date.
163+
*/
164+
private async isInstalled(): Promise<boolean> {
165+
try {
166+
// Check if version file exists and matches current version
167+
if (!(await this.fs.exists(this.versionFilePath))) {
168+
return false;
169+
}
170+
171+
const versionContent = await this.fs.readFile(this.versionFilePath);
172+
const installedVersion = versionContent.toString().trim();
173+
174+
if (installedVersion !== this.toolkitVersion) {
175+
logger.info(`Version mismatch: installed=${installedVersion}, expected=${this.toolkitVersion}`);
176+
return false;
177+
}
178+
179+
// Check if the actual package is installed
180+
const packagePath = Uri.joinPath(this.sharedInstallationPath, 'deepnote_toolkit');
181+
return await this.fs.exists(packagePath);
182+
} catch (ex) {
183+
logger.debug(`Error checking shared installation: ${ex}`);
184+
return false;
185+
}
186+
}
187+
188+
/**
189+
* Installs the shared toolkit in the versioned directory.
190+
*/
191+
private async installSharedToolkit(
192+
baseInterpreter: PythonEnvironment,
193+
token?: CancellationToken
194+
): Promise<boolean> {
195+
try {
196+
Cancellation.throwIfCanceled(token);
197+
198+
logger.info(
199+
`Installing shared deepnote-toolkit v${this.toolkitVersion} to ${this.sharedInstallationPath.fsPath}`
200+
);
201+
this.outputChannel.appendLine(`Installing shared deepnote-toolkit v${this.toolkitVersion}...`);
202+
203+
// Create shared installation directory
204+
await this.fs.createDirectory(this.sharedInstallationPath);
205+
206+
// Remove existing installation if it exists
207+
const existingPackage = Uri.joinPath(this.sharedInstallationPath, 'deepnote_toolkit');
208+
if (await this.fs.exists(existingPackage)) {
209+
await this.fs.delete(existingPackage);
210+
}
211+
212+
// Install deepnote-toolkit to the shared directory
213+
const processService = await this.processServiceFactory.create(baseInterpreter.uri);
214+
const installResult = await processService.exec(
215+
baseInterpreter.uri.fsPath,
216+
[
217+
'-m',
218+
'pip',
219+
'install',
220+
'--target',
221+
this.sharedInstallationPath.fsPath,
222+
'--upgrade',
223+
`deepnote-toolkit[server] @ ${DEEPNOTE_TOOLKIT_WHEEL_URL}`
224+
],
225+
{ throwOnStdErr: false }
226+
);
227+
228+
Cancellation.throwIfCanceled(token);
229+
230+
if (installResult.stdout) {
231+
this.outputChannel.appendLine(installResult.stdout);
232+
}
233+
if (installResult.stderr) {
234+
this.outputChannel.appendLine(installResult.stderr);
235+
}
236+
237+
// Verify installation
238+
if (await this.fs.exists(existingPackage)) {
239+
// Write version file
240+
await this.fs.writeFile(this.versionFilePath, Buffer.from(this.toolkitVersion, 'utf8'));
241+
242+
logger.info(`Shared deepnote-toolkit v${this.toolkitVersion} installed successfully`);
243+
this.outputChannel.appendLine(`✓ Shared deepnote-toolkit v${this.toolkitVersion} ready`);
244+
return true;
245+
} else {
246+
logger.error('Shared deepnote-toolkit installation failed - package not found');
247+
this.outputChannel.appendLine('✗ Shared deepnote-toolkit installation failed');
248+
return false;
249+
}
250+
} catch (ex) {
251+
logger.error(`Failed to install shared deepnote-toolkit: ${ex}`);
252+
this.outputChannel.appendLine(`Error installing shared deepnote-toolkit: ${ex}`);
253+
return false;
254+
}
255+
}
256+
}

0 commit comments

Comments
 (0)