Skip to content

Commit 9f8db9c

Browse files
DonJayamanneDonJayamanne
authored
Better logs of failures when installing packages (microsoft#15934)
* Better logging of failures * Fix tests
1 parent efc3a4a commit 9f8db9c

9 files changed

Lines changed: 142 additions & 107 deletions

File tree

‎news/3 Code Health/15933.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Better logging (telemetry) when installation of Python packages fail.

‎src/client/common/installer/moduleInstaller.ts‎

Lines changed: 66 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,10 @@ import { STANDARD_OUTPUT_CHANNEL } from '../constants';
1515
import { IFileSystem } from '../platform/types';
1616
import * as internalPython from '../process/internal/python';
1717
import { ITerminalServiceFactory, TerminalCreationOptions } from '../terminal/types';
18-
import { ExecutionInfo, IConfigurationService, IOutputChannel } from '../types';
18+
import { ExecutionInfo, IConfigurationService, IOutputChannel, ModuleNamePurpose, Product } from '../types';
1919
import { Products } from '../utils/localize';
2020
import { isResource } from '../utils/misc';
21+
import { ProductNames } from './productNames';
2122
import { IModuleInstaller, InterpreterUri } from './types';
2223

2324
@injectable()
@@ -29,12 +30,17 @@ export abstract class ModuleInstaller implements IModuleInstaller {
2930
constructor(protected serviceContainer: IServiceContainer) {}
3031

3132
public async installModule(
32-
name: string,
33+
productOrModuleName: Product | string,
3334
resource?: InterpreterUri,
3435
cancel?: CancellationToken,
3536
isUpgrade?: boolean,
3637
): Promise<void> {
37-
sendTelemetryEvent(EventName.PYTHON_INSTALL_PACKAGE, undefined, { installer: this.displayName });
38+
const name =
39+
typeof productOrModuleName == 'string'
40+
? productOrModuleName
41+
: translateProductToModule(productOrModuleName, ModuleNamePurpose.install);
42+
const productName = typeof productOrModuleName === 'string' ? name : ProductNames.get(productOrModuleName);
43+
sendTelemetryEvent(EventName.PYTHON_INSTALL_PACKAGE, undefined, { installer: this.displayName, productName });
3844
const uri = isResource(resource) ? resource : undefined;
3945
const options: TerminalCreationOptions = {};
4046
if (isResource(resource)) {
@@ -145,3 +151,60 @@ export abstract class ModuleInstaller implements IModuleInstaller {
145151
return args;
146152
}
147153
}
154+
155+
export function translateProductToModule(product: Product, purpose: ModuleNamePurpose): string {
156+
switch (product) {
157+
case Product.mypy:
158+
return 'mypy';
159+
case Product.nosetest: {
160+
return purpose === ModuleNamePurpose.install ? 'nose' : 'nosetests';
161+
}
162+
case Product.pylama:
163+
return 'pylama';
164+
case Product.prospector:
165+
return 'prospector';
166+
case Product.pylint:
167+
return 'pylint';
168+
case Product.pytest:
169+
return 'pytest';
170+
case Product.autopep8:
171+
return 'autopep8';
172+
case Product.black:
173+
return 'black';
174+
case Product.pycodestyle:
175+
return 'pycodestyle';
176+
case Product.pydocstyle:
177+
return 'pydocstyle';
178+
case Product.yapf:
179+
return 'yapf';
180+
case Product.flake8:
181+
return 'flake8';
182+
case Product.unittest:
183+
return 'unittest';
184+
case Product.rope:
185+
return 'rope';
186+
case Product.bandit:
187+
return 'bandit';
188+
case Product.jupyter:
189+
return 'jupyter';
190+
case Product.notebook:
191+
return 'notebook';
192+
case Product.pandas:
193+
return 'pandas';
194+
case Product.ipykernel:
195+
return 'ipykernel';
196+
case Product.nbconvert:
197+
return 'nbconvert';
198+
case Product.kernelspec:
199+
return 'kernelspec';
200+
case Product.tensorboard:
201+
return 'tensorboard';
202+
case Product.torchProfilerInstallName:
203+
return 'torch-tb-profiler';
204+
case Product.torchProfilerImportName:
205+
return 'torch_tb_profiler';
206+
default: {
207+
throw new Error(`Product ${product} cannot be installed as a Python Module.`);
208+
}
209+
}
210+
}

‎src/client/common/installer/productInstaller.ts‎

Lines changed: 32 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import '../extensions';
88
import { IInterpreterService } from '../../interpreter/contracts';
99
import { IServiceContainer } from '../../ioc/types';
1010
import { EnvironmentType, PythonEnvironment } from '../../pythonEnvironments/info';
11+
import { sendTelemetryEvent } from '../../telemetry';
12+
import { EventName } from '../../telemetry/constants';
1113
import { IApplicationShell, IWorkspaceService } from '../application/types';
1214
import { STANDARD_OUTPUT_CHANNEL } from '../constants';
1315
import { traceError, traceInfo } from '../logger';
@@ -26,6 +28,7 @@ import {
2628
} from '../types';
2729
import { Installer } from '../utils/localize';
2830
import { isResource, noop } from '../utils/misc';
31+
import { translateProductToModule } from './moduleInstaller';
2932
import { ProductNames } from './productNames';
3033
import {
3134
IInstallationChannelManager,
@@ -101,17 +104,25 @@ abstract class BaseInstaller {
101104
const channels = this.serviceContainer.get<IInstallationChannelManager>(IInstallationChannelManager);
102105
const installer = await channels.getInstallationChannel(product, resource);
103106
if (!installer) {
107+
sendTelemetryEvent(EventName.PYTHON_INSTALL_PACKAGE, undefined, {
108+
installer: 'unavailable',
109+
productName: ProductNames.get(product),
110+
});
104111
return InstallerResponse.Ignore;
105112
}
106113

107-
const moduleName = translateProductToModule(product, ModuleNamePurpose.install);
108114
await installer
109-
.installModule(moduleName, resource, cancel, isUpgrade)
110-
.catch((ex) => traceError(`Error in installing the module '${moduleName}', ${ex}`));
111-
112-
return this.isInstalled(product, resource).then((isInstalled) =>
113-
isInstalled ? InstallerResponse.Installed : InstallerResponse.Ignore,
114-
);
115+
.installModule(product, resource, cancel, isUpgrade)
116+
.catch((ex) => traceError(`Error in installing the product '${ProductNames.get(product)}', ${ex}`));
117+
118+
return this.isInstalled(product, resource).then((isInstalled) => {
119+
sendTelemetryEvent(EventName.PYTHON_INSTALL_PACKAGE, undefined, {
120+
installer: installer.displayName,
121+
productName: ProductNames.get(product),
122+
isInstalled,
123+
});
124+
return isInstalled ? InstallerResponse.Installed : InstallerResponse.Ignore;
125+
});
115126
}
116127

117128
/**
@@ -355,7 +366,7 @@ class DataScienceInstaller extends BaseInstaller {
355366

356367
// Pick an installerModule based on whether the interpreter is conda or not. Default is pip.
357368
const moduleName = translateProductToModule(product, ModuleNamePurpose.install);
358-
let installerModule;
369+
let installerModule: IModuleInstaller | undefined;
359370
const isAvailableThroughConda = !UnsupportedChannelsForProduct.get(product)?.has(EnvironmentType.Conda);
360371
if (interpreter.envType === EnvironmentType.Conda && isAvailableThroughConda) {
361372
installerModule = channels.find((v) => v.name === EnvironmentType.Conda);
@@ -371,16 +382,25 @@ class DataScienceInstaller extends BaseInstaller {
371382

372383
if (!installerModule) {
373384
this.appShell.showErrorMessage(Installer.couldNotInstallLibrary().format(moduleName)).then(noop, noop);
385+
sendTelemetryEvent(EventName.PYTHON_INSTALL_PACKAGE, undefined, {
386+
installer: 'unavailable',
387+
productName: ProductNames.get(product),
388+
});
374389
return InstallerResponse.Ignore;
375390
}
376391

377392
await installerModule
378-
.installModule(moduleName, interpreter, cancel, isUpgrade)
393+
.installModule(product, interpreter, cancel, isUpgrade)
379394
.catch((ex) => traceError(`Error in installing the module '${moduleName}', ${ex}`));
380395

381-
return this.isInstalled(product, interpreter).then((isInstalled) =>
382-
isInstalled ? InstallerResponse.Installed : InstallerResponse.Ignore,
383-
);
396+
return this.isInstalled(product, interpreter).then((isInstalled) => {
397+
sendTelemetryEvent(EventName.PYTHON_INSTALL_PACKAGE, undefined, {
398+
installer: installerModule?.displayName || '',
399+
isInstalled,
400+
productName: ProductNames.get(product),
401+
});
402+
return isInstalled ? InstallerResponse.Installed : InstallerResponse.Ignore;
403+
});
384404
}
385405

386406
/**
@@ -483,60 +503,3 @@ export class ProductInstaller implements IInstaller {
483503
throw new Error(`Unknown product ${product}`);
484504
}
485505
}
486-
487-
function translateProductToModule(product: Product, purpose: ModuleNamePurpose): string {
488-
switch (product) {
489-
case Product.mypy:
490-
return 'mypy';
491-
case Product.nosetest: {
492-
return purpose === ModuleNamePurpose.install ? 'nose' : 'nosetests';
493-
}
494-
case Product.pylama:
495-
return 'pylama';
496-
case Product.prospector:
497-
return 'prospector';
498-
case Product.pylint:
499-
return 'pylint';
500-
case Product.pytest:
501-
return 'pytest';
502-
case Product.autopep8:
503-
return 'autopep8';
504-
case Product.black:
505-
return 'black';
506-
case Product.pycodestyle:
507-
return 'pycodestyle';
508-
case Product.pydocstyle:
509-
return 'pydocstyle';
510-
case Product.yapf:
511-
return 'yapf';
512-
case Product.flake8:
513-
return 'flake8';
514-
case Product.unittest:
515-
return 'unittest';
516-
case Product.rope:
517-
return 'rope';
518-
case Product.bandit:
519-
return 'bandit';
520-
case Product.jupyter:
521-
return 'jupyter';
522-
case Product.notebook:
523-
return 'notebook';
524-
case Product.pandas:
525-
return 'pandas';
526-
case Product.ipykernel:
527-
return 'ipykernel';
528-
case Product.nbconvert:
529-
return 'nbconvert';
530-
case Product.kernelspec:
531-
return 'kernelspec';
532-
case Product.tensorboard:
533-
return 'tensorboard';
534-
case Product.torchProfilerInstallName:
535-
return 'torch-tb-profiler';
536-
case Product.torchProfilerImportName:
537-
return 'torch_tb_profiler';
538-
default: {
539-
throw new Error(`Product ${product} cannot be installed as a Python Module.`);
540-
}
541-
}
542-
}

‎src/client/common/installer/types.ts‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,24 @@ export interface IModuleInstaller {
2424
* @memberof IModuleInstaller
2525
*/
2626
installModule(
27-
name: string,
27+
product: string,
28+
resource?: InterpreterUri,
29+
cancel?: CancellationToken,
30+
isUpgrade?: boolean,
31+
): Promise<void>;
32+
/**
33+
* Installs a Product
34+
* If a cancellation token is provided, then a cancellable progress message is dispalyed.
35+
* At this point, this method would resolve only after the module has been successfully installed.
36+
* If cancellation token is not provided, its not guaranteed that module installation has completed.
37+
* @param {string} name
38+
* @param {InterpreterUri} [resource]
39+
* @param {CancellationToken} [cancel]
40+
* @returns {Promise<void>}
41+
* @memberof IModuleInstaller
42+
*/
43+
installModule(
44+
product: Product,
2845
resource?: InterpreterUri,
2946
cancel?: CancellationToken,
3047
isUpgrade?: boolean,

‎src/client/telemetry/index.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -803,10 +803,17 @@ export interface IEventNamePropertyMapping {
803803
[EventName.PYTHON_INSTALL_PACKAGE]: {
804804
/**
805805
* The name of the module. (pipenv, Conda etc.)
806-
*
807-
* @type {string}
806+
* One of the possible values includes `unavailable`, meaning user doesn't have pip, conda, or other tools available that can be used to install a python package.
808807
*/
809808
installer: string;
809+
/**
810+
* Name of the corresponding product (package) to be installed.
811+
*/
812+
productName?: string;
813+
/**
814+
* Whether the product (package) has been installed or not.
815+
*/
816+
isInstalled?: boolean;
810817
};
811818
/**
812819
* Telemetry sent with details immediately after linting a document completes

‎src/test/common/installer.test.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -350,9 +350,8 @@ suite('Installer', () => {
350350
const moduleInstallers = ioc.serviceContainer.getAll<MockModuleInstaller>(IModuleInstaller);
351351
const moduleInstallerOne = moduleInstallers.find((item) => item.displayName === 'two')!;
352352

353-
moduleInstallerOne.on('installModule', (moduleName) => {
354-
const installName = installer.translateProductToModuleName(product, ModuleNamePurpose.install);
355-
if (installName === moduleName) {
353+
moduleInstallerOne.on('installModule', (name: Product | string) => {
354+
if (product === name) {
356355
checkInstalledDef.resolve();
357356
}
358357
});

‎src/test/common/installer/installer.unit.test.ts‎

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ import {
4242
IOutputChannel,
4343
IPersistentState,
4444
IPersistentStateFactory,
45-
ModuleNamePurpose,
4645
Product,
4746
ProductType,
4847
} from '../../../client/common/types';
@@ -525,15 +524,10 @@ suite('Module Installer only', () => {
525524
test(`Ensure resource info is passed into the module installer ${product.name} (${
526525
resource ? 'With a resource' : 'without a resource'
527526
})`, async () => {
528-
const moduleName = installer.translateProductToModuleName(
529-
product.value,
530-
ModuleNamePurpose.install,
531-
);
532-
533527
moduleInstaller
534528
.setup((m) =>
535529
m.installModule(
536-
TypeMoq.It.isValue(moduleName),
530+
TypeMoq.It.isValue(product.value),
537531
TypeMoq.It.isValue(resource),
538532
TypeMoq.It.isValue(undefined),
539533
),
@@ -546,7 +540,7 @@ suite('Module Installer only', () => {
546540
moduleInstaller.verify(
547541
(m) =>
548542
m.installModule(
549-
TypeMoq.It.isValue(moduleName),
543+
TypeMoq.It.isValue(product.value),
550544
TypeMoq.It.isValue(resource),
551545
TypeMoq.It.isValue(undefined),
552546
),
@@ -558,15 +552,10 @@ suite('Module Installer only', () => {
558552
test(`Return InstallerResponse.Ignore for the module installer ${product.name} (${
559553
resource ? 'With a resource' : 'without a resource'
560554
}) if installation channel is not defined`, async () => {
561-
const moduleName = installer.translateProductToModuleName(
562-
product.value,
563-
ModuleNamePurpose.install,
564-
);
565-
566555
moduleInstaller
567556
.setup((m) =>
568557
m.installModule(
569-
TypeMoq.It.isValue(moduleName),
558+
TypeMoq.It.isValue(product.value),
570559
TypeMoq.It.isValue(resource),
571560
TypeMoq.It.isValue(undefined),
572561
),
@@ -586,15 +575,10 @@ suite('Module Installer only', () => {
586575
test(`Ensure resource info is passed into the module installer (created using ProductInstaller) ${
587576
product.name
588577
} (${resource ? 'With a resource' : 'without a resource'})`, async () => {
589-
const moduleName = installer.translateProductToModuleName(
590-
product.value,
591-
ModuleNamePurpose.install,
592-
);
593-
594578
moduleInstaller
595579
.setup((m) =>
596580
m.installModule(
597-
TypeMoq.It.isValue(moduleName),
581+
TypeMoq.It.isValue(product.value),
598582
TypeMoq.It.isValue(resource),
599583
TypeMoq.It.isValue(undefined),
600584
),
@@ -607,7 +591,7 @@ suite('Module Installer only', () => {
607591
moduleInstaller.verify(
608592
(m) =>
609593
m.installModule(
610-
TypeMoq.It.isValue(moduleName),
594+
TypeMoq.It.isValue(product.value),
611595
TypeMoq.It.isValue(resource),
612596
TypeMoq.It.isValue(undefined),
613597
),

0 commit comments

Comments
 (0)