Skip to content

fix: accumulate nested provider usage fields - #2500

Closed
mikemikimike wants to merge 4 commits into
567-labs:mainfrom
mikemikimike:fix/issue-2493-usage-accumulation
Closed

fix: accumulate nested provider usage fields#2500
mikemikimike wants to merge 4 commits into
567-labs:mainfrom
mikemikimike:fix/issue-2493-usage-accumulation

Conversation

@mikemikimike

Copy link
Copy Markdown
Contributor

Describe your changes

Fix retry usage accumulation so provider usage models are merged recursively instead of relying on hand-maintained field lists.

  • Accumulates newly added numeric fields inside nested Anthropic usage objects such as cache_creation and server_tool_use.
  • Accumulates all current and future numeric fields in OpenAI token detail models.
  • Preserves the existing behavior of carrying totals into a response when a retry omits an optional field.
  • Adds API-key-free regression coverage for nested Anthropic usage fields across multiple retries.

Issue ticket number and link

Closes #2493

Validation

  • PYTHONUTF8=1 uv run --extra dev --extra anthropic ruff check ... — passed
  • PYTHONUTF8=1 uv run --extra dev --extra anthropic ty check instructor/v2/core/usage.py instructor/v2/providers/anthropic/usage.py — passed
  • PYTHONUTF8=1 uv run --extra dev --extra anthropic pytest tests/coverage/test_core_patch_retry_coverage.py tests/test_utils.py -q — 43 passed
  • Anthropic usage-focused tests — 2 passed
  • Full tests/coverage/test_anthropic_support_coverage.py — 8 passed; 1 pre-existing Windows path assertion failed in test_anthropic_multimodal_encodes_remote_image_and_local_pdf (\\tmp\\example.pdf vs /tmp/example.pdf), unrelated to changed files.

Checklist before requesting a review

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests.
  • If it is a core feature, I have added documentation.

@ErenAta16

Copy link
Copy Markdown

Thanks for picking this up so quickly — the recursive walk is the right shape, and using type(value) is int rather than isinstance is a nice touch, since it keeps bool fields from being summed as 0/1.

One gap though, and it's the half of the original report that motivated the "newer" in the title.

Both _zero_numeric_fields and _accumulate_models iterate type(model).model_fields. Fields the API returns that the installed SDK version doesn't declare don't live there — both SDKs set extra="allow" on their base model, so unknown fields land in __pydantic_extra__:

anthropics/anthropic-sdk-python  src/anthropic/_models.py  ConfigDict(extra="allow", ...)
openai/openai-python             src/openai/_models.py     ConfigDict(extra="allow", ...)

That's deliberate on their side — it's how a new billable token field reaches you before you upgrade the SDK. But it means this accumulator walks straight past it.

Reproduced with a stand-in model shaped like the real ones:

class Usage(BaseModel):
    model_config = ConfigDict(extra="allow")
    input_tokens: int = 0
    output_tokens: int = 0

total = Usage(input_tokens=0, output_tokens=0)
for _ in range(2):
    r = Usage.model_validate({
        "input_tokens": 10, "output_tokens": 5,
        "cache_creation_5m_input_tokens": 100,   # newer than this SDK
    })
    _accumulate_models(r, total)
total.model_dump() -> {'input_tokens': 20, 'output_tokens': 10}
cache_creation_5m_input_tokens -> absent

200 billable tokens accounted for, silently. The declared fields accumulate correctly, so this passes any test written against the fields the SDK already knows about — which is why I'd want a case using model_validate with an undeclared key rather than a constructor call.

The loop probably wants to run over the union rather than just declared fields, something like:

field_names = list(type(total).model_fields) + list(total.__pydantic_extra__ or {})

with the same treatment on the response side so a field present only in response.__pydantic_extra__ gets seeded into total. getattr/setattr already work on extras under extra="allow", so the body shouldn't need to change much.

Separately, worth deciding explicitly what should happen to float fields: they currently fall through to the elif response_value is not None branch and get overwritten rather than summed. Fine if no usage field is ever a float, but it's the kind of thing that quietly becomes wrong when a cost field shows up.

@mikemikimike

Copy link
Copy Markdown
Contributor Author

Addressed the SDK compatibility issue from the review.

Changes:

  • Accumulate both declared Pydantic fields and model_extra fields, so newer billable usage fields are not dropped when the installed SDK model is older.
  • Accumulate numeric extra fields including floats (excluding booleans), while preserving nested model behavior.
  • Added a regression test using OpenAI CompletionUsage with undeclared integer and float fields.

Validation:

  • ruff format passed
  • ruff check passed
  • ty check passed
  • pytest tests/coverage/test_core_patch_retry_coverage.py -q: 19 passed

The broader Anthropic coverage run still has the pre-existing Windows-only /tmp/example.pdf path assertion failure; the usage tests themselves pass.

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran the new version through the same harness I used to demonstrate the gap, plus three cases around it. All four behave correctly now.

scenario result
field the installed SDK doesn't declare, two retries of 100 200 accumulated (was silently dropped)
float extra field, three retries of 0.25 0.75
bool extra field, three retries of True stays True, not summed to 3
field present on retry 1, absent on retry 2 total holds at 7

_field_names unioning model_fields with model_extra, and _all_field_names taking the union across both sides, is what closes it — a field that exists only on the response now gets seeded into the total rather than skipped because the total's class never declared it.

Switching from type(value) is int to isinstance(value, Real) and not isinstance(value, bool) also picks up the float case I raised as the secondary note, and keeps bool out, which was the thing the original type() is check got right and an isinstance(value, int) would have broken. Good trade.

One small thing, non-blocking: _zero_numeric_fields assigns integer 0 regardless of the field's declared type, so a nested float field is zeroed to an int. Harmless in practice — subsequent addition promotes it back to float, and the SDK models don't run validate_assignment — but if that ever changes it would start raising, and a type(value)(0) or value * 0 would be immune.

Thanks for turning this around so quickly, and for taking the extra-fields point seriously rather than treating it as an edge case — it's the half of the report that's hardest to notice in production, since the tokens just quietly don't appear.

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulled 5c0af36 and exercised it against models configured the way the two SDK base classes are (ConfigDict(extra="allow")), rather than reading the diff. The gap is closed.

Undeclared fields now accumulate alongside the declared ones, both integer and float:

call 1   prompt=10  completion=5   extra={'cache_creation_input_tokens': 7,  'cost_usd': 0.25}
call 2   prompt=20  completion=10  extra={'cache_creation_input_tokens': 14, 'cost_usd': 0.5}

That is exactly the case that motivated the report. An installed SDK older than the API no longer silently drops the billable field.

The three things I would have gone looking for as side effects all behave:

bool extra (cache_hit=True)         stays True, not summed to 2
nested model's own extra field      reasoning_tokens 3->6, audio_tokens 4->8
string extra (service_tier)         carried through, last value wins

Good call moving the bool guard into _is_numeric rather than leaving it implicit in type(value) is int. Widening to Real while keeping not isinstance(value, bool) is the correct order, since bool is a subclass of int and would otherwise have started summing the moment floats were allowed in.

One note, not a request. numbers.Real excludes Decimal:

int       Real=True    numeric=True
float     Real=True    numeric=True
Fraction  Real=True    numeric=True
bool      Real=True    numeric=False   (correctly excluded)
Decimal   Real=False   numeric=False
str       Real=False   numeric=False

Decimal is registered against numbers.Number but deliberately not against Real, so a cost field that arrives as a Decimal would fall through to the carry-last branch and be overwritten each call instead of summed. Neither SDK produces Decimal today, they both hand back plain floats for money-shaped fields, so this is not something this PR needs to handle. Worth knowing if anyone later adds a custom parser, since it would fail quietly rather than loudly.

_zero_numeric_fields leaving an int 0 in a float slot is fine in practice for the same reason, 0 + 0.25 gives back a float and the running total stays correct. I checked rather than assumed.

Looks good to me.

@mikemikimike
mikemikimike deleted the fix/issue-2493-usage-accumulation branch August 2, 2026 02:16
@mikemikimike
mikemikimike restored the fix/issue-2493-usage-accumulation branch August 2, 2026 11:20
@mikemikimike mikemikimike reopened this Aug 2, 2026

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed #2493, so not a neutral reviewer. I ran the merge in this PR against the real SDKs at the versions this repo installs, and it holds up on every case I could construct. One asymmetry worth a line, and one reason the shape-driven approach is not just tidier but necessary.

RED/GREEN on the OpenAI half, openai==2.52.0, _initialize_usage verbatim from core/retry.py, three attempts each reporting 7 in every prompt-detail field:

                      main    this PR   expected
prompt_tokens          300      300       300
  audio_tokens          21       21        21
  cached_tokens         21       21        21
  cache_write_tokens  None       21        21

_initialize_usage builds PromptTokensDetails(audio_tokens=0, cached_tokens=0), so cache_write_tokens stays None on the accumulator, and the unconditional model_copy write-back at the end of update_total_usage puts that None over the value the provider actually sent. Same shape for accepted_prediction_tokens and rejected_prediction_tokens on CompletionTokensDetails, which the accumulator also never initializes. Recursing over model_fields | model_extra picks all of them up without naming any.

Anthropic half, real anthropic==0.93.0, three attempts at 100/50 with cache_creation=(10, 20) and server_tool_use=(2, 3):

after 1: in=100 out=50  eph1h=10 eph5m=20 fetch=2 search=3
after 2: in=200 out=100 eph1h=20 eph5m=40 fetch=4 search=6
after 3: in=300 out=150 eph1h=30 eph5m=60 fetch=6 search=9

Every nested field sums instead of latching, which is the exact symptom from the issue. I also ran the case where a middle attempt omits both sub-models entirely: the totals stay right (eph1h=20 from the two attempts that contributed) and the response still receives the running total, so _zero_numeric_fields on the first-seen shape does the right thing rather than restarting the count. And response.cache_creation is total.cache_creation is False after the merge, so the deep copies are doing their job and a later attempt cannot retroactively mutate an earlier returned response.

Why the generic walk is the right call and not just the prettier one. pyproject.toml pins anthropic==0.93.0. That release has no output_tokens_details and no output_tokens_details.py module at all; it arrives in a later version. Any fix that names the sub-models explicitly has to either import something that does not exist on the pinned SDK or carry a version guard per field. I confirmed this the hard way while reading #2499, whose from anthropic.types.usage import ... OutputTokensDetails raises ImportError on 0.93.0, on every call. This PR never names them, so it accumulates output_tokens_details.thinking_tokens automatically on a newer SDK and ignores it cleanly on the pinned one. That property is worth more than the diff size difference.

The one asymmetry. Numeric fields and sub-models are carried back onto the response when the response omits them, but plain non-numeric fields are not. Measured, attempt 1 carries inference_geo='us' and attempt 2 omits it:

total.inference_geo      'us'
response.inference_geo   None

The numeric case has an explicit elif _is_numeric(total_value): setattr(response, field_name, total_value) and the model case has its own branch, but a string that only the total holds falls through every branch. Since the point of the write-back is that callers reading response.usage see the run totals, service_tier and inference_geo behaving differently from the counters is a small inconsistency. One more branch closes it:

elif total_value is not None:
    setattr(response, field_name, total_value)

Non-blocking, and arguably you want last-write-wins on a string anyway, but it should be a decision rather than a fallthrough.

Small note on _is_numeric. Excluding bool via isinstance(value, Real) and not isinstance(value, bool) is the right call and easy to get wrong, since bool is a Real subclass. Worth a one-line comment so nobody simplifies it later; there is nothing in the current usage models that would fail loudly if someone did.

jxnl added a commit that referenced this pull request Aug 3, 2026
## Summary

- accumulate declared, nested, and unknown numeric OpenAI/Anthropic
usage fields across retries while preserving non-numeric metadata
- add corrective feedback when a Responses API retry receives no tool
call
- preserve raw iterable type hints through sync and async v2
parallel-tool wrappers
- strengthen API-key-free coverage for current and future SDK usage
counters

## Consolidated and superseded items

- closes #2493
- consolidates contributor work from #2498, #2500, and #2501 with
original commit authorship preserved
- supersedes #2497 because it drops unknown `model_extra` counters
- supersedes #2499 because its hand-maintained provider field lists
would drift as SDKs evolve

## Validation

- focused changed-surface suite: `110 passed`
- broad offline v2/coverage suite: `2368 passed, 91 skipped, 73
deselected`
- Ruff check and format check: passed
- scoped `ty check`: passed
- `uv lock --check`: passed
- pre-commit hooks and `git diff --check`: passed

The 73 deselected tests require live provider credentials. An unfiltered
local run confirmed its 22 failures were provider network connections in
the restricted environment; GitHub provider jobs remain the
authoritative validation for those paths.

## Intentionally skipped

- provider additions or expansions: #2436, #2435, #2423, #2409, #2384,
#2322, #2306, #2298, #2283, #2168, #2086; issues #2408, #2383, #2365,
#2260, #2084, #2076
- broad architecture, product, security, or streaming decisions: #2394,
#2392, #2357, #2356, #2355, #2351, #2321, #2307, #2287, #2263; issues
#2479, #2403, #2393, #2391, #2316, #2272, #2056
- dependency batch: #2433
- nontrivial examples and editorial/resource additions: #2468, #2405,
#2401, #2354, #2346, #2311, #2305; issue #2404

These remain open because they need dedicated product, architecture,
provider, security, dependency, or editorial review and are not required
for the `1.15.5` patch release.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Changes retry usage totals and reask message content on failure paths;
scope is limited and heavily covered by tests, with no auth or
data-store changes.
> 
> **Overview**
> Bundles three v2 retry and wrapper fixes for a patch release.
> 
> **Retry usage accounting** replaces hand-maintained token field sums
with generic `_accumulate_models` on Pydantic usage objects. Numeric
fields (including nested models and `model_extra` counters) add across
retries; booleans and other non-numeric metadata are not treated as
billable. OpenAI and Anthropic paths share this logic.
> 
> **OpenAI Responses reask** appends a user correction when
`RESPONSES_TOOLS` validation fails but the output has no tool calls
(e.g. reasoning-only), so retries include feedback instead of repeating
the same request.
> 
> **Parallel tools** in `patch_v2` skips `prepare_response_model` and
does not replace `response_model` with the handler’s prepared wrapper
for parallel modes, keeping raw `Iterable[...]` hints so schemas and
parsed results include every member type.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
bbddca1. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
@jxnl

jxnl commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Consolidated in #2502, now merged to main. Your generic nested/model_extra accumulation commits and authorship were preserved, with additional current/future counter and shape-change coverage added. Closing this source PR as superseded by the consolidation.

@jxnl jxnl closed this Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Retry usage accumulation drops newer billable token fields (Anthropic stale, OpenAI overwritten with None)

3 participants