fix: token counting edits altering PR #3121, fixes issue #3026 #3194
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
camel/utils/token_counting.py
Outdated
| Union, | ||
| ) | ||
|
|
||
| from deprecation import deprecated # type: ignore[import-untyped] |
There was a problem hiding this comment.
it's not necessary to introduce a new dependency for deprecation warning
| 'prompt_tokens': getattr(usage, 'prompt_tokens', 0), | ||
| 'completion_tokens': getattr( | ||
| usage, 'completion_tokens', 0 | ||
| ), | ||
| 'total_tokens': getattr(usage, 'total_tokens', 0), |
There was a problem hiding this comment.
would be better also record cached token
| def extract_usage_from_response( | ||
| self, response: Any | ||
| ) -> Optional[Dict[str, int]]: | ||
| r"""Extract native usage data from Anthropic response. | ||
| Args: | ||
| response: Anthropic response object (Message or similar) | ||
| Returns: | ||
| Dict with keys: prompt_tokens, completion_tokens, total_tokens | ||
| None if usage data not available | ||
| """ | ||
| try: | ||
| if hasattr(response, 'usage') and response.usage is not None: | ||
| usage = response.usage | ||
| input_tokens = getattr(usage, 'input_tokens', 0) | ||
| output_tokens = getattr(usage, 'output_tokens', 0) | ||
| return { | ||
| 'prompt_tokens': input_tokens, | ||
| 'completion_tokens': output_tokens, | ||
| 'total_tokens': input_tokens + output_tokens, | ||
| } | ||
|
|
||
| except Exception as e: | ||
| logger.debug( | ||
| f"Failed to extract usage from Anthropic response: {e}" | ||
| ) | ||
|
|
||
| return None |
There was a problem hiding this comment.
seems duplicated, as it's already defined in base class
There was a problem hiding this comment.
This definition is required since this Anthropic specific method implements the abstract method defined earlier
| def extract_usage_from_response( | ||
| self, response: Any | ||
| ) -> Optional[Dict[str, int]]: |
There was a problem hiding this comment.
seems duplicated, as it's already defined in base class
There was a problem hiding this comment.
Same reasoning as above
examples/token_counter/__init__.py
Outdated
There was a problem hiding this comment.
this file is not needed in example code
examples/token_counter/__init__.py
Outdated
There was a problem hiding this comment.
seems this folder is AI-generated, some modules like enable_streaming_usage_for_openai is redundant, could we rewrite the example code to make it tidy?
test/utils/test_token_counting.py
Outdated
| ) | ||
|
|
||
|
|
||
| class TestOpenAITokenCounterExtractUsage: |
There was a problem hiding this comment.
this test code also could be polished to be more meaningful, this kind of mock and test could not do meaningful test
There was a problem hiding this comment.
I wrote a new example test script with this approach
`def test_extract_basic_usage(self):
"""Test extracting basic usage data from OpenAI response."""
if not OPENAI_AVAILABLE:
pytest.skip("OpenAI not available")
counter = OpenAITokenCounter(ModelType.GPT_4O_MINI)
# Create mock response with spec to prevent auto-creation of attributes
mock_response = Mock()
mock_response.usage = Mock(spec=['prompt_tokens', 'completion_tokens', 'total_tokens'])
mock_response.usage.prompt_tokens = 50
mock_response.usage.completion_tokens = 25
mock_response.usage.total_tokens = 75
# Extract usage
usage = counter.extract_usage_from_response(mock_response)
# Validate
assert usage is not None
assert usage["prompt_tokens"] == 50
assert usage["completion_tokens"] == 25
assert usage["total_tokens"] == 75
assert len(usage) == 3 # Only basic fields`
The only way to make the test more realistic would be to make actual API calls for real extraction but this resembles more of an integration test, let me know if you would prefer this!
Wendong-Fan
added
the
Waiting for Update
|
Let's put this PR on hold for the time being. We've decided to move forward with a more agentic approach to memory management, and the new implementation will likely supersede the work done here @waleedalzarooni |
Wendong-Fan
added
Pending
and removed
Waiting for Update
Description
Implementing changes from code review of @MaxonZhao
Checklist
Go over all the following points, and put an
xin all the boxes that apply.Fixes #issue-numberin the PR description (required)pyproject.tomlanduv lockIf you are unsure about any of these, don't hesitate to ask. We are here to help!