Send server contextualization to Copilot extension #24230
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,10 +5,11 @@ | |
|
|
||
| import * as azdata from 'azdata'; | ||
| import { invalidProvider } from 'sql/base/common/errors'; | ||
| import { IConnectionManagementService } from 'sql/platform/connection/common/connectionManagement'; | ||
| import { IQueryEditorConfiguration } from 'sql/platform/query/common/query'; | ||
| import { IServerContextualizationService } from 'sql/workbench/services/contextualization/common/interfaces'; | ||
| import { Disposable } from 'vs/base/common/lifecycle'; | ||
| import { ICommandService } from 'vs/platform/commands/common/commands'; | ||
| import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; | ||
| import { IExtensionService } from 'vs/workbench/services/extensions/common/extensions'; | ||
|
|
||
|
|
@@ -19,18 +20,10 @@ export class ServerContextualizationService extends Disposable implements IServe | |
| constructor( | ||
| @IConnectionManagementService private readonly _connectionManagementService: IConnectionManagementService, | ||
| @IConfigurationService private readonly _configurationService: IConfigurationService, | ||
| @IExtensionService private readonly _extensionService: IExtensionService, | ||
| @ICommandService private readonly _commandService: ICommandService | ||
| ) { | ||
| super(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -63,32 +56,86 @@ export class ServerContextualizationService extends Disposable implements IServe | |
| throw invalidProvider(providerId); | ||
| } | ||
|
|
||
| /** | ||
| * Contextualizes the provided URI for GitHub Copilot. | ||
| * @param uri The URI to contextualize for Copilot. | ||
| * @returns Copilot will have the URI contextualized when the promise completes. | ||
| */ | ||
| public async contextualizeUriForCopilot(uri: string): Promise<void> { | ||
| // Don't need to take any actions if contextualization is not enabled and can return | ||
| const isContextualizationNeeded = await this.isContextualizationNeeded(); | ||
| if (!isContextualizationNeeded) { | ||
| return; | ||
| } | ||
|
|
||
| const getServerContextualizationResult = await this.getServerContextualization(uri); | ||
| if (getServerContextualizationResult.context) { | ||
| await this.sendServerContextualizationToCopilot(getServerContextualizationResult.context); | ||
| } | ||
| else { | ||
| const generateServerContextualizationResult = await this.generateServerContextualization(uri); | ||
| if (generateServerContextualizationResult.context) { | ||
| await this.sendServerContextualizationToCopilot(generateServerContextualizationResult.context); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Generates server context | ||
| * @param ownerUri The URI of the connection to generate context for. | ||
| */ | ||
| private async generateServerContextualization(ownerUri: string): Promise<azdata.contextualization.GenerateServerContextualizationResult> { | ||
| const providerName = this._connectionManagementService.getProviderIdFromUri(ownerUri); | ||
| const handler = this.getProvider(providerName); | ||
| if (handler) { | ||
| return await handler.generateServerContextualization(ownerUri); | ||
| } | ||
| else { | ||
| return Promise.resolve({ | ||
| context: undefined | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Gets all database context. | ||
| * @param ownerUri The URI of the connection to get context for. | ||
| */ | ||
| private async getServerContextualization(ownerUri: string): Promise<azdata.contextualization.GetServerContextualizationResult> { | ||
| const providerName = this._connectionManagementService.getProviderIdFromUri(ownerUri); | ||
| const handler = this.getProvider(providerName); | ||
| if (handler) { | ||
| return await handler.getServerContextualization(ownerUri); | ||
| } | ||
| else { | ||
| return Promise.resolve({ | ||
|
There was a problem hiding this comment. would it be helpful to also log an error here that the provider wasn't able to be retrieved for debugging? There was a problem hiding this comment. I'm going to have a follow up PR with all the logging. |
||
| context: undefined | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Sends the provided context over to copilot, so that it can be used to generate improved suggestions. | ||
| * @param serverContext The context to be sent over to Copilot | ||
| */ | ||
| private async sendServerContextualizationToCopilot(serverContext: string | undefined): Promise<void> { | ||
| if (serverContext) { | ||
| // LEWISSANCHEZ TODO: Find way to set context on untitled query editor files. Need to save first for Copilot status to say "Has Context" | ||
|
There was a problem hiding this comment. Hey @lewis-sanchez, let's chat about this --- I can change the fork slightly to better work w/ untitled files. I also want to make sure that, before the call to I can play around with the compression and get back to you on that. (I'd like to start w/ similar heuristics to what we use elsewhere.) |
||
| await this._commandService.executeCommand('github.copilot.provideContext', '**/*.sql', { | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. I can comment here (since I'm working on the copilot fork side): context is associated to files via a glob-style pattern. One context object is allowed per unique pattern (so, in this case, calling You could make this call a bit more granular, so that it matches only the given file (but we still need to work through what happens w/ untitled editors). There was a problem hiding this comment. Cool, thanks for the info. How does copilot know what editor to associate what context with? There was a problem hiding this comment. On the copilot side, as you type in an editor, we'll see if the uri for that editor matches any pattern in the This seemingly works well (except for untitled editors, not sure what the uri for those looks like on the Copilot side, need to do some tests there). There was a problem hiding this comment. Great! So yeah @lewis-sanchez, sounds like we should scope the context to be for each file. @jjhenkel Is there any concern about context not being cleaned up if the editor is closed (or does copilot handle that already)? There was a problem hiding this comment. Cleanup could be done by setting the context to empty when an editor is closed, right? Anyways, I agree it's not something that is critical to address now it sounds like - but something to keep in mind as we're designing how this will all fit together. There was a problem hiding this comment. Set to empty (assuming we're using the more specific per-file patterns) would be a way to implement some level of cleanup for now, yes! There was a problem hiding this comment. @Charles-Gagnon, I thought this was already scoped to the file, but guess not. Do you know where I would need to make changes to scope context to a file? There was a problem hiding this comment. @jjhenkel Should be able to help. From what he was saying earlier you have to modify the pattern you use when setting it (right now hardcoded to There was a problem hiding this comment. @lewis-sanchez I'm looking at your branch today/tomorrow and can do some testing w/ stricter path constraints and what happens w/ untitled files (and modify the copilot fork if needed). |
||
| value: serverContext | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Checks if contextualization is needed. This is based on whether the Copilot extension is installed and the GitHub Copilot | ||
| * contextualization setting is enabled. | ||
| * @returns A promise that resolves to true if contextualization is needed, false otherwise. | ||
| */ | ||
| private async isContextualizationNeeded(): Promise<boolean> { | ||
| const copilotExt = await this._extensionService.getExtension('github.copilot'); | ||
| const isContextualizationEnabled = this._configurationService.getValue<IQueryEditorConfiguration>('queryEditor').githubCopilotContextualizationEnabled | ||
|
|
||
| return (copilotExt && isContextualizationEnabled); | ||
| } | ||
| } | ||
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.
If you're planning on consolidating into one message than this is fine for now, but if you're keeping both then there's some room for cleanup here since you get context from both the generate and get calls which is a bit redundant.