fix(providers/ollama): return LLMResponse from generate_with_usage, matching all other providers#193
Merged
himanshu231204 merged 1 commit intoJul 17, 2026
Conversation
…atching all other providers (fixes OpenAgentHQ#78) Both Ollama.generate_with_usage (sync) and generate_with_usage_async returned tuple[str, TokenUsage] while every other LLM provider (OpenAI, Gemini, Groq, Mock, OpenRouter) returns an LLMResponse. This broke polymorphic use (e.g. the pipeline reads response.content/.usage/ .latency_ms) and dropped provider/latency metadata. Both methods now build and return an LLMResponse populated like the sibling providers: content, model, usage, provider, and a latency_ms measured with time.monotonic() around the HTTP call. Tests: add sync + async LLMResponse-contract tests (type and every field) plus model-override and connection-error coverage, all with a mocked HTTP client (no live ollama server). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🎉 Congratulations @Nitjsefnie! Your pull request has been successfully merged into main. 🚀 Thank you for contributing to OpenAgentHQ and helping improve the project. We truly appreciate your contribution and hope to see you back with more amazing PRs! Happy Open Sourcing! ❤️ |
Member
|
Hi @Nitjsefnie , Thank you for your contribution to OpenAgentHQ. We truly appreciate your time and effort in helping improve the project. We've recently launched a new open-source project, ModelDock—a package manager for local AI models. We'd love to have you contribute and help shape it as well. 🚀 https://github.com/OpenAgentHQ/modeldock Looking forward to your contributions! |
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.
Fixes #78.
Both
generate_with_usage()(sync) andgenerate_with_usage_async()in the Ollama provider now returnLLMResponse— content, model, usage, provider,latency_ms(monotonic-clocked around the HTTP call) — mirroring the exact construction pattern in the OpenAI and Groq providers. Docstrings updated; the sync one's stale "use generate() directly" note now points at the async variant.Call-site inventory (why nothing else changed)
core/pipeline.py:196already reads.content/.usage/.latency_ms— it assumedLLMResponseall along; this fix makes Ollama conform.text, usage = ...) consumer of these methods exists anywhere in src, tests, or examples (grepped).generate/get_token_count, so no ABC signature to realign.Tests
5 new mocked-HTTP tests (
TestOllamaGenerateWithUsage): full field assertions for sync + async, model-override for both, and async connection-error behavior. Mutation-verified: withollama.pyreverted to the tuple version, exactly the 4 shape tests fail (isinstance(result, LLMResponse)), the error-behavior test rightly still passes.Verification
tests/unit: 904 passed / 4 skipped, plus one pre-existing failure (TestBERTScore::test_identical) identical on clean main. Ollama's file: 19 passed. ruff check + format clean on touched files (formatting normalized a few pre-existing long lines in the two files touched); mypy (3.11): same 164 pre-existing errors as main, zero in ollama.py, none added.tests/integrationcrashes identically on clean main in this environment (torch/numpy) and contains no ollama references.One thing this PR deliberately does NOT do
Ollama's methods keep their current names. Note that
pipeline.pyawaitsgenerate_with_usage— which isasyncon every other provider but sync on Ollama — so the pipeline + Ollama combination was and remains broken in a separate way (the awaited non-awaitable raisesTypeError, which the bareexcept Exceptionswallows into an empty answer). Renaming is an API decision, so I'm filing it as its own issue rather than smuggling it in here.Generated by Claude Fable 5 (brief, review), Claude Opus 4.8 (1M context) (implementation)