fix(llm): set reasoning_effort for known Ollama chain-of-thought models (#2932) - #2956
fix(llm): set reasoning_effort for known Ollama chain-of-thought models (#2932)#2956amtulifra wants to merge 2 commits into
Conversation
…ls (Graphify-Labs#2932) The Gemini backend already sends reasoning_effort="low" via its static BACKENDS config, but Ollama never sent one at all. Local reasoning models served through Ollama (nemotron, deepseek-r1, qwq) narrate at length before answering without it, burning most of --api-timeout on reasoning tokens instead of the JSON reply and causing bisection failures/dropped files on real extraction chunks. Add a model-name-based resolver that sends reasoning_effort="high" for recognized reasoning models and omits it for everything else (unchanged default behaviour), with GRAPHIFY_OLLAMA_REASONING_EFFORT as an explicit override ("none"/"omit" disables it outright). Wired into both extract_files_direct's OpenAI-compat dispatch and _call_llm (the --dedup-llm tiebreaker path), which had the identical static cfg lookup and would hit the same failure mode.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Adds dynamic reasoning_effort resolution for the ollama backend via new _resolve_ollama_reasoning_effort/_model_is_ollama_reasoning_model, defaulting known chain-of-thought models (nemotron, deepseek-r1, qwq) to "high" and honoring a GRAPHIFY_OLLAMA_REASONING_EFFORT override (with none/omit to disable). Wires this into both extract_files_direct and _call_llm for ollama only, leaving other backends on their static config. Adds tests covering the resolver, env-var precedence, and both call paths.
Worth a look
- Configured Ollama reasoning_effort is ignored in direct extraction —
graphify/llm.py:2020· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 804 functions depend on the 309 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 46 callers, 14 callees - new:
extract_files_direct()— 21 callers, 21 callees - new:
extract_corpus_parallel()— 26 callers, 11 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
_call_llm()— 13 callers, 19 callees - new:
dispatch_command()— 2 callers, 119 callees - new:
_extract_with_adaptive_retry()— 22 callers, 10 callees - …and 15 more — each is listed as a finding
Verification — 804 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 528 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_call\_llm.
The verifier did not have enough to check \_call\_llm, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly TypeError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify extract\_files\_direct.
The verifier did not have enough to check extract\_files\_direct, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `root` is annotated `Path` — outside the synthesizable primitive/collection set
· 23 more finding(s) on lines outside this diff (see the check run).
Review flagged that the ollama branch unconditionally used the dynamic
resolver, discarding cfg.get("reasoning_effort") outright instead of
falling back to it. Not reachable today — BACKENDS["ollama"] carries no
such key, and a same-named custom provider can't add one (providers.json
entries matching an existing BACKENDS name are skipped) — but the
resolver silently dropping a static default if that ever changes is a
real footgun. Pass cfg.get("reasoning_effort") through as the resolver's
bottom-of-precedence default instead of bypassing it, at both call sites.
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Adds a _resolve_ollama_reasoning_effort resolver (env var > known reasoning-model default of "high" > static config) plus a _model_is_ollama_reasoning_model marker check, wired into both extract_files_direct and _call_llm for the ollama backend. This makes chain-of-thought models (nemotron, deepseek-r1, qwq) send reasoning_effort="high" so they stop burning --api-timeout on narration, overridable via GRAPHIFY_OLLAMA_REASONING_EFFORT (including "none"/"omit" to disable). Non-ollama backends keep reading the static config unchanged.
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 809 functions depend on the 314 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 46 callers, 14 callees - new:
extract_files_direct()— 22 callers, 21 callees - new:
extract_corpus_parallel()— 26 callers, 11 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
_call_llm()— 13 callers, 19 callees - new:
dispatch_command()— 2 callers, 119 callees - new:
_extract_with_adaptive_retry()— 22 callers, 10 callees - …and 15 more — each is listed as a finding
Verification — 809 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 533 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_call\_llm.
The verifier did not have enough to check \_call\_llm, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly TypeError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify extract\_files\_direct.
The verifier did not have enough to check extract\_files\_direct, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `root` is annotated `Path` — outside the synthesizable primitive/collection set
· 23 more finding(s) on lines outside this diff (see the check run).
1 participant
Fixes #2932. The Gemini backend already sends reasoning_effort="low" via its static config. Still, Ollama never sent one at all — local reasoning models (nemotron, deepseek-r1, qwq) narrate at length before answering without it, burning most of --api-timeout on reasoning tokens instead of the JSON reply.
Added a model-name resolver that sends reasoning_effort="high" for recognized reasoning models, omits it for everything else (unchanged default), with GRAPHIFY_OLLAMA_REASONING_EFFORT as an override ("none"/"omit" disables it). Wired into both extract_files_direct and _call_llm (the --dedup-llm tiebreaker), which had the identical gap.
Tested: unit coverage for the resolver (model-name matching, env override precedence, case/whitespace edge cases) and end-to-end kwargs-capture tests on both call sites; checked interaction with the existing GRAPHIFY_DISABLE_THINKING/extra_body mechanism and the dedup tiebreaker's 200-token budget (both safe — see PR discussion if asked); confirmed no other code path builds reasoning_effort independently; full test suite green (only pre-existing, unrelated environment-gap failures).