feat: preserve provider timing in normalized usage - #1261
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesGroq and Ollama converters now preserve provider timing fields in Provider timing conversion
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/any_llm/providers/groq/utils.py`:
- Around line 38-45: Replace dynamic getattr access in _groq_timing_details at
src/any_llm/providers/groq/utils.py:38-45 with direct typed access to the Groq
usage type’s fixed timing attributes, while retaining numeric filtering and
omission of absent fields. In src/any_llm/providers/ollama/utils.py:69-76, build
the timing mapping from direct OllamaChatResponse attributes rather than dynamic
access.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 89675344-4987-4d60-ae1a-228ea612d87a
📒 Files selected for processing (4)
src/any_llm/providers/groq/utils.pysrc/any_llm/providers/ollama/utils.pytests/unit/providers/test_groq_provider.pytests/unit/providers/test_ollama_provider.py
The timing fields are declared optionals on both provider SDK types (groq.types.completion_usage.CompletionUsage and ollama.ChatResponse), so dynamic getattr access is unnecessary and hides typos from mypy. AGENTS.md prefers direct attribute access when the field is typed. Two existing Ollama tests relied on the getattr fallback: pydantic v2 field names are absent from dir(ChatResponse), so Mock(spec=...) raises AttributeError for the duration fields. Declare them as None on those mocks, which also makes the no-timing assertion exercise the real filter path instead of the swallowed AttributeError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rename the timing helpers to the repo's verb-first shape (_extract_<provider>_<noun>, as in _extract_anthropic_thinking_signature). The provider-prefixed verb-less form was the only one of its kind in the providers tree. Add the missing Groq branch: a streaming chunk whose usage carries no timing fields leaves usage extras empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
njbrake
left a comment
njbrake
left a comment
There was a problem hiding this comment.
Note: this review was drafted by Claude Opus 5 via back-and-forth with @njbrake. The reasoning and decisions are his; the prose is Claude's.
Thanks for this. The Ollama half is correct and I verified it end to end. Blocking on the Groq streaming half, which cannot reach the new code.
Groq streaming timing never arrives. _create_openai_chunk_from_groq_chunk reads groq_chunk.usage, which is never populated on any-llm's Groq path:
- Groq SDK 1.6.0
Completions.create()has nostream_optionsparameter. - The SDK documents
ChatCompletionChunk.usageas "only be present when you setstream_options: {"include_usage": true}in your request." src/any_llm/providers/groq/groq.py:77stripsstream_options, with a comment that the Groq SDK rejects it.- Groq delivers streaming usage under
x_groq.usage; the SDK definesXGroq.usage. Nothing in this repo readsx_groq.
Feeding a Groq-shaped final chunk (usage under x_groq, no top-level usage) through the converter yields usage=None, so neither timing nor token counts survive. test_streaming_chunk_extracts_cached_tokens passes only because it hand-builds a top-level usage that Groq does not send. That test predates this PR, so you inherited the false green rather than creating it, but it now certifies an acceptance criterion the code does not meet.
Two ways to resolve, your pick:
- Fix it here: fall back to
x_groq.usage, and guardgroq/utils.py:127, wheregroq_chunk.choices[0]is unguarded and the SDK documentschoicesas possibly empty on the final usage chunk. This needs an integration test against real Groq streaming; I could not verify the live wire shape without a key. - Land the non-streaming half, drop the Groq streaming claim from the description, and open a follow-up. The Ollama half meets its criteria either way.
Three small commits pushed to your branch. Typed attribute access in the timing helpers, since both SDK types declare the four fields as optionals. Worth knowing why CodeRabbit's "quick win" was not one: pydantic v2 field names are absent from dir(ChatResponse), so Mock(spec=OllamaChatResponse).total_duration raises AttributeError and getattr(..., None) was swallowing it. Two existing Ollama tests broke until their mocks declared the fields, and your model_extra == {} assertion in the think-content test was passing via that swallowed error rather than the real filter path. I also renamed the helpers to the repo's verb-first shape (_extract_groq_timing_details, matching _extract_anthropic_thinking_signature) and added the missing Groq branch for streaming usage with no timing. Unit suite green at 2000 passed, ruff and mypy clean.
Two things that are fine as-is: your switch to model_validate departs from the issue text but is required, because pydantic's dataclass_transform makes mypy reject both **timing unpacking and literal extra kwargs under strict mode. And the or timing_details widening in the Ollama chunk converter is safe; on realistic /api/chat payloads, intermediate chunks still yield usage=None and only the final chunk gains timing.
Non-blocking: ollama.EmbedResponse carries the same four duration fields and the embedding converter drops them. Out of scope for #1258, but a reasonable follow-up.
…ntegration tests Both new Groq streaming unit tests used shapes Groq does not send on this path: top-level chunk.usage requires stream_options (absent from groq 1.6.0's create()), and an empty choices list only occurs with include_usage. The real final chunk carries a finish_reason choice alongside x_groq.usage, so a regression that keyed the fallback on empty choices would have stayed green. Add integration coverage for the four timing fields on both providers, streaming and non-streaming. The Groq streaming case is the only check that proves the live wire carries x_groq.usage; it needs real credentials, so it skips without them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
... and 22 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/test_provider_timing.py`:
- Around line 73-78: Update the exception handlers around both provider timing
calls in tests/integration/test_provider_timing.py (73-78 and 92-102) to stop
skipping on httpx.HTTPStatusError, while continuing to skip only confirmed
connection failures or timeouts. Include the caught connection exception details
in each pytest.skip message so every skip identifies its concrete root cause.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f5cc1f9-4ec7-4f6e-9649-608ba458e098
📒 Files selected for processing (2)
tests/integration/test_provider_timing.pytests/unit/providers/test_groq_provider.py
| try: | ||
| result = await llm.acompletion(model=provider_model_map[LLMProvider.OLLAMA], messages=_PROMPT) | ||
| # An unreachable host surfaces as a builtin ConnectionError from the ollama SDK on the | ||
| # non-streaming call and as a raw httpx.ConnectError on the streaming one. | ||
| except (ConnectionError, httpx.ConnectError, httpx.HTTPStatusError): | ||
| pytest.skip("Local Ollama host is not set up, skipping") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not skip all HTTP response failures.
httpx.HTTPStatusError includes invalid model names, authentication failures, and provider failures. These conditions must fail the test. Skip only confirmed unavailable-service conditions, such as connection errors or connection timeouts. Include the concrete connection failure in the skip message.
tests/integration/test_provider_timing.py#L73-L78: Removehttpx.HTTPStatusErrorfrom the skip handler.tests/integration/test_provider_timing.py#L92-L102: Removehttpx.HTTPStatusErrorfrom the skip handler.
As per coding guidelines, “Every test skip and fix must have a concrete root cause”.
📍 Affects 1 file
tests/integration/test_provider_timing.py#L73-L78(this comment)tests/integration/test_provider_timing.py#L92-L102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/test_provider_timing.py` around lines 73 - 78, Update the
exception handlers around both provider timing calls in
tests/integration/test_provider_timing.py (73-78 and 92-102) to stop skipping on
httpx.HTTPStatusError, while continuing to skip only confirmed connection
failures or timeouts. Include the caught connection exception details in each
pytest.skip message so every skip identifies its concrete root cause.
Source: Coding guidelines
njbrake
added
the
run-integration-tests
github-actions
Bot
removed
the
run-integration-tests
This branch was previously deployed
2 participants
Description
Preserve provider-reported server timing in normalized
CompletionUsageobjects.queue_time,prompt_time,completion_time,total_time) now survive for non-streaming and streaming conversions.total_duration,load_duration,prompt_eval_duration,eval_duration) now survive for non-streaming and streaming conversions.usage.model_extra, while reported zero values are preserved.PR Type
Relevant issues
Fixes #1258
Checklist
test_sync_messages_streaming_consumes_response_on_a_single_event_loop.AI Usage Information
AI Model used: GPT-5
AI Developer Tool used: Codex
Any other info you'd like to share: I reviewed the implementation and test results, and the change is limited to the issue scope.
I am an AI Agent filling out this form.
Summary by CodeRabbit