Conversation
Signed-off-by: aanokh <[email protected]>
Assisted-by: Codex Signed-off-by: Nigel Jones <[email protected]>
Assisted-by: Codex Signed-off-by: Nigel Jones <[email protected]>
Assisted-by: Codex Signed-off-by: Nigel Jones <[email protected]>
Assisted-by: Codex Signed-off-by: Nigel Jones <[email protected]>
Assisted-by: Codex Signed-off-by: Nigel Jones <[email protected]>
psschwei
left a comment
There was a problem hiding this comment.
Review of the Ollama bundled-adapter path. The uncertainty/aLoRA flow this PR tests looks correct, but the generalization to the other adapter functions has gaps, and three of them fail silently with a schema-valid score rather than raising. Details inline; openai.py already has the correct version of most of these.
…computing#1634) Fixes 8 findings from psschwei's review plus AngeloDanducci's suggestion: - Fold extra_body.documents into a message in _generate_from_intrinsic; Ollama's chat SDK has no extra_body passthrough, so answerability, citations, hallucination_detection, clarify_query, and find_context_attributions were sending zero documents. - resolve_adapter now raises if name has no adapter_models entry, instead of registering successfully and letting generation silently fall back to the base model. - resolve_adapter picks LoRA vs aLoRA from the catalog's adapter_types instead of hardcoding aLoRA; restricted context-attribution, citations, and hallucination_detection to LoRA-only (verified against the Hub). - reroute_to_alora now follows a new default_to_constraint_checking_alora flag (default True), matching OpenAIBackend/LocalHFBackend, instead of only firing for ALoraRequirement. - _generate_from_context now awaits do_generate_walk before the Requirement/Intrinsic dispatch, matching the other backends. - Fix Path(__file__).parents[2] -> parents[3] in the example script. - Constrain granite4.1:3b's context in CI before the adapter build step; only granite4.2:3b was constrained, so the new bundled tag inherited the unconstrained default and reintroduced the CI-stall risk. - Fix export VAR="$(cmd)" masking command-substitution failures under set -e in the test runner script, the build script's own usage comment, and docs/docs/advanced/intrinsics.md. - test_ollama.py's adapter-build fixture now skips on build failure instead of erroring. Added regression tests for the resolve_adapter and document-forwarding fixes; updated two existing unit tests that had encoded the old (buggy) unconditional-resolve behaviour. Assisted-by: Claude Code Signed-off-by: Nigel Jones <[email protected]>
psschwei
left a comment
There was a problem hiding this comment.
There's a conflict, but otherwise LGTM
Signed-off-by: Nigel Jones <[email protected]>
|
Will leave ready to merge so @jakelorocco can review on his return |
| "add one to `adapter_models` before resolving it." | ||
| ) | ||
|
|
||
| metadata = fetch_intrinsic_metadata(name) |
There was a problem hiding this comment.
Doesn't this mean that you can only add ollama adapters that are known to our internal catalog?
There was a problem hiding this comment.
Sort-of
- if you use adapter_models={…} then the ollama support is the same as OpenAI and hugging face, and it does rely on an entry in the catalog
- You can use add_adapter() on all backends as this doesn’t use the catalog, but you lose the simplity/consistency
- There was an old CustomIntrinsicAdapter, and that function was lost in the refactor
- We should probably open up an issue to create a proper custom adapter registration api
- So I would suggest it’s not a gap with this pr in itself, rather something more we need to improve (and maybe patch up the CustomsIntrinsicAdapter removal regression?)
jakelorocco
left a comment
There was a problem hiding this comment.
a few additional thoughts
| if rewritten.extra_body is not None and rewritten.extra_body.documents: | ||
| rewritten = move_documents_to_message( # type: ignore[assignment] | ||
| rewritten, "string" | ||
| ) |
There was a problem hiding this comment.
Doesn't the rewriter handle this internally? The io.yaml should specify this. If it doesn't, then it's incompatible with ollama and we should throw an error / handle this in the io.yaml conversion.
There was a problem hiding this comment.
Not what you suggested — found a different, real bug instead.
Before this fix — silent failure:
find_citations,flag_hallucinated_content, etc. ran without error and returned a result.- That result was always empty. No citations, no flags, ever, on Ollama.
- Nothing indicated anything was wrong — it looked like the documents just had nothing to say.
If we'd thrown an error instead (your suggestion) — loud failure:
- Same functions would raise instead of returning an empty result.
- Trades "silently useless" for "doesn't work at all" — worse for the user, since these are meant to work on Ollama.
After this fix — working:
- Same functions now return actual citations / actual hallucination flags, correctly.
Root cause: two parts of the code disagreed about where the documents ended up in the request. One part moved them into the message; the other part still looked for them in the old spot, found nothing, and returned an empty (but valid-looking) result. Fixed by making both parts agree.
Side note: io.yaml is the right long-term owner of this setting, but that's upstream/published config, not ours to change here.
There was a problem hiding this comment.
actually, is the io.yaml not handling this correctly right now? I do see for citations (and hallucination detection), the docs get added using roles which appears to be the correct way to send docs to ollama: https://huggingface.co/ibm-granite/granitelib-rag-r1.0/blob/main/citations/granite4_micro/lora/io.yaml#L92.
There was a problem hiding this comment.
- We pin
citationsto a fixed revision + thegranite-4.1-3bvariant. That file has nodocs_as_messagekey. - Your link is
main+granite4_micro— different variant, different (newer) content. - Checked
main'sgranite-4.1-3btoo — still missing the key there as well. Not just a stale pin. - So
rolesis the right fix, just not present yet where we actually pull from. - Our code-side default handles it either way — becomes a no-op once upstream adds the key.
- Worth asking upstream to add it to
granite-4.1-3btoo.
| await asyncio.to_thread(self.resolve_adapter, adapter_name) | ||
| alora_req_adapter = self._find_adapter(adapter_name, search_types) |
There was a problem hiding this comment.
Does this need to be guarded with a try-catch so that a failure when trying to reroute doesn't cause LLMaJ to fail?
There was a problem hiding this comment.
Done as suggested. Resolve is now wrapped in try/except — a failure falls back to regular generation instead of killing the whole call.
| top_logprobs = model_opts.pop("top_logprobs", None) | ||
|
|
||
| # each adapter function is served by its own ollama model tag | ||
| model = self._adapter_models.get(action.intrinsic_name, self._model_id) |
There was a problem hiding this comment.
Does the above find_adapter call ensure that the adapter exists and that we don't just fall back to the base model? If so, can we add a comment here?
There was a problem hiding this comment.
Done as suggested. Raises instead of silently falling back to the base model — same guard resolve_adapter() already has, extended to generation time.
| alora_req_adapter is None | ||
| and reroute_to_alora | ||
| and adapter_name in self._adapter_models | ||
| and not explicit_types |
There was a problem hiding this comment.
Should we check that if there is an explicit type here, that it's alora?
There was a problem hiding this comment.
Done as suggested. An explicit override that excludes aLoRA now skips the resolve — resolve_adapter() has no way to request a specific type, so resolving there could register the wrong one. An override that includes aLoRA still resolves, since thats exactly what a cold resolve would produce anyway. Added tests for both cases.
Fix three issues from jakelorocco's review of the Ollama adapter-function support: - _generate_from_intrinsic() looked up the adapter's model tag via self._adapter_models.get(name, self._model_id), silently falling back to the plain base model for an adapter registered directly via add_adapter() (bypassing adapter_models). The rewriter still built the adapter's activation prompt and enforced its response schema, so the base model returned a schema-valid but meaningless answer with no adapter weights behind it. Now raises instead, matching the guard resolve_adapter() already has for the same failure class. - The opportunistic resolve_adapter() call in the automatic requirement-check reroute path was unguarded; a network or config error during resolution killed the whole generate call instead of falling back to regular generation as intended. Wrapped in try/except. - That same reroute path skipped the opportunistic resolve whenever any explicit adapter_types override was given, even one that included aLoRA and would have been satisfied by the resolve. Now only skips when the override excludes aLoRA. - Investigating the docs_as_message question surfaced an unrelated bug: the result processor was constructed from the adapter's unmodified io.yaml config while the rewriter separately folded documents into the message, so the two disagreed about where documents lived. Citations and hallucination flags decoded as empty on every call, silently. Fixed by forcing docs_as_message onto the config before constructing both the rewriter and the result processor, and removed the now-redundant manual fold. Updated test/backends/test_ollama_intrinsics_unit.py to match: replaced test_adapter_model_tag_defaults_to_model_id (asserted the old silent fallback) with test_generation_without_configured_tag_raises, defaulted the adapter fixture to a configured tag so unrelated tests aren't affected, and added coverage for the explicit-adapter-types resolve behaviour. Assisted-by: Claude Code Co-Authored-By: Claude Sonnet 5 <[email protected]> Signed-off-by: Nigel Jones <[email protected]>
|
@jakelorocco this is another candidate for our release. |
…er path Upstream's send_to_queue() (generative-computing#1631) now takes the ModelOutputThunk directly instead of a bare queue, to stamp TTFB at provider receipt. The adapter function code path added by this PR still passed output._gen.queue, which broke type-checking after merging upstream/main. Assisted-by: Claude Code Co-Authored-By: Claude Sonnet 5 <[email protected]> Signed-off-by: Nigel Jones <[email protected]>
Pull Request
Issue
Supersedes #1622. Related: #1633.
Description
This takes over the Ollama adapter-function work originally proposed in #1622. The original implementation commit remains in this branch history. The takeover rebases the feature onto the current composed-adapter API and adds a reproducible test model built from pinned official Granite artefacts instead of depending on a user-published model.
ServerMediatedBindingregistration.ALoraRequirementrouting for configured catalogue adapter models.For one adapter function, use its bundled aLoRA tag for both
model_idand the adapter route, withadapter_base_model_nameset to the matching Hugging Face base-model directory. Before its invocation tokens appear, the bundle behaves as the base model and retains one Ollama model identity. Applications that map several adapter functions to separate Ollama tags cannot share a KV cache across those tags; multi-adapter, single-model serving remains a Granite Switch use case.The broader server-mediated lifecycle and telemetry design remains tracked in #1633.
Testing
ruff,mypy, Markdown lint, shell syntax, and whitespace checks pass.Attribution
Adding a new component, requirement, sampling strategy, or tool?
NOTE: This PR supersedes an already acknowledged contribution in #1622.