Skip to content

Commit

Permalink
Warn when files could override Python modules (#10787)
Browse files Browse the repository at this point in the history
* Misc

* Fixes

* Changes

* misc

* Add tests

* Fixes

* Misc

* Fix tests

* Misc

* Remove console logging

* More fixes

* Added tests

* Misc

* Move files

* More tests

* Misc

* Address code review comments
  • Loading branch information
DonJayamanne authored Jul 14, 2022
1 parent 31c561d commit 4d1a5c7
Show file tree
Hide file tree
Showing 45 changed files with 1,950 additions and 148 deletions.
2 changes: 2 additions & 0 deletions news/1 Enhancements/7538.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Warn users when a Python file could override an existing Python package (there by interfering with the kernels. [More info](https://aka.ms/JupyterKernelStartFailureOverrideReservedName)).
This feature could be turned off via the setting `"jupyter.diagnostics.reservedPythonNames.enabled": false`.
26 changes: 26 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2033,6 +2033,32 @@
"default": false,
"markdownDescription": "%jupyter.configuration.jupyter.enableExtendedKernelCompletions.markdownDescription%",
"scope": "machine"
},
"jupyter.diagnostics.reservedPythonNames.enabled": {
"type": "boolean",
"default": true,
"markdownDescription": "%jupyter.configuration.jupyter.diagnostics.reservedPythonNames.enabled.markdownDescription%",
"scope": "machine",
"tags": [
"diagnostics"
]
},
"jupyter.diagnostics.reservedPythonNames.exclude": {
"type": "array",
"default": [
"**/site-packages/**",
"**/lib/python/**",
"**/lib64/python/**"
],
"items": {
"type": "string"
},
"uniqueItems": true,
"markdownDescription": "%jupyter.configuration.jupyter.diagnostics.reservedPythonNames.exclude.markdownDescription%",
"scope": "machine",
"tags": [
"diagnostics"
]
}
}
},
Expand Down
15 changes: 14 additions & 1 deletion package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -852,6 +852,10 @@
"message": "The file '{0}' seems to be overriding built in modules and interfering with the startup of the kernel. Consider renaming the file and starting the kernel again.",
"comment": ["{Locked='kernel'}"]
},
"DataScience.DataScience.moduleSeemsToBeInterferingWithKernelStartup": {
"message": "The module '{0}' seems to be overriding built in modules and interfering with the startup of the kernel. Consider renaming the folder and starting the kernel again.",
"comment": ["{Locked='kernel'}"]
},
"DataScience.failedToStartKernelDueToMissingModule": {
"message": "The kernel failed to start due to the missing module '{0}'. Consider installing this module.",
"comment": ["{Locked='kernel'}"]
Expand Down Expand Up @@ -1106,5 +1110,14 @@
"DataScience.switchToLocalKernelsTitle": "Connect to Local Kernels",
"DataScience.switchToRemoteKernelsTitle": "Connect to Your Own Jupyter Server",
"DataScience.switchToAnotherRemoteKernelsTitle": "Connect to Another Jupyter Server",
"DataScience.failedToInstallPythonExtension": "Failed to install the Python Extension."
"DataScience.failedToInstallPythonExtension": "Failed to install the Python Extension.",
"DataScience.pythonFileOverridesPythonPackage": "This file could potentially override an existing Python package and interfere with kernel execution, consider renaming it.",
"DataScience.alwaysIgnoreWarningsAboutOverridingPythonPackages": "Always ignore warnings about overriding Python packages",
"DataScience.ignoreWarningAboutOverridingPythonPackage": "Ignore warning for '{0}'",
"DataScience.moreInfoAboutFileNamesOverridingPythonPackages": "More information on overriding Python packages",
"DataScience.reservedPythonFileNamesDiagnosticCollectionName": "Reserved Python Filenames",
"DataScience.filesPossiblyOverridingPythonModulesMayHavePreventedKernelFromStarting": "Some of the following files found in the working directory may have prevented the Kernel from starting. Consider renaming them.",
"DataScience.listOfFilesWithLinksThatMightNeedToBeRenamed": "File(s): {0} might need to be renamed.",
"jupyter.configuration.jupyter.diagnostics.reservedPythonNames.enabled.markdownDescription": "Whether to show warnings about Python file names that could potentially [override Python packages](https://aka.ms/JupyterKernelStartFailureOverrideReservedName).",
"jupyter.configuration.jupyter.diagnostics.reservedPythonNames.exclude.markdownDescription": "Configure [glob patterns](https://code.visualstudio.com/docs/editor/codebasics#_advanced-search-options) for excluding files and folders from from being warned about [overriding Python packages](https://aka.ms/JupyterKernelStartFailureOverrideReservedName)."
}
7 changes: 4 additions & 3 deletions src/extension.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ import { registerTypes as registerKernelTypes } from './kernels/serviceRegistry.
import { registerTypes as registerNotebookTypes } from './notebooks/serviceRegistry.node';
import { registerTypes as registerInteractiveTypes } from './interactive-window/serviceRegistry.node';
import { registerTypes as registerStandaloneTypes } from './standalone/serviceRegistry.node';
import { registerTypes as registerTelemetryTypes } from './platform/telemetry/serviceRegistry.node';
import { registerTypes as registerWebviewTypes } from './webviews/extension-side/serviceRegistry.node';
import { IExtensionActivationManager } from './platform/activation/types';
import {
Expand All @@ -93,6 +92,7 @@ import { ConsoleLogger } from './platform/logging/consoleLogger';
import { FileLogger } from './platform/logging/fileLogger.node';
import { createWriteStream } from 'fs-extra';
import { initializeGlobals as initializeTelemetryGlobals } from './platform/telemetry/telemetry';
import { IInterpreterPackages } from './platform/interpreter/types';

durations.codeLoadingTime = stopWatch.elapsedTime;

Expand Down Expand Up @@ -169,7 +169,9 @@ async function activateUnsafe(

const [serviceManager, serviceContainer] = initializeGlobals(context);
activatedServiceContainer = serviceContainer;
initializeTelemetryGlobals(serviceContainer);
initializeTelemetryGlobals((interpreter) =>
serviceContainer.get<IInterpreterPackages>(IInterpreterPackages).getPackageVersions(interpreter)
);
const activationPromise = activateComponents(context, serviceManager, serviceContainer);

//===============================================
Expand Down Expand Up @@ -317,7 +319,6 @@ async function activateLegacy(

// Register the rest of the types (platform is first because it's needed by others)
registerPlatformTypes(serviceManager);
registerTelemetryTypes(serviceManager);
registerKernelTypes(serviceManager, isDevMode);
registerNotebookTypes(serviceManager);
registerInteractiveTypes(serviceManager);
Expand Down
4 changes: 1 addition & 3 deletions src/extension.web.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ import { IServiceContainer, IServiceManager } from './platform/ioc/types';
import { sendErrorTelemetry, sendStartupTelemetry } from './platform/telemetry/startupTelemetry';
import { noop } from './platform/common/utils/misc';
import { registerTypes as registerPlatformTypes } from './platform/serviceRegistry.web';
import { registerTypes as registerTelemetryTypes } from './platform/telemetry/serviceRegistry.web';
import { registerTypes as registerKernelTypes } from './kernels/serviceRegistry.web';
import { registerTypes as registerNotebookTypes } from './notebooks/serviceRegistry.web';
import { registerTypes as registerInteractiveTypes } from './interactive-window/serviceRegistry.web';
Expand Down Expand Up @@ -162,7 +161,7 @@ async function activateUnsafe(

const [serviceManager, serviceContainer] = initializeGlobals(context);
activatedServiceContainer = serviceContainer;
initializeTelemetryGlobals(serviceContainer);
initializeTelemetryGlobals(() => Promise.resolve(new Map()));
const activationPromise = activateComponents(context, serviceManager, serviceContainer);

//===============================================
Expand Down Expand Up @@ -287,7 +286,6 @@ async function activateLegacy(

// Register the rest of the types (platform is first because it's needed by others)
registerPlatformTypes(serviceManager);
registerTelemetryTypes(serviceManager);
registerNotebookTypes(serviceManager);
registerKernelTypes(serviceManager, isDevMode);
registerInteractiveTypes(serviceManager);
Expand Down
4 changes: 2 additions & 2 deletions src/interactive-window/interactiveWindow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
}

private async finishSysInfoWithFailureMessage(error: Error, cellPromise: Promise<NotebookCell>) {
let message = await this.errorHandler.getErrorMessageForDisplayInCell(error, 'start');
let message = await this.errorHandler.getErrorMessageForDisplayInCell(error, 'start', this.owningResource);
// As message is displayed in markdown, ensure linebreaks are formatted accordingly.
message = message.split('\n').join(' \n');
this.updateSysInfoMessage(message, true, cellPromise);
Expand Down Expand Up @@ -595,7 +595,7 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
.then((cell) =>
// If our cell result was a failure show an error
this.errorHandler
.getErrorMessageForDisplayInCell(ex, 'execution')
.getErrorMessageForDisplayInCell(ex, 'execution', this.owningResource)
.then((message) => this.addErrorMessage(message, cell))
)
.catch(noop);
Expand Down
94 changes: 94 additions & 0 deletions src/kernels/errors/kernelErrorHandler.node.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
import { inject, injectable, optional } from 'inversify';
import { Uri } from 'vscode';
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../platform/common/application/types';
import {
IBrowserService,
IConfigurationService,
IExtensions,
IsWebExtension,
Resource
} from '../../platform/common/types';
import { DataScience, Common } from '../../platform/common/utils/localize';
import { IKernelDependencyService } from '../types';
import { IJupyterInterpreterDependencyManager, IJupyterServerUriStorage } from '../jupyter/types';
import * as path from '../../platform/vscode-path/resources';
import { IReservedPythonNamedProvider } from '../../platform/interpreter/types';
import { JupyterKernelStartFailureOverrideReservedName } from '../../platform/interpreter/constants';
import { DataScienceErrorHandler } from './kernelErrorHandler';
import { getDisplayPath } from '../../platform/common/platform/fs-paths';

@injectable()
export class DataScienceErrorHandlerNode extends DataScienceErrorHandler {
constructor(
@inject(IApplicationShell) applicationShell: IApplicationShell,
@inject(IJupyterInterpreterDependencyManager)
@optional()
dependencyManager: IJupyterInterpreterDependencyManager | undefined,
@inject(IBrowserService) browser: IBrowserService,
@inject(IConfigurationService) configuration: IConfigurationService,
@inject(IKernelDependencyService)
@optional()
kernelDependency: IKernelDependencyService | undefined,
@inject(IWorkspaceService) workspaceService: IWorkspaceService,
@inject(IJupyterServerUriStorage) serverUriStorage: IJupyterServerUriStorage,
@inject(ICommandManager) commandManager: ICommandManager,
@inject(IsWebExtension) isWebExtension: boolean,
@inject(IExtensions) extensions: IExtensions,
@inject(IReservedPythonNamedProvider) private readonly reservedPythonNames: IReservedPythonNamedProvider
) {
super(
applicationShell,
dependencyManager,
browser,
configuration,
kernelDependency,
workspaceService,
serverUriStorage,
commandManager,
isWebExtension,
extensions
);
}
protected override async addErrorMessageIfPythonArePossiblyOverridingPythonModules(
messages: string[],
resource: Resource
) {
// Looks like some other module is missing.
// Sometimes when you create files like xml.py, then kernel startup fails due to xml.dom module not being found.
const problematicFiles = await this.getFilesInWorkingDirectoryThatCouldPotentiallyOverridePythonModules(
resource
);
if (problematicFiles.length > 0) {
const cwd = resource ? path.dirname(resource) : undefined;
const fileLinks = problematicFiles.map((item) => {
if (item.type === 'file') {
const displayPath = resource ? getDisplayPath(item.uri, [], cwd) : path.basename(item.uri);
return `<a href='${item.uri.toString()}?line=1'>${displayPath}</a>`;
} else {
const displayPath = resource
? getDisplayPath(item.uri, [], cwd)
: `${path.basename(path.dirname(item.uri))}/__init__.py`;
return `<a href='${item.uri.toString()}?line=1'>${displayPath}</a>`;
}
});
let files = '';
if (fileLinks.length === 1) {
files = fileLinks[0];
} else {
files = `${fileLinks.slice(0, -1).join(', ')} ${Common.and()} ${fileLinks.slice(-1)}`;
}
messages.push(
DataScience.filesPossiblyOverridingPythonModulesMayHavePreventedKernelFromStarting().format(files)
);
messages.push(DataScience.listOfFilesWithLinksThatMightNeedToBeRenamed().format(files));
messages.push(Common.clickHereForMoreInfoWithHtml().format(JupyterKernelStartFailureOverrideReservedName));
}
}
protected override async getFilesInWorkingDirectoryThatCouldPotentiallyOverridePythonModules(
resource: Resource
): Promise<{ uri: Uri; type: 'file' | '__init__' }[]> {
return resource ? this.reservedPythonNames.getUriOverridingReservedPythonNames(path.dirname(resource)) : [];
}
}
Loading

0 comments on commit 4d1a5c7

Please sign in to comment.