Skip to content
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

When the remote server is actually 'localhost' we can behave better ... #8527

Merged
merged 12 commits into from
Dec 13, 2021
1 change: 1 addition & 0 deletions news/2 Fixes/1551.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Use the appropriate directory for a notebook if remoting to 'localhost'
1 change: 1 addition & 0 deletions news/2 Fixes/8285.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Support intellisense after connecting to a remote server (defaults to active python interpreter)
26 changes: 16 additions & 10 deletions src/client/datascience/errors/errorHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,17 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {
err instanceof IpyKernelNotInstalledError &&
err.reason === KernelInterpreterDependencyResponse.uiHidden &&
(purpose === 'start' || purpose === 'restart') &&
kernelConnection.interpreter
kernelConnection.interpreter &&
kernelConnection.kind !== 'connectToLiveKernel' &&
kernelConnection.kind !== 'startUsingRemoteKernelSpec'
) {
// Its possible auto start ran and UI was disabled, but subsequently
// user attempted to run a cell, & the prompt wasn't displayed to the user.
const token = new CancellationTokenSource();
await this.kernelDependency
.installMissingDependencies(
resource,
kernelConnection.interpreter,
kernelConnection,
new DisplayOptions(false),
token.token,
true
Expand Down Expand Up @@ -142,18 +144,14 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {
if (
failureInfo.moduleName.toLowerCase().includes('ipykernel') &&
kernelConnection.interpreter &&
!(await this.kernelDependency.areDependenciesInstalled(
kernelConnection.interpreter,
undefined,
true
))
!(await this.kernelDependency.areDependenciesInstalled(kernelConnection, undefined, true))
) {
const token = new CancellationTokenSource();
try {
await this.kernelDependency
.installMissingDependencies(
resource,
kernelConnection.interpreter,
kernelConnection,
new DisplayOptions(false),
token.token,
true
Expand Down Expand Up @@ -404,7 +402,11 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {
if (!cellToDisplayErrors) {
return;
}
if (kernelConnection.kind === 'connectToLiveKernel' || !kernelConnection.interpreter) {
if (
kernelConnection.kind === 'connectToLiveKernel' ||
kernelConnection.kind === 'startUsingRemoteKernelSpec' ||
!kernelConnection.interpreter
) {
return;
}
const displayNameOfKernel = kernelConnection.interpreter.displayName || kernelConnection.interpreter.path;
Expand Down Expand Up @@ -443,7 +445,11 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {
if (!cellToDisplayErrors) {
return;
}
if (kernelConnection.kind === 'connectToLiveKernel' || !kernelConnection.interpreter) {
if (
kernelConnection.kind === 'connectToLiveKernel' ||
kernelConnection.kind === 'startUsingRemoteKernelSpec' ||
!kernelConnection.interpreter
) {
Copy link
Contributor

@DonJayamanne DonJayamanne Dec 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops,

return;
}
const productNames = `${ProductNames.get(Product.jupyter)} ${Common.and()} ${ProductNames.get(
Expand Down
2 changes: 2 additions & 0 deletions src/client/datascience/errors/ipyKernelNotInstalledError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@
// Licensed under the MIT License.

import { BaseError } from '../../common/errors/types';
import { traceError } from '../../common/logger';
import { KernelInterpreterDependencyResponse } from '../types';

export class IpyKernelNotInstalledError extends BaseError {
constructor(message: string, public reason: KernelInterpreterDependencyResponse) {
super('noipykernel', message);
traceError(`IPykernel not detected`);
}
}
23 changes: 19 additions & 4 deletions src/client/datascience/jupyter/kernels/jupyterKernelService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,23 @@ export class JupyterKernelService {
const token = wrapCancellationTokens(cancelToken, tokenSource.token);

// If we have an interpreter, make sure it has the correct dependencies installed
if (kernel.kind !== 'connectToLiveKernel' && kernel.interpreter) {
await this.kernelDependencyService.installMissingDependencies(resource, kernel.interpreter, ui, token);
if (
kernel.kind !== 'connectToLiveKernel' &&
kernel.interpreter &&
kernel.kind !== 'startUsingRemoteKernelSpec'
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoot even more, yup, i think i know what would have happened...
Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No these are my fault. A remote kernel couldn't have an interpreter until my change. So your old if statement would have worked.

await this.kernelDependencyService.installMissingDependencies(resource, kernel, ui, token);
}

var specFile: string | undefined = undefined;

// If the spec file doesn't exist or is not defined, we need to register this kernel
if (kernel.kind !== 'connectToLiveKernel' && kernel.kernelSpec && kernel.interpreter) {
if (
kernel.kind !== 'connectToLiveKernel' &&
kernel.kind !== 'startUsingRemoteKernelSpec' &&
kernel.kernelSpec &&
kernel.interpreter
) {
// Default to the kernel spec file.
specFile = kernel.kernelSpec.specFile;

Expand All @@ -86,7 +95,13 @@ export class JupyterKernelService {
}

// Update the kernel environment to use the interpreter's latest
if (kernel.kind !== 'connectToLiveKernel' && kernel.kernelSpec && kernel.interpreter && specFile) {
if (
kernel.kind !== 'connectToLiveKernel' &&
kernel.kind !== 'startUsingRemoteKernelSpec' &&
kernel.kernelSpec &&
kernel.interpreter &&
specFile
) {
traceInfoIfCI(
`updateKernelEnvironment ${kernel.interpreter.displayName}, ${getDisplayPath(
kernel.interpreter.path
Expand Down
17 changes: 14 additions & 3 deletions src/client/datascience/jupyter/kernels/kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,13 @@ import {
sendTelemetryForPythonKernelExecutable
} from './helpers';
import { KernelExecution } from './kernelExecution';
import { IKernel, isLocalConnection, KernelConnectionMetadata, NotebookCellRunState } from './types';
import {
IKernel,
isLocalConnection,
isLocalHostConnection,
KernelConnectionMetadata,
NotebookCellRunState
} from './types';
import { SysInfoReason } from '../../interactive-common/interactiveWindowTypes';
import { MARKDOWN_LANGUAGE } from '../../../common/constants';
import { InteractiveWindowView } from '../../notebook/constants';
Expand Down Expand Up @@ -837,7 +843,11 @@ export class Kernel implements IKernel {

private async updateWorkingDirectoryAndPath(launchingFile?: string): Promise<void> {
traceInfo('UpdateWorkingDirectoryAndPath in Kernel');
if (isLocalConnection(this.kernelConnectionMetadata)) {
if (
(isLocalConnection(this.kernelConnectionMetadata) ||
isLocalHostConnection(this.kernelConnectionMetadata)) &&
this.kernelConnectionMetadata.kind !== 'connectToLiveKernel' // Skip for live kernel. Don't change current directory on a kernel that's already running
) {
let suggestedDir = await calculateWorkingDirectory(this.configService, this.workspaceService, this.fs);
if (suggestedDir && (await this.fs.localDirectoryExists(suggestedDir))) {
// We should use the launch info directory. It trumps the possible dir
Expand All @@ -855,7 +865,8 @@ export class Kernel implements IKernel {
// Update both current working directory and sys.path with the desired directory
private async changeDirectoryIfPossible(directory: string): Promise<void> {
if (
isLocalConnection(this.kernelConnectionMetadata) &&
(isLocalConnection(this.kernelConnectionMetadata) ||
isLocalHostConnection(this.kernelConnectionMetadata)) &&
isPythonKernelConnection(this.kernelConnectionMetadata)
) {
traceInfo('changeDirectoryIfPossible');
Expand Down
50 changes: 37 additions & 13 deletions src/client/datascience/jupyter/kernels/kernelDependencyService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
KernelInterpreterDependencyResponse
} from '../../types';
import { selectKernel } from './kernelSelector';
import { KernelConnectionMetadata } from './types';

/**
* Responsible for managing dependencies of a Python interpreter required to run as a Jupyter Kernel.
Expand All @@ -64,24 +65,31 @@ export class KernelDependencyService implements IKernelDependencyService {
@traceDecorators.verbose('Install Missing Dependencies', TraceOptions.ReturnValue)
public async installMissingDependencies(
resource: Resource,
interpreter: PythonEnvironment,
kernelConnection: KernelConnectionMetadata,
ui: IDisplayOptions,
@ignoreLogging() token: CancellationToken,
ignoreCache?: boolean
): Promise<void> {
traceInfo(`installMissingDependencies ${getDisplayPath(interpreter.path)}`);
if (await this.areDependenciesInstalled(interpreter, token, ignoreCache)) {
traceInfo(`installMissingDependencies ${getDisplayPath(kernelConnection.interpreter?.path)}`);
if (
kernelConnection.kind === 'connectToLiveKernel' ||
kernelConnection.kind === 'startUsingRemoteKernelSpec' ||
kernelConnection.interpreter === undefined
) {
return;
}
if (await this.areDependenciesInstalled(kernelConnection, token, ignoreCache)) {
return;
}
if (token?.isCancellationRequested) {
return;
}

// Cache the install run
let promise = this.installPromises.get(interpreter.path);
let promise = this.installPromises.get(kernelConnection.interpreter.path);
if (!promise) {
promise = this.runInstaller(resource, interpreter, ui, token);
this.installPromises.set(interpreter.path, promise);
promise = this.runInstaller(resource, kernelConnection.interpreter, ui, token);
this.installPromises.set(kernelConnection.interpreter.path, promise);
}

// Get the result of the question
Expand All @@ -90,29 +98,45 @@ export class KernelDependencyService implements IKernelDependencyService {
if (token?.isCancellationRequested) {
return;
}
await this.handleKernelDependencyResponse(result, interpreter, resource);
await this.handleKernelDependencyResponse(result, kernelConnection.interpreter, resource);
} finally {
// Don't need to cache anymore
this.installPromises.delete(interpreter.path);
this.installPromises.delete(kernelConnection.interpreter.path);
}
}
public async areDependenciesInstalled(
interpreter: PythonEnvironment,
kernelConnection: KernelConnectionMetadata,
token?: CancellationToken,
ignoreCache?: boolean
): Promise<boolean> {
if (
kernelConnection.kind === 'connectToLiveKernel' ||
kernelConnection.kind === 'startUsingRemoteKernelSpec' ||
kernelConnection.interpreter === undefined
) {
return true;
}
// Check cache, faster than spawning process every single time.
// Makes a big difference with conda on windows.
if (!ignoreCache && isModulePresentInEnvironmentCache(this.memento, Product.ipykernel, interpreter)) {
traceInfo(`IPykernel found previously in this environment ${getDisplayPath(interpreter.path)}`);
if (
!ignoreCache &&
isModulePresentInEnvironmentCache(this.memento, Product.ipykernel, kernelConnection.interpreter)
) {
traceInfo(
`IPykernel found previously in this environment ${getDisplayPath(kernelConnection.interpreter.path)}`
);
return true;
}
const installedPromise = this.installer
.isInstalled(Product.ipykernel, interpreter)
.isInstalled(Product.ipykernel, kernelConnection.interpreter)
.then((installed) => installed === true);
void installedPromise.then((installed) => {
if (installed) {
void trackPackageInstalledIntoInterpreter(this.memento, Product.ipykernel, interpreter);
void trackPackageInstalledIntoInterpreter(
this.memento,
Product.ipykernel,
kernelConnection.interpreter
);
}
});
return Promise.race([
Expand Down
11 changes: 10 additions & 1 deletion src/client/datascience/jupyter/kernels/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type {
KernelSocketInformation
} from '../../types';
import type * as nbformat from '@jupyterlab/nbformat';
import * as url from 'url';

export type LiveKernelModel = IJupyterKernel &
Partial<IJupyterKernelSpec> & { model: Session.IModel | undefined; notebook?: { path?: string } };
Expand Down Expand Up @@ -63,7 +64,7 @@ export type LocalKernelSpecConnectionMetadata = Readonly<{
*/
export type RemoteKernelSpecConnectionMetadata = Readonly<{
kernelModel?: undefined;
interpreter?: undefined;
interpreter?: PythonEnvironment; // Can be set if URL is localhost
kernelSpec: IJupyterKernelSpec;
kind: 'startUsingRemoteKernelSpec';
baseUrl: string;
Expand Down Expand Up @@ -112,6 +113,14 @@ export function isLocalConnection(
);
}

export function isLocalHostConnection(kernelConnection: KernelConnectionMetadata): boolean {
if (kernelConnection.kind === 'connectToLiveKernel' || kernelConnection.kind === 'startUsingRemoteKernelSpec') {
const parsed = new url.URL(kernelConnection.baseUrl);
return parsed.hostname.toLocaleLowerCase() === 'localhost' || parsed.hostname === '127.0.0.1';
}
return false;
}

export interface IKernel extends IAsyncDisposable {
readonly connection: INotebookProviderConnection | undefined;
readonly notebookDocument: NotebookDocument;
Expand Down
11 changes: 10 additions & 1 deletion src/client/datascience/kernel-launcher/kernelDaemonPool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { IInterpreterService } from '../../interpreter/contracts';
import { logValue, TraceOptions } from '../../logging/trace';
import { PythonEnvironment } from '../../pythonEnvironments/info';
import { KernelLauncherDaemonModule } from '../constants';
import { createInterpreterKernelSpec } from '../jupyter/kernels/helpers';
import { IJupyterKernelSpec, IKernelDependencyService } from '../types';
import { PythonKernelDaemon } from './kernelDaemon';
import { IPythonKernelDaemon } from './types';
Expand Down Expand Up @@ -140,7 +141,15 @@ export class KernelDaemonPool implements IDisposable {
private async preWarmKernelDaemon(resource: Resource) {
traceInfo(`Pre-warming kernel daemon for ${getDisplayPath(resource)}`);
const interpreter = await this.interpreterService.getActiveInterpreter(resource);
if (!interpreter || !(await this.kernelDependencyService.areDependenciesInstalled(interpreter))) {
if (
!interpreter ||
!(await this.kernelDependencyService.areDependenciesInstalled({
interpreter,
kind: 'startUsingPythonInterpreter',
kernelSpec: createInterpreterKernelSpec(interpreter, ''),
id: '1'
}))
) {
return;
}
const key = this.getDaemonKey(resource, interpreter);
Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/kernel-launcher/kernelLauncher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export class KernelLauncher implements IKernelLauncher {
if (kernelConnectionMetadata.interpreter) {
await this.kernelDependencyService.installMissingDependencies(
resource,
kernelConnectionMetadata.interpreter,
kernelConnectionMetadata,
ui,
cancelToken
);
Expand Down
34 changes: 24 additions & 10 deletions src/client/datascience/kernel-launcher/remoteKernelFinder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import { Kernel } from '@jupyterlab/services';
import type * as nbformat from '@jupyterlab/nbformat';
import * as url from 'url';
import { injectable, inject } from 'inversify';
import { CancellationToken } from 'vscode';
import { IDisposableRegistry, Resource } from '../../common/types';
Expand All @@ -30,6 +31,7 @@ import { PYTHON_LANGUAGE } from '../../common/constants';
import { getTelemetrySafeLanguage } from '../../telemetry/helpers';
import { sendKernelListTelemetry } from '../telemetry/kernelTelemetry';
import { ignoreLogging } from '../../logging/trace';
import { IInterpreterService } from '../../interpreter/contracts';

// This class searches for a kernel that matches the given kernel name.
// First it searches on a global persistent state, then on the installed python interpreters,
Expand All @@ -44,7 +46,8 @@ export class RemoteKernelFinder implements IRemoteKernelFinder {
@inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry,
@inject(PreferredRemoteKernelIdProvider)
private readonly preferredRemoteKernelIdProvider: PreferredRemoteKernelIdProvider,
@inject(IJupyterSessionManagerFactory) private jupyterSessionManagerFactory: IJupyterSessionManagerFactory
@inject(IJupyterSessionManagerFactory) private jupyterSessionManagerFactory: IJupyterSessionManagerFactory,
@inject(IInterpreterService) private interpreterService: IInterpreterService
) {
disposableRegistry.push(
this.jupyterSessionManagerFactory.onRestartSessionCreated(this.addKernelToIgnoreList.bind(this))
Expand Down Expand Up @@ -120,15 +123,18 @@ export class RemoteKernelFinder implements IRemoteKernelFinder {
]);

// Turn them both into a combined list
const mappedSpecs = specs.map((s) => {
const kernel: RemoteKernelSpecConnectionMetadata = {
kind: 'startUsingRemoteKernelSpec',
kernelSpec: s,
id: getKernelId(s, undefined, connInfo.baseUrl),
baseUrl: connInfo.baseUrl
};
return kernel;
});
const mappedSpecs = await Promise.all(
specs.map(async (s) => {
const kernel: RemoteKernelSpecConnectionMetadata = {
kind: 'startUsingRemoteKernelSpec',
interpreter: await this.getInterpreter(s, connInfo.baseUrl),
kernelSpec: s,
id: getKernelId(s, undefined, connInfo.baseUrl),
baseUrl: connInfo.baseUrl
};
return kernel;
})
);
const mappedLive = sessions.map((s) => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const liveKernel = s.kernel as any;
Expand Down Expand Up @@ -196,4 +202,12 @@ export class RemoteKernelFinder implements IRemoteKernelFinder {
this.kernelIdsToHide.delete(kernel.id);
this.kernelIdsToHide.delete(kernel.clientId);
}

private async getInterpreter(spec: IJupyterKernelSpec, baseUrl: string) {
const parsed = new url.URL(baseUrl);
if (parsed.hostname.toLocaleLowerCase() === 'localhost' || parsed.hostname === '127.0.0.1') {
// Interpreter is possible. Same machine as VS code
return this.interpreterService.getInterpreterDetails(spec.argv[0]);
}
}
}
Loading