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

Direct users to the Jupyter extension when using Run in Interactive window #21072

Merged
merged 3 commits into from
Apr 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
21 changes: 21 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,11 @@
"light": "resources/light/repl.svg"
},
"title": "%python.command.python.viewOutput.title%"
},
{
"category": "Python",
"command": "python.installJupyter",
"title": "%python.command.python.installJupyter.title%"
}
],
"configuration": {
Expand Down Expand Up @@ -1712,6 +1717,18 @@
"group": "Refactor",
"title": "%python.command.python.sortImports.title%",
"when": "editorLangId == python && !notebookEditorFocused && !virtualWorkspace && shellExecutionSupported"
},
{
"submenu": "python.runFileInteractive",
"group": "Jupyter2",
"when": "editorLangId == python && !virtualWorkspace && shellExecutionSupported && !isJupyterInstalled"

Choose a reason for hiding this comment

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

Shouldn't this be enabled only when workspace is trusted?

Copy link
Author

Choose a reason for hiding this comment

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

}
],
"python.runFileInteractive": [
{
"command": "python.installJupyter",
"group": "Jupyter2",
"when": "resourceLangId == python && !virtualWorkspace && shellExecutionSupported"
}
],
"python.run": [
Expand Down Expand Up @@ -1779,6 +1796,10 @@
"id": "python.run",
"label": "%python.editor.context.submenu.runPython%",
"icon": "$(play)"
},
{
"id": "python.runFileInteractive",
"label": "%python.editor.context.submenu.runPythonInteractive%"
}
],
"viewsWelcome": [
Expand Down
2 changes: 2 additions & 0 deletions package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"python.command.python.setInterpreter.title": "Select Interpreter",
"python.command.python.clearWorkspaceInterpreter.title": "Clear Workspace Interpreter Setting",
"python.command.python.viewOutput.title": "Show Output",
"python.command.python.installJupyter.title": "Install the Jupyter extension",
"python.command.python.viewLanguageServerOutput.title": "Show Language Server Output",
"python.command.python.configureTests.title": "Configure Tests",
"python.command.testing.rerunFailedTests.title": "Rerun Failed Tests",
Expand All @@ -26,6 +27,7 @@
"python.command.python.refreshTensorBoard.title": "Refresh TensorBoard",
"python.menu.createNewFile.title": "Python File",
"python.editor.context.submenu.runPython": "Run Python",
"python.editor.context.submenu.runPythonInteractive": "Run in Interactive window",
"python.activeStateToolPath.description": "Path to the State Tool executable for ActiveState runtimes (version 0.36+).",
"python.autoComplete.extraPaths.description": "List of paths to libraries and the like that need to be imported by auto complete engine. E.g. when using Google App SDK, the paths are not in system path, hence need to be added into this list.",
"python.condaPath.description": "Path to the conda executable to use for activation (version 4.4+).",
Expand Down
1 change: 1 addition & 0 deletions src/client/common/application/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export type CommandsWithoutArgs = keyof ICommandNameWithoutArgumentTypeMapping;
*/
interface ICommandNameWithoutArgumentTypeMapping {
[Commands.InstallPythonOnMac]: [];
[Commands.InstallJupyter]: [];
[Commands.InstallPythonOnLinux]: [];
[Commands.InstallPython]: [];
[Commands.ClearWorkspaceInterpreter]: [];
Expand Down
1 change: 1 addition & 0 deletions src/client/common/application/contextKeys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ export enum ExtensionContextKey {
showInstallPythonTile = 'showInstallPythonTile',
HasFailedTests = 'hasFailedTests',
RefreshingTests = 'refreshingTests',
IsJupyterInstalled = 'isJupyterInstalled'
}
1 change: 1 addition & 0 deletions src/client/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export namespace Commands {
export const Exec_Selection_In_Django_Shell = 'python.execSelectionInDjangoShell';
export const Exec_Selection_In_Terminal = 'python.execSelectionInTerminal';
export const GetSelectedInterpreterPath = 'python.interpreterPath';
export const InstallJupyter = 'python.installJupyter';
export const InstallPython = 'python.installPython';
export const InstallPythonOnLinux = 'python.installPythonOnLinux';
export const InstallPythonOnMac = 'python.installPythonOnMac';
Expand Down
5 changes: 5 additions & 0 deletions src/client/common/serviceRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ import { IMultiStepInputFactory, MultiStepInputFactory } from './utils/multiStep
import { Random } from './utils/random';
import { ContextKeyManager } from './application/contextKeyManager';
import { CreatePythonFileCommandHandler } from './application/commands/createPythonFile';
import { RequireJupyterPrompt } from '../jupyter/requireJupyterPrompt';

export function registerTypes(serviceManager: IServiceManager): void {
serviceManager.addSingletonInstance<boolean>(IsWindows, IS_WINDOWS);
Expand All @@ -110,6 +111,10 @@ export function registerTypes(serviceManager: IServiceManager): void {
IJupyterExtensionDependencyManager,
JupyterExtensionDependencyManager,
);
serviceManager.addSingleton<IExtensionSingleActivationService>(
IExtensionSingleActivationService,
RequireJupyterPrompt,
);
serviceManager.addSingleton<IExtensionSingleActivationService>(
IExtensionSingleActivationService,
CreatePythonFileCommandHandler,
Expand Down
3 changes: 3 additions & 0 deletions src/client/common/utils/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,9 @@ export namespace LanguageService {
);
}
export namespace Interpreters {
export const requireJupyter = l10n.t(
'Running in Interactive window requires Jupyter Extension. Would you like to install it? [Learn more](https://aka.ms/pythonJupyterSupport).',
);
export const installingPython = l10n.t('Installing Python into Environment...');
export const discovering = l10n.t('Discovering Python Interpreters');
export const refreshing = l10n.t('Refreshing Python Interpreters');
Expand Down
5 changes: 4 additions & 1 deletion src/client/jupyter/jupyterIntegration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { inject, injectable, named } from 'inversify';
import { dirname } from 'path';
import { CancellationToken, Event, Extension, Memento, Uri } from 'vscode';
import type { SemVer } from 'semver';
import { IWorkspaceService } from '../common/application/types';
import { IContextKeyManager, IWorkspaceService } from '../common/application/types';
import { JUPYTER_EXTENSION_ID, PYLANCE_EXTENSION_ID } from '../common/constants';
import { InterpreterUri, ModuleInstallFlags } from '../common/installer/types';
import {
Expand All @@ -35,6 +35,7 @@ import {
import { PythonEnvironment } from '../pythonEnvironments/info';
import { IDataViewerDataProvider, IJupyterUriProvider } from './types';
import { PylanceApi } from '../activation/node/pylanceApi';
import { ExtensionContextKey } from '../common/application/contextKeys';
/**
* This allows Python extension to update Product enum without breaking Jupyter.
* I.e. we have a strict contract, else using numbers (in enums) is bound to break across products.
Expand Down Expand Up @@ -201,13 +202,15 @@ export class JupyterExtensionIntegration {
@inject(IComponentAdapter) private pyenvs: IComponentAdapter,
@inject(IWorkspaceService) private workspaceService: IWorkspaceService,
@inject(ICondaService) private readonly condaService: ICondaService,
@inject(IContextKeyManager) private readonly contextManager: IContextKeyManager,
) {}

public registerApi(jupyterExtensionApi: JupyterExtensionApi): JupyterExtensionApi | undefined {
if (!this.workspaceService.isTrusted) {
this.workspaceService.onDidGrantWorkspaceTrust(() => this.registerApi(jupyterExtensionApi));
return undefined;
}
this.contextManager.setContext(ExtensionContextKey.IsJupyterInstalled, true);
karrtikr marked this conversation as resolved.
Show resolved Hide resolved
// Forward python parts
jupyterExtensionApi.registerPythonApi({
onDidChangeInterpreter: this.interpreterService.onDidChangeInterpreter,
Expand Down
45 changes: 45 additions & 0 deletions src/client/jupyter/requireJupyterPrompt.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import { inject, injectable } from 'inversify';
import { IExtensionSingleActivationService } from '../activation/types';
import { IApplicationShell, ICommandManager } from '../common/application/types';
import { Common, Interpreters } from '../common/utils/localize';
import { Commands, JUPYTER_EXTENSION_ID } from '../common/constants';
import { IDisposable, IDisposableRegistry } from '../common/types';
import { sendTelemetryEvent } from '../telemetry';
import { EventName } from '../telemetry/constants';

@injectable()
export class RequireJupyterPrompt implements IExtensionSingleActivationService {
public readonly supportedWorkspaceTypes = { untrustedWorkspace: false, virtualWorkspace: true };

constructor(
@inject(IApplicationShell) private readonly appShell: IApplicationShell,
@inject(ICommandManager) private readonly commandManager: ICommandManager,
@inject(IDisposableRegistry) private readonly disposables: IDisposable[],
) {}

public async activate(): Promise<void> {
this.disposables.push(this.commandManager.registerCommand(Commands.InstallJupyter, () => this.showPrompt()));
}

private async showPrompt() {

Choose a reason for hiding this comment

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

Shoudln't this be a modal dialog, as this is a blocking action.
As a user I would like to run a cell, and then that cannot be done until and unless Jupyter is installed, and thats seems to be a blocking action which would require (IMHO) a modal dialog and improve the UX
In Jupyter we used to have such messages and found that Modal dialogs for such workflows was better

Copy link
Author

@karrtikr karrtikr Apr 17, 2023

Choose a reason for hiding this comment

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

The way we're showing it I think a modal dialog can be too harsh when user has already selected to "Install Jupyter".:

installjupyter

If "Run in Interactive window" was not a submenu and user clicked it and expected to run directly, I could see why we would show the modal dialog.

cc/ @luabud @karthiknadig

const prompts = [Common.bannerLabelYes, Common.bannerLabelNo];
const telemetrySelections: ['Yes', 'No'] = ['Yes', 'No'];
const selection = await this.appShell.showInformationMessage(Interpreters.requireJupyter, ...prompts);
sendTelemetryEvent(EventName.REQUIRE_JUPYTER_PROMPT, undefined, {
selection: selection ? telemetrySelections[prompts.indexOf(selection)] : undefined,
});
if (!selection) {
return;
}
if (selection === prompts[0]) {
await this.commandManager.executeCommand(
'workbench.extensions.installExtension',
JUPYTER_EXTENSION_ID,
undefined,
);
}
}
}
1 change: 1 addition & 0 deletions src/client/telemetry/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export enum EventName {
PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT = 'PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT',
PYTHON_NOT_INSTALLED_PROMPT = 'PYTHON_NOT_INSTALLED_PROMPT',
CONDA_INHERIT_ENV_PROMPT = 'CONDA_INHERIT_ENV_PROMPT',
REQUIRE_JUPYTER_PROMPT = 'REQUIRE_JUPYTER_PROMPT',
ACTIVATED_CONDA_ENV_LAUNCH = 'ACTIVATED_CONDA_ENV_LAUNCH',
ENVFILE_VARIABLE_SUBSTITUTION = 'ENVFILE_VARIABLE_SUBSTITUTION',
ENVFILE_WORKSPACE = 'ENVFILE_WORKSPACE',
Expand Down
16 changes: 16 additions & 0 deletions src/client/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1315,6 +1315,22 @@ export interface IEventNamePropertyMapping {
*/
selection: 'Allow' | 'Close' | undefined;
};
/**
* Telemetry event sent with details when user attempts to run in interactive window when Jupyter is not installed.
*/
/* __GDPR__
"conda_inherit_env_prompt" : {
"selection" : { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "karrtikr" }
}
*/
[EventName.REQUIRE_JUPYTER_PROMPT]: {
/**
* `Yes` When 'Yes' option is selected
* `No` When 'No' option is selected
* `undefined` When 'x' is selected
*/
selection: 'Yes' | 'No' | undefined;
};
/**
* Telemetry event sent with details when user clicks the prompt with the following message:
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
} from '../../../client/interpreter/contracts';
import { IInterpreterSelector } from '../../../client/interpreter/configuration/types';
import { IEnvironmentActivationService } from '../../../client/interpreter/activation/types';
import { IWorkspaceService } from '../../../client/common/application/types';
import { IContextKeyManager, IWorkspaceService } from '../../../client/common/application/types';
import { MockMemento } from '../../mocks/mementos';

suite('Pylance Language Server - Interactive Window LSP Notebooks', () => {
Expand All @@ -41,6 +41,7 @@ suite('Pylance Language Server - Interactive Window LSP Notebooks', () => {
mock<IComponentAdapter>(),
mock<IWorkspaceService>(),
mock<ICondaService>(),
mock<IContextKeyManager>(),
);
jupyterApi.registerGetNotebookUriForTextDocumentUriFunction(getNotebookUriFunction);
});
Expand Down