Skip to content

Remove DebugAdapterNewPtvsd and DebugAdapterDescriptorFactory experiment. #11769

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 14, 2020
Merged

Remove DebugAdapterNewPtvsd and DebugAdapterDescriptorFactory experiment. #11769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8a08b06
Remove DebugAdapterNewPtvsd and DebugAdapterDescriptorFactory experim…
karthiknadig May 12, 2020
8506f83
Fix tests
karthiknadig May 12, 2020
7c6c60e
Remove experiments from json files
karthiknadig May 14, 2020
d8198e5
Update src/client/debugger/extension/adapter/outdatedDebuggerPrompt.ts
karthiknadig May 14, 2020
2fb8a0c
Undoing changes to misc.test.ts
karthiknadig May 14, 2020
d55a32b
Merge branch 'remove-ptvsd-experiments' of github.com:karthiknadig/vs…
karthiknadig May 14, 2020
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
25 changes: 0 additions & 25 deletions experiments.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,30 +29,6 @@
"min": 0,
"max": 40
},
{
"name": "DebugAdapterFactory - control",
"salt": "DebugAdapterFactory",
"min": 0,
"max": 0
},
{
"name": "DebugAdapterFactory - experiment",
"salt": "DebugAdapterFactory",
"min": 0,
"max": 100
},
{
Copy link

@karrtikr karrtikr May 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh wait I just realized removing these from experiments.json will opt users out of experiment. @karthiknadig

Copy link

Choose a reason for hiding this comment

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

The change hasn't landed in master, so we can restore experiments.json😅

"name": "PtvsdWheels37 - control",
"salt": "DebugAdapterFactory",
"min": 0,
"max": 0
},
{
"name": "PtvsdWheels37 - experiment",
"salt": "DebugAdapterFactory",
"min": 0,
"max": 100
},
{
"name": "Reload - control",
"salt": "DebugAdapterFactory",
Expand Down Expand Up @@ -185,5 +161,4 @@
"max": 0,
"min": 0
}

]
4 changes: 0 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1592,8 +1592,6 @@
"LS - enabled",
"AlwaysDisplayTestExplorer - experiment",
"ShowExtensionSurveyPrompt - enabled",
"DebugAdapterFactory - experiment",
"PtvsdWheels37 - experiment",
"Reload - experiment",
"AA_testing - experiment",
"WebHostNotebook - experiment",
Expand All @@ -1617,8 +1615,6 @@
"LS - enabled",
"AlwaysDisplayTestExplorer - experiment",
"ShowExtensionSurveyPrompt - enabled",
"DebugAdapterFactory - experiment",
"PtvsdWheels37 - experiment",
"Reload - experiment",
"AA_testing - experiment",
"WebHostNotebook - experiment",
Expand Down
1 change: 0 additions & 1 deletion package.nls.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,6 @@
"diagnostics.consoleTypeDiagnostic": "Your launch.json file needs to be updated to change the console type string from \"none\" to \"internalConsole\", otherwise Python debugging may not work. Would you like to automatically update your launch.json file now?",
"diagnostics.justMyCodeDiagnostic": "Configuration \"debugStdLib\" in launch.json is no longer supported. It's recommended to replace it with \"justMyCode\", which is the exact opposite of using \"debugStdLib\". Would you like to automatically update your launch.json file to do that?",
"diagnostics.yesUpdateLaunch": "Yes, update launch.json",
"diagnostics.processId": "Attaching the debugger to a local process is an experimental feature. It will be available to all users soon.",
"diagnostics.invalidTestSettings": "Your settings needs to be updated to change the setting \"python.unitTest.\" to \"python.testing.\", otherwise testing Python code using the extension may not work. Would you like to automatically update your settings now?",
"DataScience.interruptKernel": "Interrupt IPython kernel",
"DataScience.clearAllOutput": "Clear All Output",
Expand Down
1 change: 0 additions & 1 deletion package.nls.zh-tw.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@
"diagnostics.consoleTypeDiagnostic": "Your launch.json file needs to be updated to change the console type string from \"none\" to \"internalConsole\", otherwise Python debugging may not work. Would you like to automatically update your launch.json file now?",
"diagnostics.justMyCodeDiagnostic": "Configuration \"debugStdLib\" in launch.json is no longer supported. It's recommended to replace it with \"justMyCode\", which is the exact opposite of using \"debugStdLib\". Would you like to automatically update your launch.json file to do that?",
"diagnostics.yesUpdateLaunch": "是，更新 launch.json",
"diagnostics.processId": "Attaching the debugger to a local process is an experimental feature. It will be available to all users soon.",
"diagnostics.invalidTestSettings": "Your settings needs to be updated to change the setting \"python.unitTest.\" to \"python.testing.\", otherwise testing Python code using the extension may not work. Would you like to automatically update your settings now?",
"Common.canceled": "已取消",
"Common.cancel": "取消",
Expand Down
12 changes: 0 additions & 12 deletions src/client/common/experimentGroups.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,6 @@ export enum ShowExtensionSurveyPrompt {
enabled = 'ShowExtensionSurveyPrompt - enabled'
}

// Experiment to check whether the extension should use the new VS Code debug adapter API.
export enum DebugAdapterDescriptorFactory {
control = 'DebugAdapterFactory - control',
experiment = 'DebugAdapterFactory - experiment'
}

// Experiment to check whether the ptvsd launcher should use pre-installed ptvsd wheels for debugging.
export enum DebugAdapterNewPtvsd {
control = 'PtvsdWheels37 - control',
experiment = 'PtvsdWheels37 - experiment'
}

// Experiment to check whether to enable re-load for web apps while debugging.
export enum WebAppReload {
control = 'Reload - control',
Expand Down
4 changes: 0 additions & 4 deletions src/client/common/utils/localize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,6 @@ export namespace Diagnostics {
'Your settings needs to be updated to change the setting "python.unitTest." to "python.testing.", otherwise testing Python code using the extension may not work. Would you like to automatically update your settings now?'
);
export const updateSettings = localize('diagnostics.updateSettings', 'Yes, update settings');
export const processId = localize(
'diagnostics.processId',
'Attaching the debugger to a local process is an experimental feature. It will be available to all users soon.'
);
}

export namespace Common {
Expand Down
24 changes: 7 additions & 17 deletions src/client/datascience/jupyter/jupyterDebugger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@ import * as vsls from 'vsls/vscode';
import { concatMultilineStringOutput } from '../../../datascience-ui/common';
import { ServerStatus } from '../../../datascience-ui/interactive-common/mainState';
import { IApplicationShell } from '../../common/application/types';
import { DebugAdapterNewPtvsd } from '../../common/experimentGroups';
import { traceError, traceInfo, traceWarning } from '../../common/logger';
import { IPlatformService } from '../../common/platform/types';
import { IConfigurationService, IExperimentsManager, Version } from '../../common/types';
import { IConfigurationService, Version } from '../../common/types';
import * as localize from '../../common/utils/localize';
import { EXTENSION_ROOT_DIR } from '../../constants';
import { captureTelemetry, sendTelemetryEvent } from '../../telemetry';
Expand Down Expand Up @@ -51,22 +50,13 @@ export class JupyterDebugger implements IJupyterDebugger, ICellHashListener {
@inject(IJupyterDebugService)
@named(Identifiers.MULTIPLEXING_DEBUGSERVICE)
private debugService: IJupyterDebugService,
@inject(IPlatformService) private platform: IPlatformService,
@inject(IExperimentsManager) private readonly experimentsManager: IExperimentsManager
@inject(IPlatformService) private platform: IPlatformService
) {
if (this.experimentsManager.inExperiment(DebugAdapterNewPtvsd.experiment)) {
this.debuggerPackage = 'debugpy';
this.enableDebuggerCode = `import debugpy;debugpy.listen(('localhost', 0))`;
this.waitForDebugClientCode = `import debugpy;debugpy.wait_for_client()`;
this.tracingEnableCode = `from debugpy import trace_this_thread;trace_this_thread(True)`;
this.tracingDisableCode = `from debugpy import trace_this_thread;trace_this_thread(False)`;
} else {
this.debuggerPackage = 'ptvsd';
this.enableDebuggerCode = `import ptvsd;ptvsd.enable_attach(('localhost', 0))`;
this.waitForDebugClientCode = `import ptvsd;ptvsd.wait_for_attach()`;
this.tracingEnableCode = `from ptvsd import tracing;tracing(True)`;
this.tracingDisableCode = `from ptvsd import tracing;tracing(False)`;
}
this.debuggerPackage = 'debugpy';
this.enableDebuggerCode = `import debugpy;debugpy.listen(('localhost', 0))`;
this.waitForDebugClientCode = `import debugpy;debugpy.wait_for_client()`;
this.tracingEnableCode = `from debugpy import trace_this_thread;trace_this_thread(True)`;
this.tracingDisableCode = `from debugpy import trace_this_thread;trace_this_thread(False)`;
}

public startRunByLine(notebook: INotebook, cellHashFileName: string): Promise<void> {
Expand Down
9 changes: 3 additions & 6 deletions src/client/debugger/extension/adapter/outdatedDebuggerPrompt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ import { inject, injectable } from 'inversify';
import { DebugAdapterTracker, DebugAdapterTrackerFactory, DebugSession, ProviderResult } from 'vscode';
import { DebugProtocol } from 'vscode-debugprotocol';
import { IApplicationShell } from '../../../common/application/types';
import { DebugAdapterNewPtvsd } from '../../../common/experimentGroups';
import { IBrowserService, IExperimentsManager } from '../../../common/types';
import { IBrowserService } from '../../../common/types';
import { Common, OutdatedDebugger } from '../../../common/utils/localize';
import { IPromptShowState } from './types';

// This situation occurs when user connects to old containers or server where
// the debugger they had installed was ptvsd. We should show a prompt to ask them to update.
class OutdatedDebuggerPrompt implements DebugAdapterTracker {
constructor(
private promptCheck: IPromptShowState,
Expand Down Expand Up @@ -71,14 +72,10 @@ class OutdatedDebuggerPromptState implements IPromptShowState {
export class OutdatedDebuggerPromptFactory implements DebugAdapterTrackerFactory {
private readonly promptCheck: OutdatedDebuggerPromptState;
constructor(
@inject(IExperimentsManager) private readonly experimentsManager: IExperimentsManager,
@inject(IApplicationShell) private readonly appShell: IApplicationShell,
@inject(IBrowserService) private browserService: IBrowserService
) {
this.promptCheck = new OutdatedDebuggerPromptState();
if (!this.experimentsManager.inExperiment(DebugAdapterNewPtvsd.experiment)) {
this.promptCheck.setShowPrompt(false);
}
}
public createDebugAdapterTracker(_session: DebugSession): ProviderResult<DebugAdapterTracker> {
return new OutdatedDebuggerPrompt(this.promptCheck, this.appShell, this.browserService);
Expand Down
13 changes: 2 additions & 11 deletions src/client/debugger/extension/configuration/resolvers/attach.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@
import { inject, injectable } from 'inversify';
import { CancellationToken, Uri, WorkspaceFolder } from 'vscode';
import { IDocumentManager, IWorkspaceService } from '../../../../common/application/types';
import { DebugAdapterNewPtvsd } from '../../../../common/experimentGroups';
import { IPlatformService } from '../../../../common/platform/types';
import { IConfigurationService, IExperimentsManager } from '../../../../common/types';
import { Diagnostics } from '../../../../common/utils/localize';
import { IConfigurationService } from '../../../../common/types';
import { AttachRequestArguments, DebugOptions, PathMapping } from '../../../types';
import { BaseConfigurationResolver } from './base';

Expand All @@ -19,8 +17,7 @@ export class AttachConfigurationResolver extends BaseConfigurationResolver<Attac
@inject(IWorkspaceService) workspaceService: IWorkspaceService,
@inject(IDocumentManager) documentManager: IDocumentManager,
@inject(IPlatformService) platformService: IPlatformService,
@inject(IConfigurationService) configurationService: IConfigurationService,
@inject(IExperimentsManager) private readonly experiments: IExperimentsManager
@inject(IConfigurationService) configurationService: IConfigurationService
) {
super(workspaceService, documentManager, platformService, configurationService);
}
Expand All @@ -29,12 +26,6 @@ export class AttachConfigurationResolver extends BaseConfigurationResolver<Attac
debugConfiguration: AttachRequestArguments,
_token?: CancellationToken
): Promise<AttachRequestArguments | undefined> {
if (
!this.experiments.inExperiment(DebugAdapterNewPtvsd.experiment) &&
debugConfiguration.processId !== undefined
) {
throw Error(Diagnostics.processId());
}
const workspaceFolder = this.getWorkspaceFolder(folder);

await this.provideAttachDefaults(workspaceFolder, debugConfiguration as AttachRequestArguments);
Expand Down
70 changes: 34 additions & 36 deletions src/client/debugger/extension/configuration/resolvers/launchConfigExperiment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
'use strict';

import { inject, injectable } from 'inversify';
import { DebugAdapterNewPtvsd, WebAppReload } from '../../../../common/experimentGroups';
import { WebAppReload } from '../../../../common/experimentGroups';
import { traceInfo } from '../../../../common/logger';
import { IExperimentsManager } from '../../../../common/types';
import { sendTelemetryEvent } from '../../../../telemetry';
Expand All @@ -17,45 +17,43 @@ export class LaunchDebugConfigurationExperiment implements ILaunchDebugConfigura
constructor(@inject(IExperimentsManager) private readonly experimentsManager: IExperimentsManager) {}

public modifyConfigurationBasedOnExperiment(debugConfiguration: LaunchRequestArguments): void {
if (this.experimentsManager.inExperiment(DebugAdapterNewPtvsd.experiment)) {
if (this.experimentsManager.inExperiment(WebAppReload.experiment)) {
if (this.isWebAppConfiguration(debugConfiguration)) {
traceInfo(
`Configuration used for Web App Reload experiment (before):\n${JSON.stringify(
debugConfiguration,
undefined,
4
)}`
);
if (this.experimentsManager.inExperiment(WebAppReload.experiment)) {
if (this.isWebAppConfiguration(debugConfiguration)) {
traceInfo(
`Configuration used for Web App Reload experiment (before):\n${JSON.stringify(
debugConfiguration,
undefined,
4
)}`
);

let subProcessModified: boolean = false;
if (!debugConfiguration.subProcess) {
subProcessModified = true;
debugConfiguration.subProcess = true;
}

let argsModified: boolean = false;
const args = debugConfiguration.args.filter((arg) => arg !== '--noreload' && arg !== '--no-reload');
if (args.length !== debugConfiguration.args.length) {
argsModified = true;
debugConfiguration.args = args;
}
let subProcessModified: boolean = false;
if (!debugConfiguration.subProcess) {
subProcessModified = true;
debugConfiguration.subProcess = true;
}

traceInfo(
`Configuration used for Web App Reload experiment (after):\n${JSON.stringify(
debugConfiguration,
undefined,
4
)}`
);
sendTelemetryEvent(EventName.PYTHON_WEB_APP_RELOAD, undefined, {
subProcessModified,
argsModified
});
let argsModified: boolean = false;
const args = debugConfiguration.args.filter((arg) => arg !== '--noreload' && arg !== '--no-reload');
if (args.length !== debugConfiguration.args.length) {
argsModified = true;
debugConfiguration.args = args;
}
} else {
this.experimentsManager.sendTelemetryIfInExperiment(WebAppReload.control);

traceInfo(
`Configuration used for Web App Reload experiment (after):\n${JSON.stringify(
debugConfiguration,
undefined,
4
)}`
);
sendTelemetryEvent(EventName.PYTHON_WEB_APP_RELOAD, undefined, {
subProcessModified,
argsModified
});
}
} else {
this.experimentsManager.sendTelemetryIfInExperiment(WebAppReload.control);
}
}

Expand Down
8 changes: 2 additions & 6 deletions src/test/debugger/attach.ptvsd.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,9 @@ import { DebugClient } from 'vscode-debugadapter-testsupport';

import { IDocumentManager, IWorkspaceService } from '../../client/common/application/types';
import { EXTENSION_ROOT_DIR } from '../../client/common/constants';
import { DebugAdapterNewPtvsd } from '../../client/common/experimentGroups';
import { IS_WINDOWS } from '../../client/common/platform/constants';
import { IPlatformService } from '../../client/common/platform/types';
import { IConfigurationService, IExperimentsManager } from '../../client/common/types';
import { IConfigurationService } from '../../client/common/types';
import { MultiStepInputFactory } from '../../client/common/utils/multiStepInput';
import { DebuggerTypeName, PTVSD_PATH } from '../../client/debugger/constants';
import { PythonDebugConfigurationService } from '../../client/debugger/extension/configuration/debugConfigurationService';
Expand Down Expand Up @@ -116,16 +115,13 @@ suite('Debugging - Attach Debugger', () => {
const workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
const documentManager = TypeMoq.Mock.ofType<IDocumentManager>();
const configurationService = TypeMoq.Mock.ofType<IConfigurationService>();
const experiments = TypeMoq.Mock.ofType<IExperimentsManager>();
experiments.setup((e) => e.inExperiment(DebugAdapterNewPtvsd.experiment)).returns(() => true);

const launchResolver = TypeMoq.Mock.ofType<IDebugConfigurationResolver<LaunchRequestArguments>>();
const attachResolver = new AttachConfigurationResolver(
workspaceService.object,
documentManager.object,
platformService.object,
configurationService.object,
Copy link

Choose a reason for hiding this comment

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

FWIW, I still think we should require trailing commas (in the tslint and prettier configs). Then we would avoid this sort of churn in PRs/commit history (and it's less annoying when editing code).

experiments.object
configurationService.object
);
const providerFactory = TypeMoq.Mock.ofType<IDebugConfigurationProviderFactory>().object;
const multistepFactory = mock(MultiStepInputFactory);
Expand Down
Loading