-
Notifications
You must be signed in to change notification settings - Fork 36.5k
Support streaming of partial tool data #278640
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
base: main
Are you sure you want to change the base?
Changes from all commits
4384189
8faa408
f40703c
f502d22
8556686
4c4db3a
bb9ca62
a4c4ce7
5c8e7d9
3494a28
edde8ce
79f690c
157d91b
7ed00f6
e143288
95bfedc
4e96567
d700026
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,117 @@ | ||||||||
| /*--------------------------------------------------------------------------------------------- | ||||||||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||||||||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||||||||
| *--------------------------------------------------------------------------------------------*/ | ||||||||
|
|
||||||||
| import { CancellationToken } from '../../../../../base/common/cancellation.js'; | ||||||||
| import { MarkdownString } from '../../../../../base/common/htmlContent.js'; | ||||||||
| import { IMarkdownRenderer } from '../../../../../platform/markdown/browser/markdownRenderer.js'; | ||||||||
| import { localize } from '../../../../../nls.js'; | ||||||||
| import { IChatPrepareToolInvocationPart, IChatProgressMessage } from '../../common/chatService.js'; | ||||||||
| import { IChatRendererContent, isResponseVM } from '../../common/chatViewModel.js'; | ||||||||
| import { ChatTreeItem } from '../chat.js'; | ||||||||
| import { IChatContentPart, IChatContentPartRenderContext } from './chatContentParts.js'; | ||||||||
| import { ChatProgressContentPart } from './chatProgressContentPart.js'; | ||||||||
| import { ILanguageModelToolsService, IToolInvocationStreamContext } from '../../common/languageModelToolsService.js'; | ||||||||
| import { IInstantiationService } from '../../../../../platform/instantiation/common/instantiation.js'; | ||||||||
| import { IChatMarkdownAnchorService } from './chatMarkdownAnchorService.js'; | ||||||||
| import { IConfigurationService } from '../../../../../platform/configuration/common/configuration.js'; | ||||||||
| import { ILogService } from '../../../../../platform/log/common/log.js'; | ||||||||
|
|
||||||||
| /** | ||||||||
| * Content part for rendering prepareToolInvocation progress. | ||||||||
| * This handles calling handleToolStream at the view layer and rendering the result. | ||||||||
| * Similar to progressMessage, this hides when other content types arrive after it. | ||||||||
| */ | ||||||||
| export class ChatPrepareToolInvocationPart extends ChatProgressContentPart implements IChatContentPart { | ||||||||
| private readonly element: ChatTreeItem; | ||||||||
| private readonly isHiddenByFollowingContent: boolean; | ||||||||
|
|
||||||||
| constructor( | ||||||||
| private prepareToolInvocation: IChatPrepareToolInvocationPart, | ||||||||
| chatContentMarkdownRenderer: IMarkdownRenderer, | ||||||||
| context: IChatContentPartRenderContext, | ||||||||
| @IInstantiationService instantiationService: IInstantiationService, | ||||||||
| @IChatMarkdownAnchorService chatMarkdownAnchorService: IChatMarkdownAnchorService, | ||||||||
| @IConfigurationService configurationService: IConfigurationService, | ||||||||
| @ILanguageModelToolsService private readonly languageModelToolsService: ILanguageModelToolsService, | ||||||||
| @ILogService private readonly logService: ILogService | ||||||||
| ) { | ||||||||
| // Create initial progress message - will be updated async | ||||||||
| const initialProgressMessage = ChatPrepareToolInvocationPart.createInitialProgressMessage(prepareToolInvocation, languageModelToolsService); | ||||||||
| super(initialProgressMessage, chatContentMarkdownRenderer, context, undefined, undefined, undefined, undefined, instantiationService, chatMarkdownAnchorService, configurationService); | ||||||||
|
|
||||||||
| this.element = context.element; | ||||||||
|
|
||||||||
| // Hide when following content contains parts that are not prepareToolInvocation or progressMessage | ||||||||
| // This is similar to how progressMessage parts hide when other content arrives | ||||||||
| const followingContent = context.content.slice(context.contentIndex + 1); | ||||||||
| this.isHiddenByFollowingContent = followingContent.some(part => part.kind !== 'prepareToolInvocation' && part.kind !== 'progressMessage'); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a huge fan of of this approach of rendering a new part for each piece of the stream. If we imagine a pedantic case where I have a tool call emitting a word at a time generating an enormous file this could get expensive even for relatively small files of a few hundred KB. How I would imagine it should work is the stream gets passed to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already do this approach for progress which is why I followed it vscode/src/vs/workbench/contrib/chat/browser/chatContentParts/chatProgressContentPart.ts Lines 51 to 53 in 95bfedc
since effectively this is how we want this to behave as well. But I guess with progress that is a part that gets emitted a lot less |
||||||||
|
|
||||||||
| // Asynchronously call handleToolStream and update the message | ||||||||
| if (!this.isHiddenByFollowingContent) { | ||||||||
| this.fetchAndUpdateMessage(prepareToolInvocation); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| private static createInitialProgressMessage(prepareToolInvocation: IChatPrepareToolInvocationPart, languageModelToolsService: ILanguageModelToolsService): IChatProgressMessage { | ||||||||
| const toolData = languageModelToolsService.getTool(prepareToolInvocation.toolName); | ||||||||
| const displayName = toolData?.displayName ?? prepareToolInvocation.toolName; | ||||||||
| return { | ||||||||
| kind: 'progressMessage', | ||||||||
| content: new MarkdownString(localize('invokingTool', "Invoking tool: {0}", displayName)) | ||||||||
| }; | ||||||||
| } | ||||||||
|
|
||||||||
| private async fetchAndUpdateMessage(prepareToolInvocation: IChatPrepareToolInvocationPart): Promise<void> { | ||||||||
| const toolData = this.languageModelToolsService.getTool(prepareToolInvocation.toolName); | ||||||||
| if (!toolData) { | ||||||||
| return; | ||||||||
| } | ||||||||
|
|
||||||||
| const streamContext: IToolInvocationStreamContext = { | ||||||||
| toolCallId: prepareToolInvocation.toolCallId, | ||||||||
| rawInput: prepareToolInvocation.streamData?.partialInput, | ||||||||
| chatRequestId: isResponseVM(this.element) ? this.element.requestId : undefined, | ||||||||
| chatSessionId: this.element.sessionId | ||||||||
| }; | ||||||||
|
|
||||||||
| try { | ||||||||
| const streamResult = await this.languageModelToolsService.handleToolStream(toolData.id, streamContext, CancellationToken.None); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We cannot do side-effects like this from the chat widgets, as this will not work for background sessions. One could say that partial results don't matter if the user isn't actively looking at the window, but we then might get into weird states if the user opens or closes the session mid-stream. Better imo to keep the view and the data model separate. |
||||||||
| if (streamResult?.invocationMessage) { | ||||||||
| const progressContent = typeof streamResult.invocationMessage === 'string' | ||||||||
| ? new MarkdownString(streamResult.invocationMessage) | ||||||||
| : new MarkdownString(streamResult.invocationMessage.value, { isTrusted: streamResult.invocationMessage.isTrusted, supportThemeIcons: streamResult.invocationMessage.supportThemeIcons, supportHtml: streamResult.invocationMessage.supportHtml }); | ||||||||
| this.updateMessage(progressContent); | ||||||||
| } | ||||||||
| } catch (error) { | ||||||||
| this.logService.warn(`ChatPrepareToolInvocationPart: Error calling handleToolStream for tool ${toolData.id}`, error); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| override hasSameContent(other: IChatRendererContent, followingContent: IChatRendererContent[], element: ChatTreeItem): boolean { | ||||||||
| if (other.kind !== 'prepareToolInvocation') { | ||||||||
| return false; | ||||||||
| } | ||||||||
|
|
||||||||
| // If following content contains parts that are not prepareToolInvocation or progressMessage, | ||||||||
| // we need to re-render to hide this part (similar to progressMessage behavior) | ||||||||
| const shouldBeHidden = followingContent.some(part => part.kind !== 'prepareToolInvocation' && part.kind !== 'progressMessage'); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should follow this pattern of hiding things. Now we have a toolcallid so we can associate the incomplete call with the real call, it seems to me like the incoplete call should just sort of resolve into the real call widget. |
||||||||
| if (shouldBeHidden && !this.isHiddenByFollowingContent) { | ||||||||
| return false; | ||||||||
| } | ||||||||
|
|
||||||||
| // Same toolCallId means this is an update to the same tool invocation | ||||||||
| // We should reuse this part and update its content | ||||||||
| if (other.toolCallId === this.prepareToolInvocation.toolCallId) { | ||||||||
| // Update with new stream data | ||||||||
| this.prepareToolInvocation = other; | ||||||||
| if (!this.isHiddenByFollowingContent) { | ||||||||
| this.fetchAndUpdateMessage(other); | ||||||||
| } | ||||||||
| return true; | ||||||||
| } | ||||||||
|
|
||||||||
| return false; | ||||||||
| } | ||||||||
| } | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,7 @@ import { ChatExtensionsContentPart } from './chatContentParts/chatExtensionsCont | |
| import { ChatMarkdownContentPart } from './chatContentParts/chatMarkdownContentPart.js'; | ||
| import { ChatMcpServersInteractionContentPart } from './chatContentParts/chatMcpServersInteractionContentPart.js'; | ||
| import { ChatMultiDiffContentPart } from './chatContentParts/chatMultiDiffContentPart.js'; | ||
| import { ChatPrepareToolInvocationPart } from './chatContentParts/chatPrepareToolInvocationPart.js'; | ||
| import { ChatProgressContentPart, ChatWorkingProgressContentPart } from './chatContentParts/chatProgressContentPart.js'; | ||
| import { ChatPullRequestContentPart } from './chatContentParts/chatPullRequestContentPart.js'; | ||
| import { ChatQuotaExceededPart } from './chatContentParts/chatQuotaExceededPart.js'; | ||
|
|
@@ -771,6 +772,11 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch | |
| return false; | ||
| } | ||
|
|
||
| // Don't show working if prepareToolInvocation is already present | ||
| if (partsToRender.some(part => part.kind === 'prepareToolInvocation')) { | ||
| return false; | ||
| } | ||
|
|
||
| // Show if no content, only "used references", ends with a complete tool call, or ends with complete text edits and there is no incomplete tool call (edits are still being applied some time after they are all generated) | ||
| const lastPart = findLast(partsToRender, part => part.kind !== 'markdownContent' || part.content.value.trim().length > 0); | ||
|
|
||
|
|
@@ -790,7 +796,7 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch | |
| ((lastPart.kind === 'toolInvocation' || lastPart.kind === 'toolInvocationSerialized') && (IChatToolInvocation.isComplete(lastPart) || lastPart.presentation === 'hidden')) || | ||
| ((lastPart.kind === 'textEditGroup' || lastPart.kind === 'notebookEditGroup') && lastPart.done && !partsToRender.some(part => part.kind === 'toolInvocation' && !IChatToolInvocation.isComplete(part))) || | ||
| (lastPart.kind === 'progressTask' && lastPart.deferred.isSettled) || | ||
| lastPart.kind === 'prepareToolInvocation' || lastPart.kind === 'mcpServersStarting' | ||
| lastPart.kind === 'mcpServersStarting' | ||
|
||
| ) { | ||
| return true; | ||
| } | ||
|
|
@@ -1370,6 +1376,8 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch | |
| return this.renderMcpServersInteractionRequired(content, context, templateData); | ||
| } else if (content.kind === 'thinking') { | ||
| return this.renderThinkingPart(content, context, templateData); | ||
| } else if (content.kind === 'prepareToolInvocation') { | ||
| return this.instantiationService.createInstance(ChatPrepareToolInvocationPart, content, this.chatContentMarkdownRenderer, context); | ||
| } | ||
|
|
||
| return this.renderNoContent(other => content.kind === other.kind); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this only be defined if the tool in the ext host side has this method defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would we know if the tool in the ext host has this defined? Isn't this the same as prepareToolInvocation?