Skip to content
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

Debounce, LlamaCpp support, expose prompt as setup option, fix passing parameters to model (ollama) #11

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

JoseConseco
Copy link

Hi,
This pr fixes #8

And gives u ability to customize prompt, and adds debounce delay:
image

debounce_delay - request will be send after x ms, after last key press.
And added support for Llama Server

Copy link
Owner

@tzachar tzachar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think this should be split into 2 PRs.
First one, which looks pretty good, should be the ollama part.
Second one should concentrate on the debounce implementation.

lua/cmp_ai/source.lua Outdated Show resolved Hide resolved
end)
end

function Source:trigger(ctx, callback)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not comfortable with this entire debounce concept.
First, I dont think it is need here. cmp already has a debounce implementation.
Second, I think this implementation is wrong; there should not be a global autocommand which handles the debounce.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Built in debounce in cmp - has issue where it is not working as it should:

  • to my understanding, it should show completion popup x mseconds after last key press in insert mode
  • But what it does - it shows completion x ms after typing in first letter in insert mode.
    I tested this with 2 second delay, and cmp will not wait for last key press.
    In my implementation it will wait till last key is pressed - thus it wont spin my gpu fans as much. I'm not sure if the implementation is ok, I just copied someone else code. I know that copilot - cmp extension is also using its own debounce code.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, looking at cmp sources I can only agree.

@ALameLlama
Copy link

+1 for fixing the ollama params, without this, you can't change it to a remote host.

@@ -6,21 +6,21 @@ function Ollama:new(o, params)
o = o or {}
setmetatable(o, self)
self.__index = self
self.params = vim.tbl_deep_extend('keep', params or {}, {
self.params = vim.tbl_deep_extend('keep', o or {}, {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this bug apply to the openai or bard backends as well? They similarly use params instead of o

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know, since I do not used Bard and openai. I can only assume that if they work ok, then this line is not needed.

@tzachar
Copy link
Owner

tzachar commented Jan 10, 2024

@JoseConseco
Any updates?

@JoseConseco
Copy link
Author

I will have to google, how do I split this into multiple pr - with one file per PR.

@tzachar
Copy link
Owner

tzachar commented Jan 11, 2024

I will have to google, how do I split this into multiple pr - with one file per PR.

The point here is that I do not believe debounce should be implemented inside this plugin, as cmp already implements debounce.
You have yet to convince me it is needed here. If you do manage to, we can merge this as is after addressing the other issues.

@JoseConseco
Copy link
Author

@tzachar let me know if all is ok now.

README.md Outdated Show resolved Hide resolved
@tzachar
Copy link
Owner

tzachar commented Feb 8, 2024

@tzachar let me know if all is ok now.

see pending issues in the review.

@JoseConseco
Copy link
Author

@tzachar let me know if all is ok now.

see pending issues in the review.
Ah, ok I though it is up to you to accept the changes and mark them as resolved. Will do it right now.

@mdietrich16
Copy link
Contributor

mdietrich16 commented Mar 12, 2024

+1 for fast merge. Maybe the debounce stuff should be done as a PR to cmp then?

@tzachar
Copy link
Owner

tzachar commented Mar 13, 2024

+1 for fast merge. Maybe the debounce stuff should be done as a PR to cmp then?

Waiting for the last issue to be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cannot change ollama model
6 participants