Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 13 commits
bedeb5a
906b1c8
c855638
bd3b9c8
07fd77e
cf6c99b
b75b287
994e598
555288b
5fdab8c
e1b4566
d589fb1
fc3f4ad
5b27ad1
5d36e96
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.)
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.
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).