[RFC] Add support for Ollama provider#338
Closed
mchehab wants to merge 11 commits into
Closed
Conversation
Contributor
Author
|
With this PR, Sashiko no works with Ollama. On this case, I asked it to review a very simple patch that it is adding an entry to MAINTAINERS file, using That's And this is Sashiko review output: |
Contributor
Author
|
As a reference, this is the |
Contributor
Author
|
This is the result of running Sashiko with Ollama with this config file, on a rDNA4 GPU with 16GB VRAM: Config fileOllama run |
Contributor
Author
On a side note, the overall bar doesn't reflect the number of actual stages. The last patch on this PR (694728f) fixes an issue to prevent Sashiko to crash when: |
Collaborator
|
Hi Mauro, Thank you for the PR. Please fix the linting/build-time errors with Furthermore, could you squash the fixup commits Additionally it would be great if you could add a commit that included documentation changes to |
Comment on lines
+132
to
+136
| let llm_concurrency = if concurrency < 2 { | ||
| 1 | ||
| } else { | ||
| std::cmp::max(1, concurrency * 3) | ||
| }; |
Collaborator
There was a problem hiding this comment.
could you replace this code block with something like?
let llm_concurrency = match concurrency {
0 | 1 => 1,
n => n * 3,
};
edit: on further thought, I don't like the idea of restricting users who want only a single review to be running at a time to be limited to such LLM calls. please split this into a separate configuration.
Contributor
Author
There was a problem hiding this comment.
Ok. any suggestions for the new name? perhaps gpu_count?
Collaborator
There was a problem hiding this comment.
from configuration.md
### `[review]`
| Key | Type | Default | Description |
|-----|------|---------|-------------|
| `concurrency` | integer | -- | Number of concurrent reviews. |
llm_concurrency looks like it could work.
Contributor
Author
There was a problem hiding this comment.
Not sure what you meant. There are two possible ways to limit it:
-
mean concurrency
The
concurrencyparameter is actuallyconcurrency * 3, because there are several steps that are sequential (from its comment). The idea behind as the logic there tries to ensure that it would look to the caller that the concurrency criteria would be used, but in a matter of fact, it sounds to be just a gestimation.IMO, a better name for such parameter would be
mean_concurrency, but renaming it would be an incompatible change. -
maximum concurrency
When we have local runners, the maximum concurrency should be, instead, the number of available GPUs, as the same GPU can't usually execute multiple jobs in parallel
There is a local var called llm_concurrency associated to the mean concurrency. Are you proposing to rename currency to llm_currency?
Alternatively, we could add a new parameter max_concurrency for (2), keeping (1) with the same behavior.
Collaborator
There was a problem hiding this comment.
hmm, sorry, I think we are misunderstanding each other here. but you raise a valid concern -- concurrency is a bit overloaded in the codebase.
anyway, what I mean is binding the value of local variable llm_concurrency (used in creating llm_semaphore) itself to a user-driven config item. we don't need to change its name; leaving the variable as-is (llm_concurrency) and documenting what it does should be sufficient.
| } else { | ||
| 0 | ||
| }; | ||
| let filled = filled.min(width); |
Collaborator
There was a problem hiding this comment.
this (related pr: #342) feels a little hacky. I would prefer a fix that takes into account retries and handles the overflow as in #341
-- @rgushchin maybe that should be merged prior to this fix?
Contributor
Author
There was a problem hiding this comment.
I'll rebase this PR once #341 is merged.
Contributor
Author
|
I addressed lint issues. Will add llm_concurrency parameter later. The actions test may fail for Ollama, as for them to work, a local ollama instance would be needed. Not sure what's the best alternative to address it. Perhaps actions could download an Ollama container and run the tests on CPU. Alternatively, Ollama could be configured to use cloud services. |
Signed-off-by: OllieinCanada <73385593+OllieinCanada@users.noreply.github.com> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
The OpenAI compatible model doesn't work with Ollama. Add another provider for it. The code there was generated from a prompt to modify src/ai/openai.rs to use an Ollama-compatible API. Assisted-by: qwen2.6_claude_27B Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Ollama needs to receive num_ctx at the generate request, according with: https://github.com/ollama/ollama/blob/main/docs/api.md If we don't do that, it will use the default from ollama serve parameters. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Those LLM-generated comments aren't useful. Drop them. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Make it similar to openai.rs code. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Drop uneeded comments and try to use the same coding style. No functional changes. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Properly map error codes from: https://docs.ollama.com/api/errors Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
As local runs may take quite a while, add an option to adjust the think mode if needed. The Ollama API accept this value as either a String or as a boolean. Implement it as string. While here, drop an obvious commented added by LLM. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Ensure that, when running on local GPUs, it won't be piling parallel concurrencies that may end timing out because the GPU is busy with a previous job. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Member
|
Merged into main via rebased commit series b106921. |
Collaborator
this is an existing challenge we have -- testing all the possible harness + model configurations is difficult and time consuming -- so we rely on users to help us out here with bug reports and PRs...
as a side note: has the performance of running models (even tiny ones) improved at all? IIRC they were very slow. the pipeline already takes a considerable amount of time to build and test. |
Contributor
Author
Performance testing a single patch is acceptable. That can help testing changes on custom prompts, with is currently my main goal. I'm currently preparing to measure performance on some machines that has some older but more high-end GPU models. I may have a better view after running such tests. |
Collaborator
agh, I forgot to type "on CPU" I also regularly test locally on my 9070xt -- but it isn't enough to consistently run sashiko without the occasional crash. |
Contributor
Author
I'm currently preparing to measure performance on some machines that has some older but more high-end GPU models. I may have a better view after running such tests. Anyway, you can easily test CPU only with Ollama:
|
Contributor
Author
It occurred to me that running on ARM servers with lots of CPU cores using only CPU could actually be decent to review big patch series. I'll try running it on one such server and see how it behaves. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
The OpenAI compatible model doesn't work with Ollama. Add another provider for it.
Assisted-by: qwen2.6_claude_27B