-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
fix(anthropic): pass logging_obj to client.post for litellm_overhead_time_ms #24071
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,42 @@ | ||
| from unittest.mock import MagicMock | ||
| from unittest.mock import AsyncMock, MagicMock | ||
|
|
||
| import pytest | ||
|
|
||
| from litellm.constants import RESPONSE_FORMAT_TOOL_NAME | ||
| from litellm.llms.anthropic.chat.handler import ModelResponseIterator | ||
| from litellm.llms.anthropic.chat.handler import ModelResponseIterator, make_call | ||
| from litellm.types.llms.openai import ( | ||
| ChatCompletionToolCallChunk, | ||
| ChatCompletionToolCallFunctionChunk, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_make_call_passes_logging_obj_to_client_post(): | ||
| """make_call must pass logging_obj to client.post so track_llm_api_timing can set llm_api_duration_ms for litellm_overhead_time_ms.""" | ||
| mock_client = AsyncMock() | ||
| mock_response = MagicMock() | ||
| mock_response.aiter_lines = MagicMock(return_value=iter([b'data: {"type":"message_start"}\n', b'data: {"type":"message_delta"}\n'])) | ||
| mock_client.post.return_value = mock_response | ||
|
|
||
| logging_obj = MagicMock() | ||
|
|
||
| await make_call( | ||
| client=mock_client, | ||
| api_base="https://api.anthropic.com/v1/messages", | ||
| headers={}, | ||
| data="{}", | ||
| model="claude-3-5-haiku", | ||
| messages=[{"role": "user", "content": "Hi"}], | ||
| logging_obj=logging_obj, | ||
| timeout=60.0, | ||
| json_mode=False, | ||
| ) | ||
|
|
||
| mock_client.post.assert_called_once() | ||
| call_kwargs = mock_client.post.call_args[1] | ||
| assert call_kwargs.get("logging_obj") is logging_obj | ||
|
Comment on lines
+13
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The PR fixes
Per the project's testing guidelines (at least 1 test is a hard requirement), this technically satisfies the bar, but adding tests for the sync path and non-streaming paths would prevent regressions where a future refactor accidentally drops Consider adding parallel tests for |
||
|
|
||
|
|
||
| def test_redacted_thinking_content_block_delta(): | ||
| chunk = { | ||
| "type": "content_block_start", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.kwargsinstead of index-basedcall_args[1]call_args[1]is the legacy tuple-indexing API for accessing keyword arguments on acallobject. The modern, more readable approach is.kwargs. Both work, but the newer form is clearer and less likely to break if positional args are added.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!