fix(utils): batched embed drops meta.tokens and image billed units - #784
fix(utils): batched embed drops meta.tokens and image billed units#784arthi-arumugam-git wants to merge 1 commit into
Conversation
afab009 to
a54fb55
Compare
|
Good catch from the bot, the User-Agent string was still on 7.0.8 while X-Fern-SDK-Version and pyproject.toml were on 7.0.9. Pushed a fix so both read cohere/7.0.9. Note that #785 also bumps 7.0.8 -> 7.0.9, so whichever of the two lands second will need its bump rebased. Happy to rebase on request, or drop the bump commit from one of them if you would rather handle versioning at release time. |
Client.embed() sends every call through merge_embed_responses unless you
pass batching=False or images, and that path rebuilds ApiMeta from
scratch in merge_meta_field. The rebuild only sets api_version, four of
the six ApiMetaBilledUnits fields, and warnings.
So meta.tokens, meta.cached_tokens, billed_units.images and
billed_units.image_tokens are discarded on the way out. The same request
returns different metadata depending on whether batching is on:
co.embed(texts=texts).meta.tokens # None
co.embed(texts=texts, batching=False).meta.tokens # populated
Anyone reading meta.tokens for usage accounting silently gets nothing.
Sum the missing fields the same way the existing four are summed.
cached_tokens sits directly on ApiMeta so it sums off the meta list
rather than the billed_units list. tokens stays None when no input meta
carried it, so a merged response with no token counts looks exactly like
it does today and the existing equality assertions still hold.
merge_meta_field names every field it copies by hand, which is why these
four went missing when they were added to the models. One of the tests
drives its assertions off the model fields instead of a fixed list, so
the next field added to ApiMeta fails there rather than silently
disappearing from merged responses.
a54fb55 to
0878e27
Compare
|
Pushed a cleanup pass on both this and #785. Dropped the version bump from both. They cannot both go to 7.0.9, and Also added a test here that drives its assertions off the model fields instead of a hardcoded list. |
|
Some prior art I found after opening this, which is worth linking. Issue #711 reported PR #714 spotted that and has been open since 2025-12-28. Its model change was overtaken by the regeneration; its one line adding That is also the argument for the model-driven test rather than another hardcoded assertion. A field was added to the model, the merge was not updated, an issue was filed, the issue was closed, and the bug survived all of it because nothing failed. The test I added fails in that situation. |
|
Bugbot reviewed afab009, which is no longer in this branch. The rebase at 0878e27 drops the version bump and the Reason for dropping it rather than fixing it: two open PRs cannot both claim 7.0.9, and This PR now touches |
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 0878e27. Configure here.
Client.embed()goes throughmerge_embed_responseson every call unless you passbatching=Falseor pass images, and that path rebuildsApiMetafrom scratch insidemerge_meta_field. The rebuild setsapi_version, four of the sixApiMetaBilledUnitsfields, andwarnings. Everything else on the metadata is dropped on the floor:meta.tokens,meta.cached_tokens,billed_units.imagesandbilled_units.image_tokens.Since
batchingdefaults toTrue, the default path is the lossy one:Same request, different metadata, and nothing in the signature suggests that a batching flag should change which usage numbers come back. Anyone reading
meta.tokensfor accounting getsNoneand no error to tell them why.The fix sums the four missing fields exactly the way the existing four are summed.
cached_tokenssits directly onApiMetarather than underbilled_units, so it sums off the meta list instead of the billed units list.One deliberate choice worth flagging:
tokensstaysNonewhen none of the input metas carried it, rather than building anApiMetaTokens(None, None). A merged response with no token counts then looks exactly like it does today, which is why the existing equality assertions intest_embed_utils.pydid not need touching.Why this happened, and stopping it happening again
merge_meta_fieldlists the fields it copies by hand.images,image_tokens,tokensandcached_tokenswere added to the models and the merge was never updated, and nothing failed. The same thing will happen to the next field added.So one of the three tests drives its assertions off the model fields rather than a hardcoded list:
Add a field to
ApiMetaBilledUnitsorApiMetaTokenswithout touchingmerge_meta_fieldand that test fails immediately. I verified it fails onmainas it stands.The other two are ordinary:
test_merge_meta_field_keeps_tokens_and_image_unitscovers the summing and usesoutput_tokens=0on both inputs so a falsy value is not confused with an absent one, andtest_merge_meta_field_leaves_tokens_unset_when_absentpins theNonebehaviour so it does not get simplified away later.Notes
src/cohere/utils.pyandtests/are touched, both listed in.fernignore.mypy .clean across 341 files,tests/test_embed_utils.py9 passed.7.0.8 -> 7.0.9bump following fix(utils): embed(texts=[]) crashes with IndexError on empty input #778 and feat(client): forward max_retries through Client/ClientV2 constructors #779 and have dropped it. I have a second fix open againstutils.pyat the same time and they cannot both bump to the same version, andcore/client_wrapper.pyis Fern generated rather than hand maintained, so the version looks like yours to set at release. Say the word if you would rather the bump were in here and I will add it back.Note
Low Risk
Localized change to embed response merging and tests; fixes incorrect metadata rather than altering auth or API contracts.
Overview
Fixes silent loss of usage metadata when
Client.embed()merges multiple batch responses (defaultbatching=True).merge_meta_fieldnow sumsbilled_units.images,billed_units.image_tokens,meta.tokens(input/output), andcached_tokensalongside the fields it already merged, so batched calls return the same usage shape as a single request.tokensstaysNonewhen no input meta had token counts (no emptyApiMetaTokens), keeping merged responses aligned with unbatched ones when token data was never present.Tests cover image/token summing, absent-token behavior, and a model-driven check that every numeric field on
ApiMetaBilledUnits/ApiMetaTokensis merged so future model fields cannot be dropped without failing CI.Reviewed by Cursor Bugbot for commit 0878e27. Bugbot is set up for automated code reviews on this repo. Configure here.