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)
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,12 @@ export class JupyterKernelService {
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' &&
Copy link
Member

Choose a reason for hiding this comment

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

missing =?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Thanks. Which also prompted me to lookup the difference. === does not do any type manipulation and just compares the values. == will convert the values to the same types. So I guess === is faster.

kernel.kernelSpec &&
kernel.interpreter
) {
// Default to the kernel spec file.
specFile = kernel.kernelSpec.specFile;

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
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.host === '127.0.0.1';
Copy link
Member

Choose a reason for hiding this comment

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

I was going to ask if localhost is more configurable, and I believe it is. But I think that we only have to worry about how Jupyter starts it, so this is fine I believe.

Copy link
Member

Choose a reason for hiding this comment

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

Was going to ask if we needed to check for an npm module or something for a more comprehensive loopback check, but I don't think we need to.

}
return false;
}

export interface IKernel extends IAsyncDisposable {
readonly connection: INotebookProviderConnection | undefined;
readonly notebookDocument: NotebookDocument;
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]);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,18 @@ export class IntellisenseProvider implements INotebookLanguageClientProvider, IE
const notebookInterpreter = controller
? controller.connection.interpreter
: this.getActiveInterpreterSync(uri.fsPath);
const notebookId = notebookInterpreter ? this.getInterpreterIdFromCache(notebookInterpreter) : undefined;
let notebookId = notebookInterpreter ? this.getInterpreterIdFromCache(notebookInterpreter) : undefined;

// Special case. For remote use the active interpreter as the controller's interpreter isn't
// usable by pylance.
if (
interpreterId !== notebookId &&
(controller?.connection.kind === 'startUsingRemoteKernelSpec' ||
controller?.connection.kind === 'connectToLiveKernel')
) {
const activeInterpreter = this.getActiveInterpreterSync(uri.fsPath);
notebookId = activeInterpreter ? this.getInterpreterIdFromCache(activeInterpreter) : undefined;
}

return interpreterId == notebookId;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@ import { NotebookDocument } from 'vscode';
import { IExtensionSingleActivationService } from '../../activation/types';
import { IPythonInstaller, IPythonExtensionChecker, IPythonApiProvider } from '../../api/types';
import { InterpreterUri } from '../../common/installer/types';
import { IExtensions, IDisposableRegistry, Product, IConfigurationService } from '../../common/types';
import { IExtensions, IDisposableRegistry, Product } from '../../common/types';
import { isResource, noop } from '../../common/utils/misc';
import { IInterpreterService } from '../../interpreter/contracts';
import { isLocalLaunch } from '../jupyter/kernels/helpers';
import { InterpreterPackages } from './interpreterPackages';
import { INotebookControllerManager } from '../notebook/types';
import { VSCodeNotebookController } from '../notebook/vscodeNotebookController';
Expand All @@ -26,13 +25,9 @@ export class InterpreterPackageTracker implements IExtensionSingleActivationServ
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
@inject(IInterpreterService) private readonly interpreterService: IInterpreterService,
@inject(IPythonApiProvider) private readonly apiProvider: IPythonApiProvider,
@inject(IConfigurationService) private readonly configurationService: IConfigurationService,
@inject(INotebookControllerManager) private readonly notebookControllerManager: INotebookControllerManager
) {}
public async activate(): Promise<void> {
if (!isLocalLaunch(this.configurationService)) {
return;
}
this.notebookControllerManager.onNotebookControllerSelected(
this.onNotebookControllerSelected,
this,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@ import { JupyterSessionManagerFactory } from '../../../client/datascience/jupyte
import { JupyterSessionManager } from '../../../client/datascience/jupyter/jupyterSessionManager';
import { noop } from '../../core';
import { LiveKernelConnectionMetadata } from '../../../client/datascience/jupyter/kernels/types';
import { IInterpreterService } from '../../../client/interpreter/contracts';

suite(`Remote Kernel Finder`, () => {
let disposables: Disposable[] = [];
let preferredRemoteKernelIdProvider: PreferredRemoteKernelIdProvider;
let kernelFinder: IRemoteKernelFinder;
let jupyterSessionManager: IJupyterSessionManager;
const dummyEvent = new EventEmitter<number>();
let interpreterService: IInterpreterService;
let sessionCreatedEvent: EventEmitter<Kernel.IKernelConnection>;
let sessionUsedEvent: EventEmitter<Kernel.IKernelConnection>;
const connInfo: IJupyterConnection = {
Expand Down Expand Up @@ -108,11 +110,13 @@ suite(`Remote Kernel Finder`, () => {
sessionUsedEvent = new EventEmitter<Kernel.IKernelConnection>();
when(jupyterSessionManagerFactory.onRestartSessionCreated).thenReturn(sessionCreatedEvent.event);
when(jupyterSessionManagerFactory.onRestartSessionUsed).thenReturn(sessionUsedEvent.event);
interpreterService = mock<IInterpreterService>();

kernelFinder = new RemoteKernelFinder(
disposables,
preferredRemoteKernelIdProvider,
instance(jupyterSessionManagerFactory)
instance(jupyterSessionManagerFactory),
instance(interpreterService)
);
});
teardown(() => {
Expand Down
28 changes: 28 additions & 0 deletions src/test/datascience/notebook/remoteNotebookEditor.vscode.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -334,4 +334,32 @@ suite('DataScience - VSCode Notebook - (Remote) (Execution) (slow)', function ()
const cell = vscodeNotebook.activeNotebookEditor?.document.cellAt(0)!;
await Promise.all([runCell(cell), waitForTextOutput(cell, '123412341234')]);
});

test('Remote kernels support intellisense', async function () {
await openNotebook(ipynbFile.fsPath);
await waitForKernelToGetAutoSelected(PYTHON_LANGUAGE);
let nbEditor = vscodeNotebook.activeNotebookEditor!;
assert.isOk(nbEditor, 'No active notebook');
// Cell 1 = `a = "Hello World"`
// Cell 2 = `print(a)`
let cell2 = nbEditor.document.getCells()![1]!;
await Promise.all([
runAllCellsInActiveNotebook(),
waitForExecutionCompletedSuccessfully(cell2),
waitForTextOutput(cell2, 'Hello World', 0, false)
]);

// Wait for tokens on the second cell (it works with just plain pylance)
await waitForCondition(
async () => {
const promise = commands.executeCommand('vscode.provideDocumentSemanticTokens', cell2.document.uri);
const result = (await promise) as any;
return result && result.data.length > 0;
},
defaultNotebookTestTimeout,
`Tokens never appear for first cell`,
100,
true
);
});
});