Skip to content

Commit

Permalink
Fix Python interpreter not found error (#5793)
Browse files Browse the repository at this point in the history 
Attempts to address #5730. My hunch is that `getInterpreters()` doesn't
wait for the Python extension to complete its discovery process, so it's
possible to get there before our runtime's interpreter is discovered.

The whole point of restoring a workspace-affiliated runtime is to skip
discovery. It looks like `getInterpreterDetails()` is a better fit,
since (I think) it "resolves" an interpreter path to a
`PythonEnvironment` independent of discovery.

### QA Notes

E2E tests C609619 and C609617 should pass in CI:

* Passing E2E run:
https://github.com/posit-dev/positron/actions/runs/12387951151/job/34578222238
* 10x run of both failing tests:
https://d38p2avprg8il3.cloudfront.net/playwright-report-12389107722-6803/index.html.
There was a single failure, but that looks like an unrelated issue.

Python and Reticulate runtimes should continue to function as expected.
  • Loading branch information
@seeM
seeM authored Dec 19, 2024
1 parent 69a2e14 commit 61a4094
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 46 deletions.
58 changes: 29 additions & 29 deletions extensions/positron-python/src/client/positron/session.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs
/** The current state of the runtime */
private _state: positron.RuntimeState = positron.RuntimeState.Uninitialized;

/** The service for getting the Python extension interpreter path */
/** The service for managing Python environments */
private _interpreterService: IInterpreterService;

/** The service for managing the active Python interpreter path */
private _interpreterPathService: IInterpreterPathService;

/**
Expand All @@ -82,37 +85,28 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs
*/
private _parentIdsByOutputCommId = new Map<string, string>();

dynState: positron.LanguageRuntimeDynState;
/** The Python interpreter executable path */
private _pythonPath: string;

private readonly interpreter: PythonEnvironment;
dynState: positron.LanguageRuntimeDynState;

constructor(
readonly runtimeMetadata: positron.LanguageRuntimeMetadata,
readonly metadata: positron.RuntimeSessionMetadata,
readonly serviceContainer: IServiceContainer,
readonly kernelSpec?: JupyterKernelSpec | undefined,
) {
const interpreterService = serviceContainer.get<IInterpreterService>(IInterpreterService);

this.onDidReceiveRuntimeMessage = this._messageEmitter.event;
this.onDidChangeRuntimeState = this._stateEmitter.event;
this.onDidEndSession = this._exitEmitter.event;

// Extract the extra data from the runtime metadata; it contains the
// environment ID that was saved when the metadata was created.
// Python path that was saved when the metadata was created.
const extraData: PythonRuntimeExtraData = runtimeMetadata.extraRuntimeData as PythonRuntimeExtraData;
if (!extraData || !extraData.pythonEnvironmentId) {
throw new Error(`Runtime metadata missing Python environment ID: ${JSON.stringify(runtimeMetadata)}`);
if (!extraData || !extraData.pythonPath) {
throw new Error(`Runtime metadata missing Python path: ${JSON.stringify(runtimeMetadata)}`);
}

const interpreter = interpreterService.getInterpreters().find((i) => i.id === extraData.pythonEnvironmentId);
if (!interpreter) {
const interpreterIds = interpreterService.getInterpreters().map((i) => `\n- ${i.id}`);
throw new Error(
`Interpreter ${extraData.pythonEnvironmentId} (path: ${extraData.pythonPath}) not found in available Python interpreters: ${interpreterIds}`,
);
}
this.interpreter = interpreter;
this._pythonPath = extraData.pythonPath;

this._queue = new PQueue({ concurrency: 1 });

Expand All @@ -125,6 +119,7 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs
this.onStateChange(state);
});

this._interpreterService = serviceContainer.get<IInterpreterService>(IInterpreterService);
this._interpreterPathService = serviceContainer.get<IInterpreterPathService>(IInterpreterPathService);
}

Expand Down Expand Up @@ -234,7 +229,7 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs
}
}

private async _installIpykernel(): Promise<void> {
private async _installIpykernel(interpreter: PythonEnvironment): Promise<void> {
// Get the installer service
const installer = this.serviceContainer.get<IInstaller>(IInstaller);

Expand All @@ -246,16 +241,16 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs
const hasCompatibleKernel = await installer.isProductVersionCompatible(
Product.ipykernel,
IPYKERNEL_VERSION,
this.interpreter,
interpreter,
);

if (hasCompatibleKernel !== ProductInstallStatus.Installed) {
// Check if sqlite3 if installed before attempting to install ipykernel
// https://github.com/posit-dev/positron/issues/4698
const hasSqlite3 = await installer.isInstalled(Product.sqlite3, this.interpreter);
const hasSqlite3 = await installer.isInstalled(Product.sqlite3, interpreter);
if (!hasSqlite3) {
throw new Error(
`The Python sqlite3 extension is required but not installed for interpreter: ${this.interpreter?.displayName}. Missing the system library for SQLite?`,
`The Python sqlite3 extension is required but not installed for interpreter: ${interpreter?.displayName}. Missing the system library for SQLite?`,
);
}

Expand All @@ -280,7 +275,7 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs

const response = await installer.promptToInstall(
product,
this.interpreter,
interpreter,
installerToken,
undefined,
installOptions,
Expand All @@ -289,12 +284,12 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs

switch (response) {
case InstallerResponse.Installed:
traceInfo(`Successfully installed ipykernel for ${this.interpreter?.displayName}`);
traceInfo(`Successfully installed ipykernel for ${interpreter?.displayName}`);
break;
case InstallerResponse.Ignore:
case InstallerResponse.Disabled:
throw new Error(
`Could not start runtime: failed to install ipykernel for ${this.interpreter?.displayName}.`,
`Could not start runtime: failed to install ipykernel for ${interpreter?.displayName}.`,
);
default:
throw new Error(`Unknown installer response type: ${response}`);
Expand All @@ -303,9 +298,14 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs
}

async start(): Promise<positron.LanguageRuntimeInfo> {
const interpreter = await this._interpreterService.getInterpreterDetails(this._pythonPath);
if (!interpreter) {
throw new Error(`Could not start runtime: failed to resolve interpreter ${this._pythonPath}`);
}

// Ensure the LSP client instance is created
if (!this._lsp) {
await this.createLsp();
await this.createLsp(interpreter);
}

// Ensure the Jupyter kernel instance is created
Expand All @@ -314,14 +314,14 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs
}

// Ensure that the ipykernel module is installed for the interpreter.
await this._installIpykernel();
await this._installIpykernel(interpreter);

if (this.metadata.sessionMode === positron.LanguageRuntimeSessionMode.Console) {
// Update the active environment in the Python extension.
this._interpreterPathService.update(
undefined,
vscode.ConfigurationTarget.WorkspaceFolder,
this.interpreter.path,
interpreter.path,
);
}

Expand Down Expand Up @@ -371,7 +371,7 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs
// Keep track of LSP init to avoid stopping in the middle of startup
private _lspStarting: Thenable<void> = Promise.resolve();

private async createLsp() {
private async createLsp(interpreter: PythonEnvironment): Promise<void> {
traceInfo(`createPythonSession: resolving LSP services`);
const environmentService = this.serviceContainer.get<IEnvironmentVariablesProvider>(
IEnvironmentVariablesProvider,
Expand All @@ -389,7 +389,7 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs
);

const resource = workspaceService.workspaceFolders?.[0].uri;
await analysisOptions.initialize(resource, this.interpreter);
await analysisOptions.initialize(resource, interpreter);
const languageClientOptions = await analysisOptions.getAnalysisOptions();

this._lsp = new PythonLsp(
Expand Down
4 changes: 2 additions & 2 deletions extensions/positron-python/src/test/positron/session.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ suite('Python Runtime Session', () => {
} as PythonEnvironment;

runtimeMetadata = {
extraRuntimeData: { pythonEnvironmentId: interpreter.id },
extraRuntimeData: { pythonPath: interpreter.path },
} as positron.LanguageRuntimeMetadata;

const interpreterService = {
getInterpreters: () => [interpreter],
getInterpreterDetails: (_pythonPath, _resource) => Promise.resolve(interpreter),
} as IInterpreterService;

const installer = ({
Expand Down
11 changes: 1 addition & 10 deletions extensions/positron-reticulate/src/extension.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -311,15 +311,6 @@ class ReticulateRuntimeSession implements positron.LanguageRuntimeSession {

const output = runtimeMetadata;
output.runtimePath = interpreterPath;

// Uses the Positron Python extension API to query the environment id
// given it's path.
const api = vscode.extensions.getExtension('ms-python.python');
if (!api) {
throw new Error('Failed to find the Positron Python extension API.');
}
const env = await api.exports.environments.resolveEnvironment(interpreterPath);
output.extraRuntimeData.pythonEnvironmentId = env.id;
output.extraRuntimeData.pythonPath = interpreterPath;

return output;
Expand Down Expand Up @@ -620,7 +611,7 @@ async function getRSession_(): Promise<positron.LanguageRuntimeSession> {

class ReticulateRuntimeMetadata implements positron.LanguageRuntimeMetadata {
extraRuntimeData: any = {
pythonEnvironmentId: 'reticulate',
pythonPath: 'Managed by the reticulate package',
};
base64EncodedIconSvg: string | undefined;
constructor() {
Expand Down
4 changes: 2 additions & 2 deletions src/vs/workbench/services/runtimeStartup/common/runtimeStartup.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -741,8 +741,8 @@ export class RuntimeStartupService extends Disposable implements IRuntimeStartup
await this.restoreWorkspaceSessions(sessions);
}
} catch (err) {
this._logService.error(`Could not restore workspace sessions: ${err} ` +
`(data: ${storedSessions})`);
this._logService.error(`Could not restore workspace sessions: ${err?.stack ?? err} ` +
`(data: ${JSON.stringify(storedSessions)})`);
}
}

Expand Down
5 changes: 2 additions & 3 deletions test/e2e/areas/new-project-wizard/new-project-python.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,7 @@ test.describe('Python - New Project Wizard', { tag: [tags.NEW_PROJECT_WIZARD] },
await app.workbench.positronQuickaccess.runCommand('workbench.action.toggleAuxiliaryBar');
});

// Skip test due to https://github.com/posit-dev/positron/issues/5730. Both have to skipped as they depend o
test.skip('With ipykernel already installed [C609619]', {
test('With ipykernel already installed [C609619]', {
tag: [tags.WIN],
annotation: [{ type: 'issue', description: 'https://github.com/posit-dev/positron/issues/5730' }],
}, async function ({ app, page, python }) {
Expand Down Expand Up @@ -100,7 +99,7 @@ test.describe('Python - New Project Wizard', { tag: [tags.NEW_PROJECT_WIZARD] },
await expect(app.workbench.positronConsole.activeConsole.getByText('>>>')).toBeVisible({ timeout: 90000 });
});

test.skip('With ipykernel not already installed [C609617]', {
test('With ipykernel not already installed [C609617]', {
tag: [tags.WIN],
annotation: [{ type: 'issue', description: 'https://github.com/posit-dev/positron/issues/5730' }],
}, async function ({ app, page }) {
Expand Down

0 comments on commit 61a4094

Please sign in to comment.