Skip to content

Remove Copilot completion request cancellation. #13012

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

Closed
wants to merge 3 commits into from
Closed

Remove Copilot completion request cancellation. #13012

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ac59fc
Remove Copilot completion request cancellation.
sean-mcmanus Dec 3, 2024
27c92cc
Reword comment.
sean-mcmanus Dec 3, 2024
bb17a72
Merge branch 'main' into seanmcm/avoidUnnecessaryCancelRetryWithGitHu…
sean-mcmanus Dec 3, 2024
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
43 changes: 32 additions & 11 deletions Extension/src/LanguageServer/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,9 +804,9 @@ export interface Client {
getShowConfigureIntelliSenseButton(): boolean;
setShowConfigureIntelliSenseButton(show: boolean): void;
addTrustedCompiler(path: string): Promise<void>;
getIncludes(maxDepth: number, token: vscode.CancellationToken): Promise<GetIncludesResult>;
getIncludes(maxDepth: number): Promise<GetIncludesResult>;
getChatContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ChatContextResult>;
getProjectContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ProjectContextResult>;
getProjectContext(uri: vscode.Uri): Promise<ProjectContextResult>;
}

export function createClient(workspaceFolder?: vscode.WorkspaceFolder): Client {
Expand Down Expand Up @@ -2228,11 +2228,11 @@ export class DefaultClient implements Client {
await this.languageClient.sendNotification(DidOpenNotification, params);
}

public async getIncludes(maxDepth: number, token: vscode.CancellationToken): Promise<GetIncludesResult> {
public async getIncludes(maxDepth: number): Promise<GetIncludesResult> {
const params: GetIncludesParams = { maxDepth: maxDepth };
await this.ready;
return DefaultClient.withLspCancellationHandling(
() => this.languageClient.sendRequest(IncludesRequest, params, token), token);
return DefaultClient.withoutLspCancellationHandling(
() => this.languageClient.sendRequest(IncludesRequest, params));
}

public async getChatContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ChatContextResult> {
Expand All @@ -2242,11 +2242,11 @@ export class DefaultClient implements Client {
() => this.languageClient.sendRequest(CppContextRequest, params, token), token);
}

public async getProjectContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ProjectContextResult> {
public async getProjectContext(uri: vscode.Uri): Promise<ProjectContextResult> {
const params: TextDocumentIdentifier = { uri: uri.toString() };
await withCancellation(this.ready, token);
return DefaultClient.withLspCancellationHandling(
() => this.languageClient.sendRequest(ProjectContextRequest, params, token), token);
await this.ready;
return DefaultClient.withoutLspCancellationHandling(
() => this.languageClient.sendRequest(ProjectContextRequest, params));
}

/**
Expand Down Expand Up @@ -2348,6 +2348,27 @@ export class DefaultClient implements Client {
return result;
}

// This is used to avoid processing unnecessary LSP cancel requests during a Copilot completion
Copy link
Member

@lukka lukka Dec 3, 2024

Choose a reason for hiding this comment

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

@sean-mcmanus the exact reason is that caching is happening on copilot-client, hence even if the token is cancelled, it means that the return value will NOT be used for the prompt construction, but it will be used for caching purposes.

Copy link
Contributor Author

@sean-mcmanus sean-mcmanus Dec 3, 2024

Choose a reason for hiding this comment

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

Yes, I know it's not used for the currently running completion (it doesn't block Copilot), but it's cached and used on the next one. Does my comment not explain that? i.e. should I re-word my comment?

Copy link
Member

@lukka lukka Dec 3, 2024

Choose a reason for hiding this comment

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

I'd omit the 2-minute detail since it is an implementation detail not located in this codebase. Instead, I'd mention that upon cancellation, the result will not be used to build the prompt (since the copilot extension gave up on waiting for a value from this provider), but the returned value is cached by the copilot extension and used on a subsequent call when available, where the cache will be hit and the call to getProjectContext will be saved.

The 2 minutes is the cache entry eviction time defined by the copilot extension.

Copy link
Contributor Author

@sean-mcmanus sean-mcmanus Dec 3, 2024

Choose a reason for hiding this comment

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

Ok, I tried to re-word it.

// when the result could still be used in 2 minutes in its cache before it sends another request.
private static async withoutLspCancellationHandling<T>(task: () => Promise<T>): Promise<T> {
let result: T;

try {
result = await task();
} catch (e: any) {
if (e instanceof ResponseError && e.code === ServerCancelled) {
// This can occur if the user switches files and makes edits fast enough.
// It doesn't seem like a very important scenario, so cancelling seems fine, even though
// not cancelling would allow the value to be cached if the user switches back to that file.
throw new vscode.CancellationError();
} else {
throw e;
}
}

return result;
}

private callTaskWithTimeout<T>(task: () => Thenable<T>, ms: number, cancelToken?: vscode.CancellationTokenSource): Promise<T> {
let timer: NodeJS.Timeout;

Expand Down Expand Up @@ -4151,7 +4172,7 @@ class NullClient implements Client {
getShowConfigureIntelliSenseButton(): boolean { return false; }
setShowConfigureIntelliSenseButton(show: boolean): void { }
addTrustedCompiler(path: string): Promise<void> { return Promise.resolve(); }
getIncludes(maxDepth: number, token: vscode.CancellationToken): Promise<GetIncludesResult> { return Promise.resolve({} as GetIncludesResult); }
getIncludes(maxDepth: number): Promise<GetIncludesResult> { return Promise.resolve({} as GetIncludesResult); }
getChatContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ChatContextResult> { return Promise.resolve({} as ChatContextResult); }
getProjectContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ProjectContextResult> { return Promise.resolve({} as ProjectContextResult); }
getProjectContext(uri: vscode.Uri): Promise<ProjectContextResult> { return Promise.resolve({} as ProjectContextResult); }
}
10 changes: 5 additions & 5 deletions Extension/src/LanguageServer/copilotProviders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,14 @@ export async function registerRelatedFilesProvider(): Promise<void> {
for (const languageId of ['c', 'cpp', 'cuda-cpp']) {
api.registerRelatedFilesProvider(
{ extensionId: util.extensionContext.extension.id, languageId },
async (uri: vscode.Uri, context: { flags: Record<string, unknown> }, token: vscode.CancellationToken) => {
async (uri: vscode.Uri, context: { flags: Record<string, unknown> }) => {
const start = performance.now();
const telemetryProperties: Record<string, string> = {};
const telemetryMetrics: Record<string, number> = {};
try {
const getIncludesHandler = async () => (await getIncludesWithCancellation(1, token))?.includedFiles.map(file => vscode.Uri.file(file)) ?? [];
const getIncludesHandler = async () => (await getIncludes(1))?.includedFiles.map(file => vscode.Uri.file(file)) ?? [];
const getTraitsHandler = async () => {
const projectContext = await getProjectContext(uri, context, token);
const projectContext = await getProjectContext(uri, context);

if (!projectContext) {
return undefined;
Expand Down Expand Up @@ -154,9 +154,9 @@ export async function registerRelatedFilesProvider(): Promise<void> {
}
}

async function getIncludesWithCancellation(maxDepth: number, token: vscode.CancellationToken): Promise<GetIncludesResult> {
async function getIncludes(maxDepth: number): Promise<GetIncludesResult> {
const activeClient = getActiveClient();
const includes = await activeClient.getIncludes(maxDepth, token);
const includes = await activeClient.getIncludes(maxDepth);
const wksFolder = activeClient.RootUri?.toString();

if (!wksFolder) {
Expand Down
4 changes: 2 additions & 2 deletions Extension/src/LanguageServer/lmTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,11 @@ function filterCompilerArguments(compiler: string, compilerArguments: string[],
return result;
}

export async function getProjectContext(uri: vscode.Uri, context: { flags: Record<string, unknown> }, token: vscode.CancellationToken): Promise<ProjectContext | undefined> {
export async function getProjectContext(uri: vscode.Uri, context: { flags: Record<string, unknown> }): Promise<ProjectContext | undefined> {
const telemetryProperties: Record<string, string> = {};
const telemetryMetrics: Record<string, number> = {};
try {
const projectContext = await checkDuration<ProjectContextResult | undefined>(async () => await getClients()?.ActiveClient?.getProjectContext(uri, token) ?? undefined);
const projectContext = await checkDuration<ProjectContextResult | undefined>(async () => await getClients()?.ActiveClient?.getProjectContext(uri) ?? undefined);
telemetryMetrics["duration"] = projectContext.duration;
if (!projectContext.result) {
return undefined;
Expand Down
4 changes: 2 additions & 2 deletions Extension/test/scenarios/SingleRootProject/tests/lmTool.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ describe('CppConfigurationLanguageModelTool Tests', () => {
}
});

const result = await getProjectContext(mockTextDocumentStub.uri, context, new vscode.CancellationTokenSource().token);
const result = await getProjectContext(mockTextDocumentStub.uri, context);

ok(result, 'result should not be undefined');
ok(result.language === 'C++');
Expand Down Expand Up @@ -364,7 +364,7 @@ describe('CppConfigurationLanguageModelTool Tests', () => {
}
});

const result = await getProjectContext(mockTextDocumentStub.uri, { flags: {} }, new vscode.CancellationTokenSource().token);
const result = await getProjectContext(mockTextDocumentStub.uri, { flags: {} });

ok(telemetryStub.calledOnce, 'Telemetry should be called once');
ok(telemetryStub.calledWithMatch('ProjectContext', sinon.match({
Expand Down
Loading