-
Notifications
You must be signed in to change notification settings - Fork 48
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
feat: Add adaptors for various backends (ollama, tgi, api-inference) #40
feat: Add adaptors for various backends (ollama, tgi, api-inference) #40
Conversation
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.
Thank you for your contribution.
Did you think about the server's response that could also be different?
I'm not sure how flexible this will be in the end. My plan is to add support to different backends in code and to differentiate with an identifier in the params object.
What API were you aiming at supporting with this PR?
Hey McPatate, my limited understanding is that a server usually responds with a text stream. If not the case I can see how that would need to be addressed, maybe for a separate PR. I feel that adding support through an identifier might be more hefty to maintain, if the project ends up having logic to handle dozens of possible backends I could see it becoming bloated. I'm still playing around with this, but I'm aiming to support usage of Ollama through llm.nvim |
Ollama's response is not supported by llm-ls currently, it needs to be handled otherwise it won't work. I wonder if we'll ever get to dozens of different backends. I believe at some point we'll stabilise on an API and people will implement "the standard" rather than re-invent a new thing. |
@McPatate haven't had a proper test yet, I'd like to get your thoughts on the implementation of the adaptors and whether you think they can be improved. Users will now be able to specify an adaptor in their configuration, which will handle the transformation of requests and responses to be compatible with the given backend. |
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.
did a quick first pass
crates/llm-ls/src/adaptors.rs
Outdated
Ok(OllamaAPIResponse::Error(err)) => return Err(internal_error(err)), | ||
Err(err) => return Err(internal_error(err)), |
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.
Ok(OllamaAPIResponse::Error(err)) => return Err(internal_error(err)), | |
Err(err) => return Err(internal_error(err)), | |
Ok(OllamaAPIResponse::Error(err)) | Err(err) => return Err(internal_error(err)), |
does this work?
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 not, since Ok(OllamaAPIResponse::Error(err))
and Err(err)
have conflicting types
Thanks for the feedback @McPatate, it's been really helpful - already looking a lot better from what you've suggested. |
Thank you for your contribution!
How have you tested your PR? If the CI passes and your testing is conclusive then we should be able to merge. We will need to update all clients to match the new API as well, is this something you'd be willing to do as well? |
I've been testing by hacking around in llm.nvim. I'll need to retest though as I haven't tested this in a good while. |
I'm trying to test this but running into an issue where it's complaining about the case of the request parameters given. I'm using https://github.com/noahbald/llm.nvim as my client here. Is this something to do with |
Yes, I've recently changed the API to make the case consistent across parameters. I haven't released a version of llm-ls yet since it's a breaking change. I was waiting for a few other things before creating a new release, like your PR and what I'm currently working on. |
@noahbald @McPatate I managed to test it locally. However, I had to copy paste all snake_case configs to camelCase local params = lsp.util.make_position_params()
params.model = utils.get_model()
params.tokens_to_clear = config.get().tokens_to_clear
params.tokensToClear = config.get().tokens_to_clear
params.api_token = config.get().api_token
params.apiTokens = config.get().tokens_to_clear
params.request_params = config.get().query_params
params.request_params.do_sample = config.get().query_params.temperature > 0
params.requestParams = config.get().query_params
params.requestParams.doSample = config.get().query_params.temperature > 0
params.fim = config.get().fim
params.tokenizer_config = config.get().tokenizer
params.tokenizerConfig = config.get().tokenizer
params.context_window = config.get().context_window
params.contextWindow = config.get().context_window
params.tls_skip_verify_insecure = config.get().tls_skip_verify_insecure
params.tlsSkipVerifyInsecure = config.get().tls_skip_verify_insecure
params.adaptor = config.get().adaptor
params.request_body = config.get().request_body
params.requestBody = config.get().request_body
params.ide = "neovim" Hopefully, the changes will get merged to main branch soon. Really excited for this one 🤠 |
Thanks for testing, the casing changes are expected at the moment, McPatate is working on that separately
|
Since I can't seem to run the CI without risking to leak the tokens to the world, I ran it locally and things seem to be broken:
Produces:
When it should be 100% |
@McPatate I'm not sure how the tests are set up here. I tried running your command but I'm getting an IO error. Do you have any advice to how I can approach the failing tests, or would you rather have a look into it on your end? |
I'd suggest using a debugger or just adding logs in the the If you're really struggling I'll take a look. I haven't completely documented |
Try setting Also, you'll need to run - -r ../../crates/testbed/repositories-ci.yaml
+ -r crates/testbed/repositories-ci.yaml With |
…t underflow crash Windows
Thanks Luc, I hadn't run a release build so llm-ls was missing from target/release.
|
Thank you very much for the hard work on this PR @noahbald, especially while putting up with my incessant pestering 😉 |
This should resolve issue #17, by allowing users to remap "input" to "prompt" add adding
{ model: "model:nb-code" }
This will require further changes on clients as well (such as llm.nvim) - happy to contribute here as well
closes #17, closes #28