Repository navigation
fix: fold Ollama's timeout-kwarg handling into the provider confi - #220
Mr-Neutr0n wants to merge 3 commits into
Conversation
…ovider confi Signed-off-by: Mr-Neutr0n <64578610+Mr-Neutr0n@users.noreply.github.com>
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Configure provider timeout arguments backend/app/services/llm_providers.py, backend/tests/services/test_llm_providers.py |
ProviderConfig now specifies the timeout argument location. apply_timeout uses that setting, defaults to the top-level timeout argument when no provider is configured, and has tests for both argument shapes. |
Apply provider timeout configuration backend/app/services/chat_model.py |
build_chat_model now calls apply_timeout instead of handling Ollama timeouts in a provider-specific branch. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Refactor
Suggested reviewers: brylie
Merge Risk: 
Minimal · up to da3a5
-
Ollama timeout handling is preserved; no merge-blocking issue was identified.
Pre-merge checks: 
5
-
Full details: Linked Issues check
-
Explanation
[
#164] The three checklist items are implemented.- Field added:
ProviderConfig.timeout_kwarg: str | None = DEFAULT_TIMEOUT_KWARG("timeout") inbackend/app/services/llm_providers.py. Theollamaentry setstimeout_kwarg=None, which means nested underclient_kwargs. Openai and anthropic use the default top-leveltimeout. build_chat_modelinbackend/app/services/chat_model.pynow callsapply_timeout(provider, kwargs, timeout)whereprovider = PROVIDERS.get(provider_name). The_OLLAMA_PROVIDERconstant and theprovider_name == _OLLAMA_PROVIDERcomparison are removed from that file.- Behavior is preserved, checked by code reading of the diff:
- ollama:
provider.timeout_kwarg is None, sokwargs["client_kwargs"] = {"timeout": timeout}. Same as before. - openai and anthropic:
timeout_kwarg == "timeout", sokwargs["timeout"] = timeout. Same as before. - Provider with no table entry:
provider is None, so the default"timeout"applies. Same as before.
- ollama:
The new tests in
backend/tests/services/test_llm_providers.pycoverapply_timeout. I did not read that test file directly. Its existence and the 300-pass result come from the PR description, not from my own run. This does not change the verdict, because the requirement is structural and the code paths above are established from source. - Field added:
Full details: Out of Scope Changes check
-
Explanation
All changed source hunks map to
#164.llm_providers.py:DEFAULT_TIMEOUT_KWARG, thetimeout_kwargfield and its comments, theollamatable entry, andapply_timeoutimplement the requested table-driven timeout wiring. The module docstring addition references the same issue.chat_model.py: the import ofapply_timeout, the removal of_OLLAMA_PROVIDER, and the replacement branch inbuild_chat_modelimplement the requested refactor.test_llm_providers.py: tests for the new helper support the objective.
No unrelated behavior change appears in the reviewed hunks. The PR description's notes about test suites and
misetasks document the author's verification. They do not add code changes.Unresolved: I did not search the wider repository for other imports of the removed
_OLLAMA_PROVIDERconstant. The removal is in scope, and the PR reports a passing suite, so I found no evidence of a problem.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 3 files.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
|
CI Results
All checks passed. 🎉 |



Fixes #164
Folded Ollama's timeout-kwarg quirk into the
ProviderConfigtable via a newtimeout_kwargfield and anapply_timeouthelper inbackend/app/services/llm_providers.py, and madebuild_chat_modelinbackend/app/services/chat_model.pyread the wiring from the table instead of comparing against the removed_OLLAMA_PROVIDERconstant. Behaviour is unchanged, including the fallback to the plain top-leveltimeoutkwarg for a provider with no table entry. New tests inbackend/tests/services/test_llm_providers.py.Verification:
uv run --directory backend pytest(asCONTRIBUTING.mddocuments) — 300 passed. The earlier "No local test suite detected" note was wrong:backend/,crawler/,ingest/andregister/each have a suite and there aremise run test:*tasks.This change was prepared with AI assistance under human direction and review.
Summary by CodeRabbit