refactor(ptu): give the rollup a source-agnostic deployment record - #37501
Conversation
The flat-cost rollup reads deployments only from LiteLLM_ProxyModelTable, so a PTU deployment declared in config.yaml never accrues flat cost. Those deployments live in llm_router.model_list as plain dicts whose id sits in model_info rather than on the entry, so they do not satisfy the shape _parse_ptu_model reads. Adds a frozen record in that shape and a factory that maps a router entry onto it, leaving _parse_ptu_model byte-identical so the existing cases stand as evidence of no behaviour change. Nothing calls the factory yet; the caller lands with the loader union. _decode_model_info also stops handing back valid JSON that is not an object. It decoded a list or a scalar and returned it as a mapping, so the caller read fields off it and raised, losing the whole run rather than the one bad deployment.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @dataclass(frozen=True, slots=True) | ||
| class _PTUDeployment: | ||
| """A deployment in the shape ``_parse_ptu_model`` reads, whatever declared it. | ||
|
|
||
| A ``LiteLLM_ProxyModelTable`` row already has it. A router entry does not: its id | ||
| lives in ``model_info.id`` rather than on the entry itself. | ||
| """ | ||
|
|
||
| model_id: str | ||
| model_name: str | ||
| model_info: Mapping[str, object] | ||
|
|
||
|
|
||
| def _router_deployment(deployment: Mapping[str, object]) -> _PTUDeployment | None: | ||
| """A router ``model_list`` entry in the shape the parser reads, else None. | ||
|
|
||
| An id is required rather than defaulted because it keys the sentinel row: every | ||
| deployment without one would collapse onto a single row per team and only the last | ||
| would be billed. The mapping is copied because the router rewrites entries in place | ||
| while the rollup runs. | ||
| """ | ||
| model_info: Final = _decode_model_info(deployment.get("model_info")) | ||
| if model_info is None: | ||
| return None | ||
| model_id: Final = model_info.get("id") | ||
| if not isinstance(model_id, str) or not model_id: | ||
| return None | ||
| return _PTUDeployment( | ||
| model_id=model_id, | ||
| model_name=str(deployment.get("model_name") or ""), | ||
| model_info=MappingProxyType(dict(model_info)), | ||
| ) |
There was a problem hiding this comment.
🟡 New deployment record helper is never used anywhere in the product
A new deployment record type and its builder are added (_router_deployment at litellm/proxy/spend_tracking/ptu_flat_cost_rollup.py:143) with no code anywhere calling them, so the product gains code that never runs while the repository's guidelines ask for the minimum, non-speculative code that solves the problem.
Impact: No user-visible behavior change, but unused machinery ships and only tests exercise it, which conflicts with the repo's simplicity rule.
Dead code confirmed by a repo-wide search for callers
_PTUDeployment and _router_deployment are referenced only by their own definition and by tests/test_litellm/proxy/spend_tracking/test_ptu_flat_cost_rollup.py. The rollup itself still loads deployments only from LiteLLM_ProxyModelTable, so the router/config.yaml path this shape exists for is unreachable (the author notes the loader union follows in a later change). CLAUDE.md's "Simplicity First" section states "Nothing speculative", "No features beyond what was asked" and "No abstractions for single-use code".
Prompt for agents
The new _PTUDeployment dataclass and _router_deployment factory in litellm/proxy/spend_tracking/ptu_flat_cost_rollup.py have no production caller; only tests reference them. CLAUDE.md asks for non-speculative, minimum code. Consider landing this shape together with the loader change that actually feeds router/config.yaml deployments into the rollup, so the abstraction arrives with its consumer, or narrow the PR to only the _decode_model_info hardening that is reachable today.
Was this helpful? React with 👍 or 👎 to provide feedback.
Greptile SummaryThis PR makes PTU deployment metadata source-agnostic while hardening the existing database rollup against JSON values that are not objects.
Confidence Score: 5/5The PR appears safe to merge, with no actionable failures found in the currently reachable rollup path. The changed decoder now isolates malformed non-object metadata instead of allowing it to abort the nightly job, while the source-agnostic factory is not yet production-reachable and its expected behavior is covered by focused tests.
|
| Filename | Overview |
|---|---|
| litellm/proxy/spend_tracking/ptu_flat_cost_rollup.py | Adds the source-agnostic deployment record and safely rejects model metadata that cannot support mapping-based parsing; no actionable regression was found on the currently reachable database path. |
| tests/test_litellm/proxy/spend_tracking/test_ptu_flat_cost_rollup.py | Adds focused coverage for database/router parsing parity, malformed metadata, deployment identity, naming, and copied immutable state without weakening existing assertions. |
Reviews (1): Last reviewed commit: "refactor(ptu): give the rollup a source-..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7097d55. Configure here.
tin-berri
left a comment
tin-berri
left a comment
There was a problem hiding this comment.
LGTM — small diff but billing-adjacent (PTU flat-cost rollup), so reviewed carefully.
- The real fix is in
_decode_model_info: previously a JSON string decoding to a non-dict (e.g."[1, 2, 3]") was handed back as-is and treated like a mapping by callers, causing anAttributeErrorthat crashed the entire nightly rollup on one malformed deployment — losing every team's PTU cost for the day, not just the bad row's. Now it correctly returnsNonefor non-object JSON, so only the malformed deployment is skipped. Demonstrated with a real Postgres repro (not mocked) showing the before/after: crash-with-zero-rows vs. skip-and-bill-correctly. _parse_ptu_modelitself is untouched in this diff (doesn't appear at all), consistent with the "byte-identical, 102 existing cases prove no behavior change" claim — verified by its absence from the diff rather than just taking the claim at face value.- The new
_router_deploymentfactory (for a future config.yaml PTU path) is well-tested — including a defensive copy into an immutableMappingProxyTypeverified to not alias the router's live, in-place-mutatedmodel_infodict, and parity tests proving a DB-sourced and router-sourced deployment parse to an identicalPTUModel. It's honestly caveated as having no caller yet, so this part is inert dead code until the follow-up PR wires it in — not yet a production behavior change. - Scope confined to the rollup module and its test file. CI green.
a1afc2f
into
litellm_internal_staging
2 participants
TLDR
Problem this solves:
How it solves it:
User Flow
This is the first of several changes behind config.yaml PTU support, so the config.yaml half is not reachable yet. What is reachable today is the second problem above
Before: an admin whose proxy holds one malformed deployment loses every team's PTU cost for the day
LITELLM_ENABLE_PTU_COST_ATTRIBUTION=Trueand a team on provisioned throughput, alongside an unrelated deployment whose stored settings are malformedAfter: the same proxy skips only the malformed deployment and bills everyone else
Relevant issues
Linear ticket
Refs LIT-5809
The ticket is only closed once a config.yaml PTU deployment actually accrues flat cost, which needs the loader union and the pricing rules that follow this
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Real Postgres, the real scheduled entry point, no mocks. Two deployments: one on provisioned throughput at 100 units and $0.02 per unit per hour, and one whose stored settings decode to a list rather than an object. The only thing not exercised is the cron trigger itself, since the job has no on-demand entry point
Setup, once:
Each side runs the same three lines, from its own worktree so the loaded tree is unambiguous:
Before (4bb3152)
dep-goodis a perfectly valid reservation and it was not billed either, because the run never got that farAfter (7097d55)
Two notes on what is and is not claimed here.
_parse_ptu_modelis byte-identical to the merge base, verified by diffing the function body, so the 102 pre-existing cases in the mapped test file run unmodified and are the evidence that the parse path is unchanged; the test file diff is 177 insertions and 0 deletions. The behaviour that does change on the stored-deployment path is_decode_model_inforeturning None for valid JSON that is not an object, where it previously handed the decoded list or scalar back as a mappingA stored deployment whose settings hold a JSON array directly, rather than a JSON string containing one, already skipped cleanly before this change, since the database driver hands back a native list that never reaches the string branch. The reachable failure is the string form shown above
Type
🧹 Refactoring
🐛 Bug Fix
Caveats (if any)
Final Attestation
Note
Cursor Bugbot is generating a summary for commit 7097d55. Configure here.