fix(logging): treat an empty prompt_id as no prompt in manager dispatch - #37477
bharadwaj-pendyala wants to merge 1 commit into
Conversation
Greptile SummaryThe PR aligns empty-string
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported Arize prompt-less dispatch failure is prevented because the new falsy guard consults the manager’s no-prompt predicate and skips Arize before compilation.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/litellm_logging.py | Updates prompt-manager fallback selection so empty prompt IDs follow the same path as absent IDs. |
| tests/test_litellm/litellm_core_utils/test_litellm_logging.py | Adds focused coverage confirming cache-control metadata is injected when prompt_id is empty. |
Reviews (2): Last reviewed commit: "fix(logging): treat an empty prompt_id a..." | Re-trigger Greptile
|
|
||
| # If prompt_id is provided, try to auto-detect which system has this prompt | ||
| if prompt_id and dynamic_callback_params is not None: | ||
| # Ask the registered prompt managers which one owns this call. Dynamic params | ||
| # such as cache_control_injection_points get here with no prompt_id, and a | ||
| # manager that declines must not block the hook the param belongs to |
There was a problem hiding this comment.
Comments duplicate dispatch logic
These comments and related test docstrings restate behavior already expressed by the code, adding maintenance overhead and avoidable documentation drift
Context Used: CLAUDE.md (source)
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!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Greptile's blocking point is real, but it predates this PR and the dispatcher is not where it can be fixed.
# litellm/integrations/arize/arize_phoenix_prompt_manager.py:353-365
def should_run_prompt_management(self, prompt_id, prompt_spec, dynamic_callback_params) -> bool:
"""
Determine if prompt management should run based on the prompt_id.
For Arize Phoenix, we always return True and handle the prompt loading
in the _compile_prompt_helper method.
"""
return TrueSo the probe this PR adds asks Phoenix whether it owns a prompt-less cache-control call, and Phoenix volunteers. # litellm/integrations/prompt_management_base.py:167-169
if prompt_id is None:
raise ValueError("prompt_id is required for Prompt Management Base class")
if not self.should_run_prompt_management(I checked whether this PR put that path there. Register the manager, send On On Identical. Before this change the unconditional The base-commit run really did use base code, not the editable install: Phoenix is the only prompt manager in the tree that answers this way. Every other one already declines a prompt-less call, which is why the probe fixes them:
Those five stop being selected, so The Phoenix fix is one line in its predicate, but it reverses a documented decision inside an integration this PR does not otherwise touch, and I would rather not bury that in a dispatcher change. Happy to send it as a follow-up, or to add it here if you would rather review both at once. Tell me which you prefer and I will do that. |
b21cd45 to
1514baa
Compare
|
Rescoped: 4829bb3 landed the prompt-less fix, so this now covers only the empty-string |
1514baa to
e8c0869
Compare
|
Rebased onto The conflict was in the test file only, and it was additive on both sides: staging appended three Re-proved on the new base rather than assuming the rebase was safe. Reverting that single line turns the new test red, restoring it turns it green, and the whole file passes: One thing I want to put on the record rather than leave for a reviewer to find, because the diff is small but it moves more than the empty string.
That skip is the point, not a side effect. Arize's own docstring says it "needs a prompt_id to compile, so it declines requests without one", and today it gets handed a call naming no prompt and goes looking for a prompt called The line above already agrees with that reading. Auto-detection at Same reasoning covers the non- Happy to narrow it to |
|
The failing step is The branch changes two files, Checked out Same three lines. Leaving it alone, since bumping |
e8c0869 to
be0b570
Compare
|
The One check is still red, and it is not from this PR: The same assertion, same numbers, fails on three unrelated branches from tonight:
This PR touches Everything else is green on |
66f9304 to
24da583
Compare
1ae10cd to
7c82ad2
Compare
should_run_prompt_management_hooks gates on `if prompt_id`, so an empty string reaches dispatch as a call that names no prompt. Dispatch gated on `prompt_id is None`, so the empty string skipped the guard, a registered prompt manager was selected, and the cache_control hook behind it in the chain never ran. 4829bb3 fixed the prompt-less case. This narrows the remaining gap so both sites read a blank prompt_id the same way.
7c82ad2 to
bcc5c90
Compare
TLDR
Problem this solves:
prompt_idsilently dropscache_control_injection_points""names a promptHow it solves it:
prompt_idthe way the entry gate doesUser Flow
Before: a developer whose client sends an empty prompt id gets no prompt caching, and nothing tells them why
callbacks: ["dotprompt"]and givesclaude-opus-4-5acache_control_injection_points: [{role: system, location: message}]"prompt_id": ""prompt_idfield and resends. That request does get the cache marker, so the empty string is the only differenceAfter: an empty prompt id behaves exactly like an absent one, and the prefix caches
"prompt_id": """cache_control": {"type": "ephemeral"}, so http://localhost:4000/ui/?page=logs bills the next identical call as a cache readRelevant issues
Follow-up to #37469
That report is the prompt-less case, and 4829bb3 already fixed it on
litellm_internal_staging. I re-verified: on35416c702da call with noprompt_idgets the cache marker. What survives is the empty-string case, which is what this PR narrows to, so it does not close the issue on its ownLinear ticket
Pre-Submission checklist
Screenshots / Proof of Fix
should_run_prompt_management_hooksdecides whether to run the prompt hooks at all, and it gates onif prompt_id, so""reaches dispatch as a call that names no prompt.get_custom_logger_for_prompt_managementthen gated onprompt_id is None, so""skipped the guard, dotprompt was selected, and it returned the messages untouched. The cache-control hook sits after that in the same chain and never ranThis is local preprocessing that happens before any provider request is built, so the run below drives real HTTP through the full SDK against an upstream that records what it receives. I have no provider credentials on this machine, so the upstream is a stub on 127.0.0.1 rather than Anthropic. What it proves is the system block on the wire, which is exactly the bytes the provider would have been sent
Shared setup, both cases:
Before (35416c7)
no prompt_id, the case #37469 reported
python /tmp/call.pyempty prompt_id
python /tmp/call.py ""cache_control:After (1514baa)
no prompt_id, the case #37469 reported
python /tmp/call.pyempty prompt_id
python /tmp/call.py ""Type
🐛 Bug Fix
Caveats (if any)
GenericPromptManageraccepts""; it stops being selected for itprompt_idis unchangedOn the first one, worth a maintainer's call before merge.
GenericPromptManager.should_run_prompt_managementreturns True for""because it testsprompt_id is not None, so today it is selected and then compiles against an empty id. After this change it is skipped, same as every other in-tree manager. I read that as the predicate being wrong rather than behaviour to keep, but if you want""to stay a valid id for that manager, the fix belongs in the entry gate atshould_run_prompt_management_hooksinstead and I will move itFinal Attestation