fix(anthropic): include cache tokens in total_tokens - #6857
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Anthropic tests now cover omitted and explicit zero cache fields. They also verify cache-read and cache-creation tokens in synchronous and asynchronous streaming usage totals. ChangesAnthropic token usage tests
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
lib/crewai/tests/llms/anthropic/test_anthropic.py (1)
1640-1660: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for missing cache fields.
This test covers only responses that include both cache fields. Add a behavior test with only
input_tokensandoutput_tokens. Assert thattotal_tokensremains their sum and both cache-specific outputs are zero.As per coding guidelines,
**/tests/**/*.py: Write unit tests for new functionality that focus on behavior rather than implementation details.🤖 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 `@lib/crewai/tests/llms/anthropic/test_anthropic.py` around lines 1640 - 1660, Extend test_anthropic_cache_creation_tokens_extraction coverage with a response usage object containing only input_tokens and output_tokens. Assert total_tokens equals their sum and both cached_prompt_tokens and cache_creation_tokens default to zero, focusing on observable extraction behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@lib/crewai/tests/llms/anthropic/test_anthropic.py`:
- Around line 1640-1660: Extend test_anthropic_cache_creation_tokens_extraction
coverage with a response usage object containing only input_tokens and
output_tokens. Assert total_tokens equals their sum and both
cached_prompt_tokens and cache_creation_tokens default to zero, focusing on
observable extraction behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0f5c07e-26c8-4a39-be55-def8ab0f31a3
📒 Files selected for processing (2)
lib/crewai/src/crewai/llms/providers/anthropic/completion.pylib/crewai/tests/llms/anthropic/test_anthropic.py
|
Addressed: the missing-cache-fields test now also asserts total_tokens stays input+output when cache counters are absent/zero. |
anujbolewar
left a comment
anujbolewar
left a comment
There was a problem hiding this comment.
Correct — total_tokens now includes cache read and cache creation tokens, which matches how Anthropic bills them, and the updated test pins it. Worth confirming the streaming path applies the identical accounting, since there is often a separate usage extractor there — if it still sums input plus output only, totals would diverge between streaming and non-streaming calls.
Confirm sync/async streaming use the shared extractor so cache read/creation stay in billed totals, and strengthen the missing-cache-fields assertions.
7fe4a50 to
d389c13
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@anujbolewar Confirmed — streaming does not use a separate usage extractor. Both sync and async streaming paths call the same To pin that, I rebased onto latest main and added unit tests that drive sync/async |
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 `@lib/crewai/tests/llms/anthropic/test_anthropic.py`:
- Around line 1683-1687: Add separate test cases for the usage-token handling
around the existing Anthropic test: one fixture must omit both cache attributes
entirely, and another must define both attributes as numeric 0 values. Assert
the resulting usage fields for each case, preserving the existing expectations
for input, output, total, and cache token values.
🪄 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: CHILL
Plan: Pro Plus
Run ID: b295f38a-2aad-42e5-93ed-e68db31fa45f
📒 Files selected for processing (1)
lib/crewai/tests/llms/anthropic/test_anthropic.py
Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Summary
Anthropic usage extraction only summed
input_tokensandoutput_tokensfortotal_tokens, so prompt-cache reads and cache writes were left out of the total.This change includes
cache_read_input_tokensandcache_creation_input_tokensintotal_tokens, and updates the existing unit test accordingly.Fixes #6768
Test plan
test_anthropic_cache_creation_tokens_extractionandtest_anthropic_token_usage_trackingtotal_tokensequals input + output + cache_read + cache_creation when cache fields are present