Conversation
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
e46497d to
460d8c2
Compare
|
Corroborating reproduction — this fires in profile pre-flight, not just during a run. Environment: fresh Provider: Ollama-hosted model via the OpenAI-compatible route — profile with Symptom: the profile Save fails with the AttributeError, even though the underlying model call succeeds. Log: Why this is a distinct case: the same profile reaches the model successfully — routing errors show up differently and are absent here (a wrong base URL produces Also confirmed against the installed SDK: Reporter context: this blocks saving any profile for a provider that omits cache fields, and pre-flight is the only validation path in the Agent Canvas settings UI — so there is no way to persist such a profile at all, independently of whether conversations would run. Happy to re-test against a patched build. |
|
I'm on it! atifnaqvi-jpg can track my progress at all-hands.dev |
|
Verified the fix and the reported pre-flight reproduction. Two real blockers remain on the current head ( 1.
|
|
I've completed the review. Here's my summary. What I foundThe PR's diagnosis is correct and the user-facing bug is real (the reporter's un-pinned
Verified with Blocker I could not clearThe PR head is I posted the full diagnosis plus a ready-to-apply patch to PR #5169. Next step needed from you: either grant this account write access to |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
The diagnosis is correct and the user-facing bug is real, but this head cannot pass the repository's own gates and the added regression test does not guard the bug in the pinned environment. Everything below was reproduced at 460d8c2 with the repo's own toolchain (uv sync --dev, litellm==1.93.0 from uv.lock).
1. Blocking — the new getattr calls violate the SDK's dynamic-attribute hook.
uv run python scripts/check_forbidden_dynamic_attributes.py exits 1 with three violations: telemetry.py:262 (x2) and telemetry.py:265. That hook is wired in .pre-commit-config.yaml as Forbid dynamic attribute access in SDK (always_run: true, pass_filenames: false), and the committed baseline may only shrink (--baseline-ref rejects additions), so this cannot be baselined away. It also contradicts the repository's stated preference for typed access over getattr/hasattr guards. Reading the details through details.model_dump() gives the same tolerance with no dynamic attribute access, because a deleted field is simply absent from the dump.
2. Blocking — ruff format fails on the same file.
uv run ruff format --check reports Would reformat, and the Ruff format pre-commit hook rewrites the file and fails. The offending part is the wrapping of the return statement on the new lines 265-267.
3. The new regression test does not guard the bug at the pinned LiteLLM.
I restored the pre-fix _cache_buckets in memory only (no tracked file was modified) and ran the new test: it passes. Under litellm==1.93.0, PromptTokensDetailsWrapper(cached_tokens=25) produces model_fields_set == {'cached_tokens'}, so the old guard is False and returns (25, 0) without raising. The same holds for PromptTokensDetailsWrapper(cached_tokens=0), the shape named in the PR's "How to Test" — so both the "fails on the old code" claim and that reproduction do not hold in the environment CI installs via uv sync --frozen.
Shapes that do raise on the old code even at 1.93.0, and would make the test a genuine guard: prompt_tokens_details={"cached_tokens": 25, "cache_creation_tokens": None}, or the equivalent raw ModelResponse usage dict with cache_creation_tokens: null (both keep the name in model_fields_set while the attribute is deleted).
4. The fix target has moved, and the branch no longer applies cleanly.
This PR is CONFLICTING with main (mergeable_state: dirty; git merge-tree reports a content conflict in telemetry.py). Main has since replaced the _cache_buckets guard with a typed normalize_usage() adapter (#5029) that reintroduces the same unsafe pattern: "cache_creation_tokens" in prompt_details.model_fields_set followed by a direct prompt_details.cache_creation_tokens read. I confirmed that this raises AttributeError on main for the cache_creation_tokens: null shape. The fix needs to be rebased and applied inside normalize_usage, not only _cache_buckets.
One scope note for the maintainer: this change also starts counting cache_write_tokens (the kimi-k2 naming) that the previous code ignored. That looks correct (litellm's own cost calculator reads both names) but it is a token-accounting change beyond the reported crash.
CI note: on this fork head the Run tests and Pre-commit checks runs are all action_required, so there is no live CI result to read; the hook failures above were reproduced locally with the pinned toolchain.
🔄 CHANGES REQUESTED
| # (e.g. providers without prompt caching like MiniMax), so direct | ||
| # attribute access raises AttributeError. getattr with a default | ||
| # is the only safe read here. | ||
| cache_write = getattr(details, "cache_creation_tokens", 0) or getattr( |
There was a problem hiding this comment.
Blocking: these getattr calls fail the repository's own Forbid dynamic attribute access in SDK pre-commit hook (scripts/check_forbidden_dynamic_attributes.py exits 1 with violations at 262 x2 and 265), and the committed baseline may only shrink, so this cannot be allowed through. uv run ruff format --check also reports this file needs reformatting.
model_dump() achieves the same tolerance without dynamic attribute access — deleted optional fields are simply absent from the dump:
dumped = details.model_dump()
cache_read = dumped.get("cached_tokens") or 0
cache_write = (
dumped.get("cache_creation_tokens")
or dumped.get("cache_write_tokens")
or 0
)
return int(cache_read), int(cache_write)Note this would also require replacing the MagicMock in test_record_usage_with_cache_read, since a mock's model_dump() returns another mock.
| token_usage = basic_telemetry.metrics.token_usages[0] | ||
| assert token_usage.cache_write_tokens == 30 | ||
|
|
||
| def test_record_usage_with_real_details_no_cache_fields(self, basic_telemetry): |
There was a problem hiding this comment.
This test does not fail on the pre-fix code in the environment CI installs. With the old _cache_buckets restored in memory, this test passes: at litellm==1.93.0 (pinned in uv.lock) PromptTokensDetailsWrapper(cached_tokens=25) yields model_fields_set == {'cached_tokens'}, so the old guard is False and returns (25, 0) without raising. PromptTokensDetailsWrapper(cached_tokens=0) behaves the same way, so it cannot stand in as the reproduction either.
A shape that does raise on the old code at 1.93.0 is the explicit null, which keeps the name in model_fields_set while the attribute is deleted:
usage = Usage(
prompt_tokens=100,
completion_tokens=50,
total_tokens=150,
prompt_tokens_details={"cached_tokens": 25, "cache_creation_tokens": None},
)Covering that (or the equivalent raw ModelResponse usage dict with cache_creation_tokens: null) makes this a real regression guard.
HUMAN:
I saved a MiniMax profile in Agent Canvas settings and chatted in a new conversation. Also chatted with ZAI glm-5.3-flash. All work with the fix.
AGENT:
Why
Any provider whose usage response includes
prompt_tokens_detailswithout cache fields crashes profile validation and token recording with'PromptTokensDetailsWrapper' object has no attribute 'cache_creation_tokens'. Reproduced with MiniMax in Agent Canvas settings; other reporters hit it with different providers (see #5099).Root cause:
Telemetry._cache_bucketsreaddetails.cache_creation_tokensdirectly, guarded only by"cache_creation_tokens" in details.model_fields_set. LiteLLM'sPromptTokensDetailsWrapper.__init__deletes unset optional fields, while its__setattr__mirroring keeps the name inmodel_fields_set. So the guard passes and the read raisesAttributeError. I confirmed this against the installed SDK:model_fields_setcontained the name even though the attribute was gone.Summary
cache_creation_tokens,cache_write_tokens) throughgetattrwith a 0 default in_cache_buckets, matching how the rest of the file already reads optional usage fields.test_record_usage_with_real_details_no_cache_fieldsusing a realPromptTokensDetailsWrapper(the existingMagicMock-based tests masked this bug).Issue Number
Fixes #5168.
How to Test
Reproduced and verified against the installed stack (openhands-sdk 1.49.1, Agent Canvas via
npm exec @openhands/agent-canvas):UsagewithPromptTokensDetailsWrapper(cached_tokens=0), which is what MiniMax returns, and calledTelemetry._cache_buckets. Result before the fix:AttributeError: 'PromptTokensDetailsWrapper' object has no attribute 'cache_creation_tokens'.(0, 0), usage with cache write returns(5, 7), usage without details returns(0, 0).tests/sdk/llm/test_llm_telemetry.pyfile: 43 passed. The new test fails on the old code and passes with the fix.minimax/MiniMax-M3,https://api.minimax.io/v1) in Settings → LLM: validation passed where it previously failed with the AttributeError, and a new conversation with MiniMax answers normally. Also verified with ZAIglm-5.3-flashin a new conversation.Video/Screenshots
No UI change, so no screenshots. Evidence is the reproduction output and test run described above.
Design Doc
Not needed, this is a small bug fix.
Type
Notes
The
model_fields_setguard looked safe but is not: LiteLLM's__setattr__mirroring adds the mirrored name tomodel_fields_setbefore__init__deletes both attributes. Later reads of optional wrapper fields should always usegetattrwith a default.Jev-Fast-Audit
⚡ Jev fast audit · estimates · 0.37s · commit 460d8c2
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage: complete supplied coverage; 2/2 hunks, 2/2 files.
All estimates and evidence