-
Notifications
You must be signed in to change notification settings - Fork 906
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
Send server contextualization to Copilot extension #24230
Send server contextualization to Copilot extension #24230
Conversation
src/sql/workbench/services/contextualization/common/serverContextualizationService.ts
Outdated
Show resolved
Hide resolved
src/sql/workbench/services/contextualization/common/serverContextualizationService.ts
Outdated
Show resolved
Hide resolved
flattenedServerContext = result.context.filter(c => c.includes('CREATE')).join('\n'); | ||
} | ||
|
||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 github.copilot.provideContext
we hook in some amount of "schema compression" (using a fixed budget, as we discussed offline).
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.)
const copilotExt = await this._extensionService.getExtension('github.copilot'); | ||
if (copilotExt && serverContext && this._configurationService.getValue<IQueryEditorConfiguration>('queryEditor').githubCopilotContextualizationEnabled) { | ||
// LEWISSANCHEZ TODO: Find way to set context on untitled query editor files. Need to save first for Copilot status to say "Has Context" | ||
await this._commandService.executeCommand('github.copilot.provideContext', '**/*.sql', { |
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.
- Is there any way to specify what editor the context is for?
- Does providing context clear the other context?
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.
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 provideContext
again with the exact same **/*.sql
pattern will replace the existing context).
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks for the info. How does copilot know what editor to associate what context with?
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.
On the copilot side, as you type in an editor, we'll see if the uri for that editor matches any pattern in the { pattern: context }
map. For each pattern that matches, we add the context as a candidate for inclusion into the prompt.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 **/*.sql
). So the change I believe it would be would be something more like <Path to file>/FileName.sql
instead (although what the pattern can accept isn't something I know)
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.
@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).
src/sql/workbench/services/contextualization/common/serverContextualizationService.ts
Outdated
Show resolved
Hide resolved
if (getServerContextualizationResult.context) { | ||
await this.serverContextualizationService.sendServerContextualizationToCopilot(getServerContextualizationResult.context); | ||
} | ||
else { |
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.
So now you're changing it to only generate the context when an editor is connected?
- Is there any concern about the duration? As I found in my testing - getting the scripts can take a long time so it might be quite a while after an editor is opened before the context is sent
- If you're going with this then would it be better to just get rid of the "generate" call completely and instead just have get be the only call (and if it doesn't have context it'll go generate that before returning)?
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.
I think it would be better to do both actions in one request, but I wasn't sure if generating and reading context was considered doing too much in a single request endpoint, which is why I kept them separate.
I can consolidate the two requests into a single one, so that this new request can read the context or generate context if there isn't any available for it to send back to ADS.
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.
Sounds good, I think consolidating into one makes sense to keep things simpler. Then you can easily add something like a "forceRefresh" param as well later on to force regenerating the context if that's something you want (probably useful for users to be able to control like how we have a refresh intellisense command)
this.contextualizeEditorForCopilot(); | ||
} | ||
|
||
private async contextualizeEditorForCopilot(): Promise<void> { |
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.
Consider adding logging at some point too for debugging issues
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.
Some cleanup suggestions, but otherwise fine. The bigger changes like switching to a single message can probably be done as a follow-up PR if needed.
src/sql/workbench/services/contextualization/common/interfaces.ts
Outdated
Show resolved
Hide resolved
return; | ||
} | ||
|
||
const getServerContextualizationResult = await this.getServerContextualization(uri); |
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.
} | ||
} | ||
|
||
/** | ||
* Gets all database context. | ||
* @param ownerUri The URI of the connection to get context for. | ||
*/ | ||
public async getServerContextualization(ownerUri: string): Promise<azdata.contextualization.GetServerContextualizationResult> { | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to have a follow up PR with all the logging.
* Add Attach Database dialog (#24225) * Enabling FILEGROUPS tab experience to the database properties (#24226) * initial changes for loadin dsc table with real values from smo * Displaying diff columns for DSC for diff sql server * checkbox maiants the selection * elevate option fails to load correct value when set to when_supported option * all working till maxdop, todo pause option, save * commented MAXDOP changes, as it is causing issues * primary,sec,checkbox working as expected, TODO:MaxDop etc options,saving,tests * Undo MAXDOP commented code * refactored with service data * column header width adjustments * Maxdop and pause resume options completed, apply button is failing now * Removed option names from loc and using Id instead as names may change in future like in doc * Apply button fixed * refactored to reduce table reload * Ledger digest completed * refactor done: maxdop secondary shows wrong data from pause_resume * refactor more: all working but table focus disturbs on update table * adds conditions for unsupported dsc to <2016 server * maxdop secondary checkbox fix * rows still loses focus after value change due to update table row data * Fixed updating secondary dropdown value * reusing the private method and removed the duplicated codes * initial commit - fullText and owner need revision * Enter key in input type allows the change to update the table data, reduces the live update issues * Setting focus to the current row * loading data, need stylings-increase col length, etc * using the existed setTableData method * Adding new file dialog * creating addFile, but not displaying in table, issue with appendData * Adding row to the table, options are getting from STS * all working except InPercent value * code review comment updates * Input type checkbox update table additional validation * all except path * fixing the input type focus and reverting the enterKeyPress logic * browse path is created, need stylings,refactor,filestream selection and add * fixing the flickering issue with data refresh * new file options toggle and grid display string updates * moving code inline and using actual component * cleanup * Add file saving is done, except one styling issue with autogrowth section * add,remove working, need to edit file * add, edit, remove - all working, need css fixes and -1 fix * addressing code review comments * adding local changes adn fixing for edit file * adjusting css * addressing code review comment for using loc var instead of duplicated line of code to get the rowinfo * all fixed, need testing and refactor * vBump STS and fixing required field causing the apply button not enable for other options on main branch * fixing autogrowth radio buttons change updates * all working except some css * disabled size for filestream * fixing filegroups and filetypes scnearios, added filename validation for newfile, todo:editingNew file * added max and min values to the inputs * editing filename validation completed, all done exccept CSS * all fixed except scroll bar * edit db file header, filename enable issue fix * PR comment supporting updates for STS * initial commit * min updates * loading data into table.. adding rows * modfying addButtonsForTable method and reusing it for edit button * add empty row/remove done, edit name and other columns required and save with tests * All working except new name validation * adding validation * code review comment updates * Dialogbase addbuttons to table refactored * more typo fixes * all working except 'Remove' revist logic and delete correct row * removing fulltext index prop * adding defualt conditions to the columns checkboxes * service fix * using path.join instead of hardcoded separators * updating files is updating filegroups tabs, removing fg to file update required * fixed toggle remove button for tab;es * filegroup refactor * update filegroups and files on new fg name * final commit changes * code review updates * vBump STS to 4.9.0.26 * [Loc] update to MSSQL xlf for 8-30-2023 (#24253) * Pass through database names to enable connection cleanup (#24251) * Also re-ordered Attach entry to match context menu placement of other admin commands. * Juno: check in to lego/hb_04604851-bac4-4681-9f74-73de611d6e48_20230831153956722. (#24259) * Add database settings tab (#24260) * [Loc] update to MSSQL xlf for 8-31-2023 (#24261) * Revalidate failed IR Validation steps (#24237) * Bump STS version to 4.9.0.28 (#24264) * Send server contextualization to Copilot extension (#24230) * Send server contextualization to Copilot extension * Keep context in editor input * Remove unnecessary server context and extension service * Send context when connecting from open editor * Remove contextualization complete event * Contextualize editor after connection success * Minor clean up * Remove nested then and use async/await * Create helper function * Remove unneeded async and add comment * Encapsulate all context logic in service * Use void operator to fix floating promise * Correct return comment * Fix comment block footer (#24268) * Juno: check in to lego/hb_04604851-bac4-4681-9f74-73de611d6e48_20230901154338064. (#24266) * Adds logging to contextualization service (#24269) * Adds logging to contextualization service * Code review changes * Adds xml-langauge-features extension back (#24271) * Add advanced tab (#24267) * Update IR Revalidation button to accurate name (#24283) * add ai key to fix undefined error (#24279) * Juno: check in to lego/hb_04604851-bac4-4681-9f74-73de611d6e48_20230902183958559. (#24278) * bumping STS (#24273) * Add loading indicator when an XEL File is opened (#24274) * Add loading indicator when an XEL File is opened * Remove custom loading message * bump sts (#24288) * Update task labels for Attach, Detach, and Drop Database (#24289) * Bump STS version to 4.9.0.32 (#24291) * Consolidate 2 context endpoints to just one (#24276) * [Loc] update to mssql and sql-migration xlfs for 9-5-2023 (#24292) * Added fix to drag and drop (#24252) * Added fix to drag and drop by updating check and adding notification upon failed drag * update to connectionConfig test * Adding Query Store Service bindings on ADS side (#24167) * Checkpoint * Adding docstrings * Updating comments * Initial changes for query store dashboard (#24272) * create empty query store dashboard * add placeholder report content * cleanup * more cleanup * Handle undefined nodeInfo for top-level database objects (#24298) * do not show individual processors in managed instance (#24302) * [Loc] xlf update for 9-6-2023 (#24324) * SQL-Migration Extension Version bump-up (#24326) * Adding additional info to sample readme, updating product strings (#24290) * Fix F11 key binding (#24323) * Fix F11 key binding to non-debug mode * Disable F11 keybinding completely to toggle between full screen * Fix compile error * add button to open query store report in new tab (#24303) * add button to open query store report in new tab * addressing comments * cleanup casing in a couple files (#24333) * Mark carbon edit with begin and end tags (#24336) * Add Open file location as an option after an excel file is saved (#24331) * Add Open file location as an option after an excel file is saved * Fix test failure * Organizing code to be easier to follow (#24332) * Organizing code to be easier to follow * Adding back trampled readme changes * Bump electron from 22.3.14 to 22.3.21 (#24299) * Bump electron from 22.3.14 to 22.3.21 Bumps [electron](https://github.com/electron/electron) from 22.3.14 to 22.3.21. - [Release notes](https://github.com/electron/electron/releases) - [Changelog](https://github.com/electron/electron/blob/main/docs/breaking-changes.md) - [Commits](electron/electron@v22.3.14...v22.3.21) --- updated-dependencies: - dependency-name: electron dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]> * Update distro hash * Bump distro hash * Update distro hash * Update distro hash * Update hash --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lewis Sanchez <[email protected]> * Enabling QueryStore options to the database properties (#24255) * Initial commit * updated query capture policy props * all done except, querydiskSpaceSection * disabling capture policy options on off mode but with custom mode enabled * adding query disk usage section with purge button * fg table replace * STS vbump to 4.9.0.34 * typo * Add visible newlines to display value for Edit Data cells (#24334) * [Loc] final XLF update prior to code complete for 9-7-2023 (#24337) * Fixes a typo in headerfilter plugin (#24342) * Add support for clearing pooled connections (#24325) * Revert the changes of row fileGroup table which was added accidentally (#24339) * Bump STS version to 4.9.0.36 (#24349) * Update required indicators in Add Database Reference Dialog (#24346) * Update required indicators appropriately in Add Database Reference Dialog * Add required to the textbox as well * Remove 'Script As Alter' command and don't show for table nodes (#24352) * Juno: check in to lego/hb_04604851-bac4-4681-9f74-73de611d6e48_20230908183910058. (#24347) * Update placeholder text for mssql server name (#24350) * [Loc] update to MSSQL xlf for 9-8-2023 (#24354) * update STS to 4.9.0.37 (#24356) * Notify user to restart server (#24351) * Fix some of the issues found in the bug bash (#24348) * Fix copy on Linux (#24341) * add return type for copy results from STS * fix test error * change Result type to CopyResultsRequestResult * remove async * bump data protocol client * bump dataprotocol client version * bump version in yarn.lock * add async back * Add serverless DW platform (#24246) * Add serverlesss * Add serverless master.dacpac * vbump --------- Co-authored-by: Kim Santiago <[email protected]> * Juno: check in to lego/hb_04604851-bac4-4681-9f74-73de611d6e48_20230909153928076. (#24358) * Juno: check in to lego/hb_04604851-bac4-4681-9f74-73de611d6e48_20230910154305184. (#24360) * updating release version in main post release branch split (#24374) --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Cory Rivera <[email protected]> Co-authored-by: Sai Avishkar Sreerama <[email protected]> Co-authored-by: Alex Ma <[email protected]> Co-authored-by: [email protected] <[email protected]> Co-authored-by: Barbara Valdez <[email protected]> Co-authored-by: Ram Uday Kumar <[email protected]> Co-authored-by: Lewis Sanchez <[email protected]> Co-authored-by: Christopher Suh <[email protected]> Co-authored-by: Benjin Dubishar <[email protected]> Co-authored-by: Sakshi Sharma <[email protected]> Co-authored-by: Kim Santiago <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lewis Sanchez <[email protected]> Co-authored-by: Cheena Malhotra <[email protected]> Co-authored-by: Z Chen <[email protected]> Co-authored-by: Kim Santiago <[email protected]> Co-authored-by: erpett <[email protected]>
This PR retrieves generated context and sends the context over to Copilot.