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

Warn when files could override Python modules #10787

Merged
merged 17 commits into from
Jul 14, 2022
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) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pass a callback instead of passing the entire interface, as we dont want the telemetry layer to have any dependencies on interpreter code.

serviceContainer.get<IInterpreterPackages>(IInterpreterPackages).getPackageVersions(interpreter)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved IInterpreterPackages from telemetry folder into Interprter folder, and passed just a callback into interpreters layer, we don't need everything, a callback is sufficient.

);
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 @@ -362,7 +362,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 @@ -543,7 +543,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