-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add support for copilot #6865
Add support for copilot #6865
Conversation
Copilot should not be in core, even as a band-aid fix until the plugins system arrives: #4037 (reply in thread) I would instead suggest splitting the copilot code into its own LSP in its own repo. |
Can you explain more what you are proposing? The copilot server sends and receives requests that are not standard to microsoft's lsp, for example 'getCompletionsCycling'. So you would still need to add functions in helix_lsp::client to send/receive this. Unless you tried wrapping the copilot server to make it lsp compliant, and then try use helix's completion functionality to display a copilot completion, I don't think this would work that nicely though, and you would also loose out on eg rust-analyzers own completions if you use copilot + rust-analyzer, is this what you mean? |
Yes that's the best way to add copilot to helix
With #2507 you can use both at the same time |
Hmm, I've encountered a few non-standard compliant lsp features for different language servers, I'm not sure how we should generally handle these features, ignoring all of them doesn't seem to be a good long-term solution to me. Anyway, as I was interested in Copilot with helix as well, what are the non-lsp-compliant features? Can they somehow be proxied via a dedicated language server, or are these too fancy (e.g. open a separate buffer with a chat window or something like that)? |
If this ends up going in then the other kids on the block i.e sourcery, tabnine, codeium etc should probs get write-ups on setup too (if feasible..) I believer they almost all have an lsp version as copilot does. |
I think you could wrap the copilot server to make it lsp compliant, and treat it as a completion capable lsp server, but I don't think this would be very usable. I dont think id actually use copilot if it was implemented like this tbh. Its like this because editor lsp clients are setup to receive simple completions like function calls and variable names, where as copilot is meant to do this and more- also write whole blocks of code and fill in structs etc... so there are more points where you want to get completions when using copilot. In my pr helix's lsp-client sends copilot completion requests when the document changes (at the same point when we emit the textDocument/didChange notification), but I think you actually want to request more frequently than this- anytime you move the cursor in insert mode, or anytime you enter insert mode. Also this point is weaker but I dont think it would be ergonomic to put the copilot completions within the same menu as eg rust-analyzers completions (btw in your pr, if I'm using two language servers that give completions, eg in the config language-servers = ["copilot", "rust-analyzer"]), and copilot was this lsp compliant wrappper, would it show me the copilot completions above the rust analyzer ones?) These are some of the non lsp standard types requests/responses - https://github.com/TerminalFi/LSP-copilot/blob/master/plugin/constants.py , https://github.com/TerminalFi/LSP-copilot/blob/master/plugin/types.py .
Maybe if we also wanted to get other non-lsp-standard language servers to work with helix then the best way would be to build a mini plugin system to only solve this problem, then remove it when the main plugin system comes? |
Yeah -- I'm not actually one of the 'zomg we have to put copilot in here' crew, just mainly wanted to give a shoutout to its competitors. The frequency of the calling the LSP is actually a really interesting point because it's potentially inviting a performance black whole that maybe goes against the goals/principles of the editor... |
There is the LSP standard option to mark completion lists as incomplete which causes the client to request the completion list on every keypress. Right now this is now implemented in helix but as this is part of the LSP standard that feature will be added in the future.
completion requests are sent on the idle timeout in insert mode which is pretty close to "anytime you move the cursor or type anything (in insert mode)" (just debounced)
No that is still a lot of work. We are working on the plugin system but won't invest large effort into holdover solutions. |
yeh that would improve it as long as it really is any keypress (specifically space)
its not that close really (not enough for copilot), my examples above show this For me there is also another drawback of using a wrapper which is that all completions that copilot sends will be displayed, I dont like getting spammed by copilot like this, in my pr i made it so a completion only gets shown if you press a keybinding (C-n). I dont think I'll attempt the wrapper because of this and the other reasons and will just use my fork.
Yeh thats fair |
I don't think you quite understood what I mean. I was talking about the initial completion requests open the autocomplete popup. With an incomplete list the completion requests are sent on every keypress (with a short debounce to not create too much traffic but the vscode copilot integration works the same) if the popup is open. If the popup is open you don't need to constantly send requests to the server because the completions aren't shown anyway. What I was talking about with the idle timeout is just that a completion request is sent to the server on idle timeout to automatically open a completion list. In your code you don't even do that the copilot completion list is just bound to a manual keybinding (as you described) so I don't think that mechanism matters to you. If you use an incomplete completion list (and the support for that lands in helix) it should work exactly as your fork does. The only thing missing would be a way to trigger completion only for a specific server instead of all of them (so you can trigger copilot and normal completions separately) that seems like a reasonable feature request to support in core IMO since that could also be useful with other LSPs and shouldn't add much complexity. Afterwards implementing a wrapper that behaves exactly the same as this fork should work well. Ofcourse it makes sense for you to keep using this fork (as it's quite a nice integration) but that's how I would integrate copilot into core since we don't add support for specific services/programs into helix (just generic concepts like LSP, DAP or "a shell"). Of course, a plugin could also be an option but I think what you have implemented here would be possible without plugins. |
yeh that makes sense, its just I thought you were making your second point separately to your first I think there are still a few use cases that wont work with this future client feature you are talking about though- the most critical imo is- the server sends a completion list marked incomplete, the user accepts one of these completions. It makes no sense for the client to now send another completion request right?, so i dont think you guys will implement this behviour into this future feature? but you would need the client to do this to make it work nice with copilot in this common situation- you are filling in a struct instance and copilot is feeding you the fields line by line, so you are accepting then pressing enter, then accepting again etc. so you will kill the completion request train by accepting, then since pressing enter keypress wont trigger a completion request you wont be able to get copilot to immediately fill in the next line. also another one is when you enter insert mode and immediately want a suggestion (this doesnt happen in my pr atm but i can add this) also are you sure the client would rerequest on any keypress (after receiving a incomplete marked completion list) ? it doesnt make sense to do so on pressing Esc or Enter, but what about space? and what about { ? (typing { doesnt trigger a completion request)
Yeh that would definitely make it better. In the long term I dont think it would beat a plugin though, there are things like the copilot completion panel and key gen for signin and (imo) superior completion ui that you cant do with the wrapper. |
Could you compromise and implement those features you think won't be possible through the LSP spec as LSP extensions? That way others can benefit from having access to copilot, while you can maintain a personal fork that functions the way you want. Of course you don't have to, and it seems like you're happy with the current implementation for your own use. |
If this could be of any help, Zed editor authors wrote an interesting blog post about how they integrated Copilot also using an LSP approach. |
Did somebody try out the lsp from zed? |
The package from zed just extracts the lsp from |
hey @AlexanderDickie, this is superb. Can you share more on how to create the copilot binary?
adding to PATH is easy, but dont know how to make it executable, nor did I find a binary... Do I need vim or nvim installed? |
Been using this for a little while, great work! I do wish it autocompleted like vscode and vim do (probably using the same system that inlay hints uses), but hitting Ctrl-n still works for me! @luccahuguet what I did was make an executable bash script called #!/bin/bash
node ~/.cargo/bin/agent.js and put it, along with agent.js, in my |
yes thats pretty much what i did, you can also download this https://github.com/zbirenbaum/copilot.lua/tree/master/copilot , and rename index.js copilot. I'll probably add some things in the next few weeks mainly what I described in my initial post in the 3rd paragraph from the bottom, could also add a option for automatic preview. |
took me a while but got it working thanks to your help, thanks ppl! the only difference is that I created a copilot.sh file and then symlinked it to just copilot ( I did not know how to do it another way) |
Yeah, exactly! Though in the meantime C-n works great too |
I cannot get it to work probably cos it cannot find the key, how do you generate the I installed copilot on VSCode, signed in into github and copilot works fine in VSCode, but it won't generate that folder. |
Solved: If anyone is having the same issue, you'll need to setup copilot with nvim/vim via |
Thanks for the details, that was very helpful ! |
Enable copilot for your language as shown in the description, then write something like: // write a function that sums two numbers[Press C-n here] copilot should fill with a stub, then press |
alright, here is my languages.toml configuration
I basically added a copilot language server and added to typescript and svelte the "copilot" language server. |
@happysalada (nice handler btw), so I'm not sure if what you are sharing is your whole config or not, but this is what I'd do with minimal steps to avoid distractions:
Create a simple config in your [language-server.copilot]
command = "copilot"
args = ["--stdio"]
[[language]]
language-servers = ["typescript-language-server", "copilot"]
name = "typescript" Create a sample TS file such as function sumTwoNumbers//[PRESS C-n HERE] right after the `s` character If that works, then you can add it to your other languages, lsps, etc. 👍🏼 |
Thanks again for the help! I was missing the I took some additional steps
I've doubled checked everything at this point, I'm sure I missed something obvious, I'll revisit this in 6 months. btw, I'm not sure if it's because I'm on a mac, but 'Ctrl-n' only writes n for me, it never did anything. |
Well that means that the key is not getting captured, question: When you switch into this branch, Are you re-compiling helix? cargo install --path helix-term --locked |
@AlexanderDickie
I understand that the PR cannot be merged until the plugin system is released, but I love the ability to use Copilot in Helix. I've made some changes and created a PR to your fork with fixing all the mentioned issues. Please review it, as it significantly improves the developer experience of using Copilot. If anyone tries my PR and finds any issues, don't hesitate to reach out to me. |
being able to add/subtract positions is very handy when writing rendering code
The line annotation as implemented in helix-editor#5420 had two shortcomings: * It required the height of virtual text lines to be known ahead time * It checked for line anchors at every grapheme The first problem made the API impractical to use in practice because almost all virtual text needs to be softwrapped. For example inline diagnostics should be softwrapped to avoid cutting off the diagnostic message (as no scrolling is possible). While more complex virtual text like side by side diffs must dynamically calculate the number of empty lines two align two documents (which requires taking account both softwrap and virtual text). To address this, the API has been refactored to use a trait. The second issue caused some performance overhead and unnecessarily complicated the `DocumentFormatter`. It was addressed by only calling the trait mentioned above at line breaks (instead of always). This allows offers additional flexibility to annotations as it offers the flexibility to align lines (needed for side by side diffs).
This commit brings the text decoration API inline with the LineAnnotation API (so they are consistent) resulting in a single streamlined API instead of multiple ADHOK callbacks.
…updating the completion state, and no longer effects rendering
Ive rebased on the latest master and changed the conditions when completions are requested (the condition is now anytime that you enter insert mode and anytime the document changes while in insert mode). Ive also refactored the should render/should request a completion logic to make it easier to reason about, this has removed a couple of race conditions. I'll rebase again when #6417 is merged if not sooner |
Hi thanks for your ongoing work 🤝 just fyi the copilot-vim repo changed the name of the agent.js file to language-server.js |
I don’t think theres much point having this pr open anymore since it won't be merged, I'll still maintain/ rebase my fork until the plugin system can implement copilot. I'll also update the readme of my fork with the setup instructions. Please feel free to make any issues/ prs in my fork 🫡 |
Builds on the multiple-lsp pr - #2507 , adding the ability to use copilot, will solve #4037 .
I don't think that copilot should be built into the core, I think it should be a plugin, but this will do until the plugin system arrives?
You need copilot on the your path, download it from copilot.vim and make it executable.
Then in languages.toml eg -
The copilot server actually handles authentication, so all you need is a key in ~/.config/github-copilot , if you've used copilot in another editor you'll have this key so dont need to do anything.
How it works-
I've added additional state to the Document struct. When the document changes, we spawn a thread that sends a completion request to the copilot server, and then stores the response in this new document state.
Then when you press C-n, if we have a response in the doc's copilot-state, translate the CopilotCompletionResponse to a vec of transactions, then push the CopilotPicker component.
Then cycle through the transactions using C-n, C-m, this just works by undoing and applying the transactions. Then Enter to simply leave the picker, or Esc to undo the current transaction then leave.
We could remove the additional document state and send the request and receive the completion response upon launching the picker, but this is slower (adds the time between finishing typing and pressing C-n, which is a good fraction of time taken to send+receive the completionResponse).
What I'll add next is the ability for the picker to be receive new completions upon being launched. For example you type '// function to sum vec of ints', then very quickly press C-n, we may not have received the completion before we launch the picker, so the picker needs to see these new transactions after it has launched- atm you would have to C-n again to relaunch the picker.
Also would be nice for the picker to view the completions in a different color before accepting, similar to other editor copilot plugins.
Also need to add additional types to copilot_types, for things like the logs that the copilot-lsp sends, which arent lsp standard.