Root cause fix - migrate all logging update to use 1 function - for centralized kwarg updates - #23659
Root cause fix - migrate all logging update to use 1 function - for centralized kwarg updates#236594 commits merged into
Conversation
… 1 place to update logging kwarg updates
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bac05524a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| litellm_logging_obj.update_from_kwargs( | ||
| kwargs=kwargs, |
There was a problem hiding this comment.
Include explicit metadata when logging responses calls
responses() takes metadata as an explicit parameter, but this pre-call logging now passes only kwargs into update_from_kwargs; since explicit params are not inside kwargs, request metadata is dropped from litellm_params for this path. Any callback/cost logic that depends on model_call_details["litellm_params"]["metadata"] (the behavior preserved before this change) will silently lose metadata when users call litellm.responses(..., metadata=...).
Useful? React with 👍 / 👎.
Greptile SummaryThis PR introduces a centralized Key observations:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/litellm_logging.py | Adds new update_from_kwargs convenience method that automatically extracts metadata/litellm_metadata from the raw kwargs dict, centralising the metadata forwarding logic that was previously scattered across all callers. |
| tests/test_litellm/llms/custom_httpx/test_llm_http_handler.py | The regression test for issue #23185 was updated to mock update_from_kwargs instead of update_environment_variables, but the new assertion only checks that litellm_metadata is in the kwargs dict — not that it is correctly extracted into litellm_params, weakening the original regression guard. |
| tests/test_litellm/litellm_core_utils/test_litellm_logging.py | Adds comprehensive TestUpdateFromKwargs test class covering metadata extraction, backfill logic, priority order, custom-pricing detection, and empty-kwargs edge cases — good unit coverage of the new method. |
| litellm/llms/custom_httpx/llm_http_handler.py | Migrates the Anthropic messages handler to update_from_kwargs, removing the manual metadata/litellm_metadata extraction from litellm_params — the direct call site for issue #23185. |
| litellm/responses/main.py | Migrates all six response API entry points (responses, delete_responses, get_responses, list_input_items, cancel_responses, compact_responses) to update_from_kwargs, correctly using local_vars (which already includes kwargs) as the source dictionary. |
| litellm/videos/main.py | Migrates video API logging to update_from_kwargs; also adds seconds, extra_headers, extra_query, extra_body to overloads and removes api_key/api_base/api_version from typed overloads (these remain accessible via **kwargs). |
| litellm/batches/main.py | Migrates create_batch and retrieve_batch to update_from_kwargs; removes the explicit "metadata": metadata key from the litellm_params dict, delegating extraction to the new helper. |
| litellm/vector_stores/main.py | Migrates all vector store operations; also adds import builtins to safely use builtins.list in the mock-response isinstance check where the local list function shadowed the builtin. |
| tests/test_litellm/images/test_image_edit_utils.py | Updates capturing mock to intercept update_from_kwargs; manually mirrors the metadata-extraction logic inside the capturing stub so the downstream assertion on captured_litellm_params remains valid. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Call site\n(batches, responses, images, videos, etc.)"] -->|"update_from_kwargs(kwargs=kwargs, litellm_params=..., ...)"| B["update_from_kwargs"]
B --> C{"'metadata'\nin kwargs?"}
C -->|Yes| D["base['metadata'] = kwargs['metadata']"]
C -->|No| E{"'litellm_metadata'\nin kwargs?"}
D --> E
E -->|"Yes (and is dict)"| F["base['litellm_metadata'] = kwargs['litellm_metadata']"]
F --> G{"'metadata' already\nset in base?"}
G -->|No| H["base['metadata'] = litellm_metadata.copy()"]
G -->|Yes| I["skip backfill"]
E -->|No| J["no-op"]
H --> K["Merge caller litellm_params\nover base (caller wins)"]
I --> K
J --> K
K --> L["update_environment_variables(\n litellm_params=merged,\n optional_params=...,\n model=..., user=...\n)"]
L --> M["self.litellm_params updated\n(metadata + litellm_metadata\nnow present)"]
M --> N["use_custom_pricing_for_model\nchecks litellm_params\n→ detects custom pricing ✓"]
Comments Outside Diff (1)
-
tests/test_litellm/llms/custom_httpx/test_llm_http_handler.py, line 186-233 (link)Regression test weakened for issue [Bug]: Custom Pricing is broken for /messages and /responses endpoints (streaming) #23185
The original regression test for [Bug]: Custom Pricing is broken for /messages and /responses endpoints (streaming) #23185 verified the complete end-to-end data flow: that
litellm_metadataended up insidelitellm_paramsas seen byuse_custom_pricing_for_model. The new test only verifies the first hop — thatlitellm_metadatais present in thekwargsdict forwarded toupdate_from_kwargs. Sinceupdate_from_kwargsis fully mocked (Mock()), the test no longer validates that the metadata is correctly extracted and placed intolitellm_paramsat all.If a future regression broke the extraction logic inside
update_from_kwargs, this test would still pass. The unit tests inTestUpdateFromKwargspartially compensate, but the integration regression from [Bug]: Custom Pricing is broken for /messages and /responses endpoints (streaming) #23185 is no longer guarded here.Consider removing the mock for
update_from_kwargsand instead asserting onlogging_obj.litellm_paramsafter the call, similar to howTestUpdateFromKwargs.test_custom_pricing_detected_via_litellm_metadataworks:# Instead of: mock_logging_obj.update_from_kwargs = Mock() # ... mock_logging_obj.update_from_kwargs.assert_called_once() kwargs_arg = ... assert "litellm_metadata" in kwargs_arg # Prefer a real logging obj + assert the downstream effect: assert "litellm_metadata" in mock_logging_obj.litellm_params assert mock_logging_obj.litellm_params["litellm_metadata"]["model_info"] == custom_model_info
Rule Used: # Code Review Rule: Mock Test Integrity
What:... (source)
Last reviewed commit: 90252ee
…entralized kwarg updates (BerriAI#23659) * fix: Fixes BerriAI#23185 * fix(responses/main.py): ensure litellm metadata custom cost works * refactor: move all logging updates to a common function, to have just 1 place to update logging kwarg updates
Relevant issues
Root cause fix for #23185
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewCI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes