[RFC] Add support for Ollama provider#338
Open
mchehab wants to merge 10 commits into
Open
Conversation
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>
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: |
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>
Author
|
As a reference, this is the |
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>
The logic at main.rs may actually have more completed states
than its total:
DEBUG: completed_stages=9, total_stages=9, width=20, filled=20
[Patch 1] media: em28xx: dro... | Execution Flow (turn 1) (+1 stages)
DEBUG: completed_stages=10, total_stages=9, width=20, filled=22
[Patch 1] media: em28xx: drop 'users'... | Locking & Sync (turn 1)
DEBUG: completed_stages=11, total_stages=9, width=20, filled=24
[Patch 1] media: em28xx: drop 'users'... | Reviewing...
[Patch 1] media: em28xx: drop 'users'... | Deduplication
DEBUG: completed_stages=12, total_stages=9, width=20, filled=26
[Patch 1] media: em28xx: drop 'users'... | Deduplication (turn 1)
DEBUG: completed_stages=12, total_stages=9, width=20, filled=26
Overall: [████████████████████] 133% | 12/9 stages | 24 turns
[Patch 1] media: em28xx: drop 'users'... | Reviewing...
[Patch 1] media: em28xx: drop 'users'... | Conflict Resolution
DEBUG: completed_stages=13, total_stages=9, width=20, filled=28
[Patch 1] media: em28xx: drop 'user... | Conflict Resolution (turn 1)
DEBUG: completed_stages=13, total_stages=9, width=20, filled=28
Overall: [████████████████████] 144% | 13/9 stages | 25 turns
[Patch 1] media: em28xx: drop 'user... | Conflict Resolution (turn 1)
DEBUG: completed_stages=13, total_stages=9, width=20, filled=28
Overall: [████████████████████] 144% | 13/9 stages | 26 turns
[Patch 1] media: em28xx: drop 'user... | Conflict Resolution (turn 1)
DEBUG: completed_stages=13, total_stages=9, width=20, filled=28
Overall: [████████████████████] 144% | 13/9 stages | 27 turns
This causes a crash as width-filled becomes a negative number:
Overall: [████████████████████] 100% | 11/11 stages | 30 turns
[Patch 1] media: em28xx: dro... | Implementation (turn 1) (+5 stages)
Overall: [█████████████████████
thread 'main' (2447762) panicked at library/alloc/src/slice.rs:523:50:
capacity overflow
stack backtrace:
0: 0x55b30561217a - <<std[52ccbc9b4b4e18b4]::sys::backtrace::BacktraceLock>::print::DisplayBacktrace as core[a0a13ccc4397ecab]
::fmt::Display>::fmt
1: 0x55b30562deca - core[a0a13ccc4397ecab]::fmt::write
2: 0x55b305619f32 - <std[52ccbc9b4b4e18b4]::sys::stdio::unix::Stderr as std[52ccbc9b4b4e18b4]::io::Write>::write_fmt
3: 0x55b3055edb53 - std[52ccbc9b4b4e18b4]::panicking::default_hook::{closure#0}
4: 0x55b3056089fc - std[52ccbc9b4b4e18b4]::panicking::default_hook
5: 0x55b305608bbb - std[52ccbc9b4b4e18b4]::panicking::panic_with_hook
6: 0x55b3055edc08 - std[52ccbc9b4b4e18b4]::panicking::panic_handler::{closure#0}
7: 0x55b3055e2ab9 - std[52ccbc9b4b4e18b4]::sys::backtrace::__rust_end_short_backtrace::<std[52ccbc9b4b4e18b4]::panicking::pani
c_handler::{closure#0}, !>
8: 0x55b3055eec3d - __rustc[149301a0351973cc]::rust_begin_unwind
9: 0x55b303ebcd1c - core[a0a13ccc4397ecab]::panicking::panic_fmt
10: 0x55b303ebca74 - core[a0a13ccc4397ecab]::option::expect_failed
11: 0x55b3041cd0b6 - alloc::slice::<impl [T]>::repeat::h2076c0dffcce8bdf
12: 0x55b3040edfaf - sashiko::render_progress::h0541955cd5cf889a
13: 0x55b30401d514 - sashiko::handle_review_command::{{closure}}::{{closure}}::h165c77097608c979
14: 0x55b3041eff25 - sashiko::local_review::review_single_patch::{{closure}}::{{closure}}::{{closure}}::hc8facc02e89571c9
15: 0x55b30409c143 - sashiko::worker::prompts::Worker::execute_stage::{{closure}}::he1ae86600eb08e04
16: 0x55b30407eeb0 - <futures_util::future::try_maybe_done::TryMaybeDone<Fut> as core::future::future::Future>::poll::h84a15cee
07a1edc6
17: 0x55b3041b54dc - <futures_util::future::try_join_all::TryJoinAll<F> as core::future::future::Future>::poll::hd081061f793f89
f8
18: 0x55b3041fe54b - sashiko::worker::prompts::Worker::run::{{closure}}::h461674680251a479
19: 0x55b3041ec8dc - sashiko::local_review::review_single_patch::{{closure}}::h0d3b0cd3d9218508
20: 0x55b3041ddcdf - <futures_util::stream::futures_unordered::FuturesUnordered<Fut> as futures_core::stream::Stream>::poll_nex
t::h145d4ece09d10d83
21: 0x55b304160df5 - <futures_util::stream::stream::next::Next<St> as core::future::future::Future>::poll::hae7eb5e448e10f26
22: 0x55b304019aa7 - sashiko::local_review::run_worker_in_worktree::{{closure}}::h8d15c424eb97e326
23: 0x55b30400edec - sashiko::local_review::run_worker::{{closure}}::hf516081af4ea7f43
24: 0x55b3040280c4 - sashiko::main::{{closure}}::hea950edadefa9f7a
25: 0x55b3040088d7 - tokio::runtime::park::CachedParkThread::block_on::he9c7ef757edf9eac
26: 0x55b3041af8d3 - tokio::runtime::runtime::Runtime::block_on::h8a019604f34765ef
27: 0x55b3040f2609 - sashiko::main::hb75d07e27cdb81f7
28: 0x55b3041c5c13 - std::sys::backtrace::__rust_begin_short_backtrace::h8cfefc670e2afde0
29: 0x55b3041e11c9 - std::rt::lang_start::{{closure}}::h2724112c1b27a7b4
30: 0x55b305607464 - std[52ccbc9b4b4e18b4]::rt::lang_start_internal
31: 0x55b3040f7085 - main
32: 0x7f62e6cd2681 - __libc_start_call_main
33: 0x7f62e6cd2798 - __libc_start_main@@GLIBC_2.34
34: 0x55b303ebcfe5 - _start
35: 0x0 - <unknown>
Prevent the crash by limiting the maximum size for the scrollbar.
I suspect that the root cause is due to retries:
2026-07-19T23:05:03.356421Z INFO sashiko: AI review retry (attempt 2/3)
2026-07-19T23:05:03.356424Z INFO sashiko::local_review: Restarting AI review for patch 1 (attempt 2/3)...
That have been accounted as completed.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Author
|
This is the result of running Sashiko with Ollama with this config file, on a rDNA4 GPU with 16GB VRAM: Config fileOllama run |
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: |
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>
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.
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.
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?
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
2 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