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

Fix kernel and server name missing in certain situations #13974

Merged
merged 4 commits into from
Sep 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/2 Fixes/13955.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make sure server name and kernel name show up when connecting.
80 changes: 52 additions & 28 deletions src/client/datascience/interactive-common/interactiveBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ import { captureTelemetry, sendTelemetryEvent } from '../../telemetry';
import { generateCellRangesFromDocument } from '../cellFactory';
import { CellMatcher } from '../cellMatcher';
import { addToUriList, translateKernelLanguageToMonaco } from '../common';
import { Commands, Identifiers, Telemetry } from '../constants';
import { Commands, Identifiers, Settings, Telemetry } from '../constants';
import { ColumnWarningSize, IDataViewerFactory } from '../data-viewing/types';
import {
IAddedSysInfo,
Expand Down Expand Up @@ -865,9 +865,54 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo
} catch (e) {
this.errorHandler.handleError(e).ignoreErrors();
}
} else {
// Just send a kernel update so it shows something
this.postMessage(InteractiveWindowMessages.UpdateKernel, {
jupyterServerStatus: ServerStatus.NotStarted,
serverName: await this.getServerDisplayName(undefined),
kernelName: '',
language: PYTHON_LANGUAGE
}).ignoreErrors();
}
}

protected async getServerDisplayName(serverConnection: INotebookProviderConnection | undefined): Promise<string> {
// If we don't have a server connection, make one if remote. We need the remote connection in order
// to compute the display name. However only do this if the user is allowing auto start.
if (
!serverConnection &&
this.configService.getSettings(this.owningResource).datascience.jupyterServerURI !==
Settings.JupyterServerLocalLaunch &&
!this.configService.getSettings(this.owningResource).datascience.disableJupyterAutoStart
) {
serverConnection = await this.notebookProvider.connect({ disableUI: true });
}

let displayName =
serverConnection?.displayName ||
(!serverConnection?.localLaunch ? serverConnection?.url : undefined) ||
(this.configService.getSettings().datascience.jupyterServerURI === Settings.JupyterServerLocalLaunch
? localize.DataScience.localJupyterServer()
: localize.DataScience.serverNotStarted());

if (serverConnection) {
// Determine the connection URI of the connected server to display
if (serverConnection.localLaunch) {
displayName = localize.DataScience.localJupyterServer();
} else {
// Log this remote URI into our MRU list
addToUriList(
this.globalStorage,
!isNil(serverConnection.url) ? serverConnection.url : serverConnection.displayName,
Date.now(),
serverConnection.displayName
);
}
}

return displayName;
}

private combineData(
oldData: nbformat.ICodeCell | nbformat.IRawCell | nbformat.IMarkdownCell | undefined,
cell: ICell
Expand Down Expand Up @@ -1154,36 +1199,15 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo
return notebook;
}

private getServerUri(serverConnection: INotebookProviderConnection | undefined): string {
let localizedUri = '';

if (serverConnection) {
// Determine the connection URI of the connected server to display
if (serverConnection.localLaunch) {
localizedUri = localize.DataScience.localJupyterServer();
} else {
// Log this remote URI into our MRU list
addToUriList(
this.globalStorage,
!isNil(serverConnection.url) ? serverConnection.url : serverConnection.displayName,
Date.now(),
serverConnection.displayName
);
}
}

return localizedUri;
}

private async listenToNotebookEvents(notebook: INotebook): Promise<void> {
const statusChangeHandler = async (status: ServerStatus) => {
const connectionMetadata = notebook.getKernelConnection();
const name = getDisplayNameOrNameOfKernelConnection(connectionMetadata);

await this.postMessage(InteractiveWindowMessages.UpdateKernel, {
jupyterServerStatus: status,
localizedUri: this.getServerUri(notebook.connection),
displayName: name,
serverName: await this.getServerDisplayName(notebook.connection),
kernelName: name,
language: translateKernelLanguageToMonaco(
getKernelConnectionLanguage(connectionMetadata) || PYTHON_LANGUAGE
)
Expand All @@ -1205,8 +1229,8 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo
// While waiting make the notebook look busy
this.postMessage(InteractiveWindowMessages.UpdateKernel, {
jupyterServerStatus: ServerStatus.Busy,
localizedUri: this.getServerUri(serverConnection),
displayName: '',
serverName: await this.getServerDisplayName(serverConnection),
kernelName: '',
language: PYTHON_LANGUAGE
}).ignoreErrors();

Expand Down Expand Up @@ -1234,8 +1258,8 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo
// No notebook, send update to UI anyway
this.postMessage(InteractiveWindowMessages.UpdateKernel, {
jupyterServerStatus: ServerStatus.NotStarted,
localizedUri: '',
displayName: getDisplayNameOrNameOfKernelConnection(data.kernelConnection),
serverName: await this.getServerDisplayName(undefined),
kernelName: getDisplayNameOrNameOfKernelConnection(data.kernelConnection),
language: translateKernelLanguageToMonaco(
getKernelConnectionLanguage(data.kernelConnection) || PYTHON_LANGUAGE
)
Expand Down
4 changes: 2 additions & 2 deletions src/client/datascience/interactive-ipynb/nativeEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -751,8 +751,8 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
if (!this.notebook && metadata?.kernelspec) {
this.postMessage(InteractiveWindowMessages.UpdateKernel, {
jupyterServerStatus: ServerStatus.NotStarted,
localizedUri: '',
displayName: metadata.kernelspec.display_name ?? metadata.kernelspec.name,
serverName: await this.getServerDisplayName(undefined),
kernelName: metadata.kernelspec.display_name ?? metadata.kernelspec.name,
language: translateKernelLanguageToMonaco(
(metadata.kernelspec.language as string) ?? PYTHON_LANGUAGE
)
Expand Down
7 changes: 6 additions & 1 deletion src/client/datascience/jupyter/kernels/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,12 @@ export function getDisplayNameOrNameOfKernelConnection(
kernelConnection.kind === 'connectToLiveKernel'
? kernelConnection.kernelModel.name
: kernelConnection.kernelSpec?.name;
return displayName || name || defaultValue;

const interpeterName =
kernelConnection.kind === 'startUsingPythonInterpreter' ? kernelConnection.interpreter.displayName : undefined;

const defaultKernelName = kernelConnection.kind === 'startUsingDefaultKernel' ? 'Python 3' : undefined;
return displayName || name || interpeterName || defaultKernelName || defaultValue;
}

export function getNameOfKernelConnection(
Expand Down
3 changes: 1 addition & 2 deletions src/datascience-ui/history-react/interactivePanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ ${buildSettingsCss(this.props.settings)}`}</style>
}

private renderKernelSelection() {
if (this.props.kernel.localizedUri === getLocString('DataScience.localJupyterServer', 'local')) {
if (this.props.kernel.serverName === getLocString('DataScience.localJupyterServer', 'local')) {
return;
}

Expand All @@ -235,7 +235,6 @@ ${buildSettingsCss(this.props.settings)}`}</style>
selectServer={this.props.selectServer}
selectKernel={this.props.selectKernel}
shouldShowTrustMessage={false}
settings={this.props.settings}
/>
);
}
Expand Down
42 changes: 4 additions & 38 deletions src/datascience-ui/interactive-common/jupyterInfo.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
'use strict';
import { isEmpty, isNil } from 'lodash';
import * as React from 'react';
import { IDataScienceExtraSettings } from '../../client/datascience/types';
import { Image, ImageName } from '../react-common/image';
import { getLocString } from '../react-common/locReactSide';
import { IFont, IServerState, ServerStatus } from './mainState';
Expand All @@ -16,7 +14,6 @@ export interface IJupyterInfoProps {
kernel: IServerState;
isNotebookTrusted?: boolean;
shouldShowTrustMessage: boolean;
settings?: IDataScienceExtraSettings | undefined;
selectServer(): void;
launchNotebookTrustPrompt?(): void; // Native editor-specific
selectKernel(): void;
Expand All @@ -37,17 +34,10 @@ export class JupyterInfo extends React.Component<IJupyterInfoProps> {
}

public render() {
let jupyterServerDisplayName: string = this.props.kernel.localizedUri;
if (!isNil(this.props.settings) && isEmpty(jupyterServerDisplayName)) {
const jupyterServerUriSetting: string = this.props.settings.jupyterServerURI;
Copy link
Author

Choose a reason for hiding this comment

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

This code was VERY specific to AML so I removed it. It should all be handled by the displayName that AML gives us.

if (!isEmpty(jupyterServerUriSetting) && this.isUriOfComputeInstance(jupyterServerUriSetting)) {
jupyterServerDisplayName = this.getComputeInstanceNameFromId(jupyterServerUriSetting);
}
}

const jupyterServerDisplayName: string = this.props.kernel.serverName;
const serverTextSize =
getLocString('DataScience.jupyterServer', 'Jupyter Server').length + jupyterServerDisplayName.length + 4; // plus 4 for the icon
const displayNameTextSize = this.props.kernel.displayName.length + this.props.kernel.jupyterServerStatus.length;
const displayNameTextSize = this.props.kernel.kernelName.length + this.props.kernel.jupyterServerStatus.length;
const dynamicFont: React.CSSProperties = {
fontSize: 'var(--vscode-font-size)', // Use the same font and size as the menu
fontFamily: 'var(--vscode-font-family)',
Expand Down Expand Up @@ -98,11 +88,11 @@ export class JupyterInfo extends React.Component<IJupyterInfoProps> {
role="button"
aria-disabled={ariaDisabled}
>
{this.props.kernel.displayName}: {this.props.kernel.jupyterServerStatus}
{this.props.kernel.kernelName}: {this.props.kernel.jupyterServerStatus}
</div>
);
} else {
const displayName = this.props.kernel.displayName ?? getLocString('DataScience.noKernel', 'No Kernel');
const displayName = this.props.kernel.kernelName ?? getLocString('DataScience.noKernel', 'No Kernel');
return (
<div className="kernel-status-section kernel-status-status" style={displayNameTextWidth} role="button">
{displayName}: {this.props.kernel.jupyterServerStatus}
Expand Down Expand Up @@ -138,30 +128,6 @@ export class JupyterInfo extends React.Component<IJupyterInfoProps> {
: getLocString('DataScience.connected', 'Connected');
}

private isUriOfComputeInstance(uri: string): boolean {
try {
const parsedUrl: URL = new URL(uri);
return parsedUrl.searchParams.get('id') === 'azureml_compute_instances';
} catch (e) {
return false;
}
}

private getComputeInstanceNameFromId(id: string | undefined): string {
if (isNil(id)) {
return '';
}

const res: string[] | null = id.match(
/\/providers\/Microsoft.MachineLearningServices\/workspaces\/[^\/]+\/computes\/([^\/]+)(\/)?/
);
if (isNil(res) || res.length < 2) {
return '';
}

return res[1];
}

private selectServer(): void {
this.props.selectServer();
}
Expand Down
8 changes: 4 additions & 4 deletions src/datascience-ui/interactive-common/mainState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ export interface IFont {

export interface IServerState {
jupyterServerStatus: ServerStatus;
localizedUri: string;
displayName: string;
serverName: string;
kernelName: string;
language: string;
}

Expand Down Expand Up @@ -192,8 +192,8 @@ export function generateTestState(filePath: string = '', editable: boolean = fal
loaded: false,
testMode: true,
kernel: {
localizedUri: 'No Kernel',
displayName: 'Python',
serverName: '',
kernelName: 'Python',
jupyterServerStatus: ServerStatus.NotStarted,
language: PYTHON_LANGUAGE
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ export namespace Kernel {
return {
...arg.prevState,
kernel: {
localizedUri: arg.payload.data.localizedUri,
serverName: arg.payload.data.serverName,
jupyterServerStatus: arg.payload.data.jupyterServerStatus,
displayName: arg.payload.data.displayName,
kernelName: arg.payload.data.kernelName,
language: arg.payload.data.language
}
};
Expand Down
4 changes: 2 additions & 2 deletions src/datascience-ui/interactive-common/redux/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ function generateDefaultState(
monacoReady: testMode, // When testing, monaco starts out ready
loaded: false,
kernel: {
displayName: getLocString('DataScience.noKernel', 'No Kernel'),
localizedUri: getLocString('DataScience.serverNotStarted', 'Not Started'),
kernelName: getLocString('DataScience.noKernel', 'No Kernel'),
serverName: getLocString('DataScience.serverNotStarted', 'Not Started'),
jupyterServerStatus: ServerStatus.NotStarted,
language: PYTHON_LANGUAGE
},
Expand Down
1 change: 0 additions & 1 deletion src/datascience-ui/native-editor/toolbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,6 @@ export class Toolbar extends React.PureComponent<INativeEditorToolbarProps> {
shouldShowTrustMessage={true}
isNotebookTrusted={this.props.isNotebookTrusted}
launchNotebookTrustPrompt={launchNotebookTrustPrompt}
settings={this.props.settings}
/>
</div>
<div className="toolbar-divider" />
Expand Down
4 changes: 2 additions & 2 deletions src/test/datascience/interactivePanel.functional.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@ suite('DataScience Interactive Panel', () => {
interruptKernel: noopAny,
isAtBottom: false,
kernel: {
displayName: '',
kernelName: '',
jupyterServerStatus: ServerStatus.Busy,
localizedUri: '',
serverName: '',
language: PYTHON_LANGUAGE
},
knownDark: false,
Expand Down
9 changes: 8 additions & 1 deletion src/test/datascience/nativeEditor.functional.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ import {
typeCode,
verifyCellIndex,
verifyCellSource,
verifyHtmlOnCell
verifyHtmlOnCell,
verifyServerStatus
} from './testHelpers';
import { ITestNativeEditorProvider } from './testNativeEditorProvider';

Expand Down Expand Up @@ -713,6 +714,8 @@ df.head()`;

// Make sure it has a server
assert.ok(editor.editor.notebook, 'Notebook did not start with a server');
// Make sure it does have a name though
verifyServerStatus(editor.mount.wrapper, 'local');
} else {
context.skip();
}
Expand All @@ -721,6 +724,7 @@ df.head()`;
runMountedTest('Server load skipped', async (context) => {
if (ioc.mockJupyter) {
ioc.getSettings().datascience.disableJupyterAutoStart = true;
ioc.getSettings().datascience.jupyterServerURI = 'https://remotetest';
await ioc.activate();

// Create an editor so something is listening to messages
Expand All @@ -731,6 +735,9 @@ df.head()`;

// Make sure it does not have a server
assert.notOk(editor.editor.notebook, 'Notebook should not start with a server');

// Make sure it does have a name though
verifyServerStatus(editor.mount.wrapper, 'Not Started');
} else {
context.skip();
}
Expand Down
8 changes: 4 additions & 4 deletions src/test/datascience/nativeEditor.toolbar.functional.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ suite('DataScience Native Toolbar', () => {
font: { family: '', size: 1 },
interruptKernel: sinon.stub(),
kernel: {
displayName: '',
kernelName: '',
jupyterServerStatus: ServerStatus.Busy,
localizedUri: '',
serverName: '',
language: PYTHON_LANGUAGE
},
restartKernel: sinon.stub(),
Expand Down Expand Up @@ -245,9 +245,9 @@ suite('DataScience Native Toolbar', () => {
font: { family: '', size: 1 },
interruptKernel: sinon.stub(),
kernel: {
displayName: '',
kernelName: '',
jupyterServerStatus: ServerStatus.Busy,
localizedUri: '',
serverName: '',
language: PYTHON_LANGUAGE
},
restartKernel: sinon.stub(),
Expand Down
9 changes: 9 additions & 0 deletions src/test/datascience/testHelpers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,15 @@ export function verifyCellSource(
assert.deepStrictEqual(inst.state.model?.getValue(), source, 'Source does not match on cell');
}

export function verifyServerStatus(wrapper: ReactWrapper<any, Readonly<{}>, React.Component>, statusText: string) {
wrapper.update();

const foundResult = wrapper.find('div.kernel-status-server');
assert.ok(foundResult.length >= 1, "Didn't find server status");
const html = foundResult.html();
assert.ok(html.includes(statusText), `${statusText} not found in server status`);
}

export function verifyHtmlOnCell(
wrapper: ReactWrapper<any, Readonly<{}>, React.Component>,
cellType: 'NativeCell' | 'InteractiveCell',
Expand Down