fix(cohere): drop redundant _try_to_dict pass in tool-call iteration#586
Merged
Abhijeet Prasad (AbhiPrasad) merged 1 commit intoJul 17, 2026
Conversation
`_iter_tool_calls` ran `_try_to_dict(output)` (Pydantic `model_dump`) on every chat/stream response just to probe for `tool_calls`, even when none were present. Replaced with `_get_field(output, "tool_calls")`, which already handles both dict and Pydantic-object shapes. Downstream tool-call helpers already read via `_get_field`, so behavior is preserved and the extra recursive walk on top of `SpanImpl.log_internal`'s `bt_safe_deep_copy` is gone. Same spirit as #583 (anthropic) — Braintrust serializes at send/log time, so integrations shouldn't pre-serialize. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Abhijeet Prasad (AbhiPrasad)
approved these changes
Jul 17, 2026
Abhijeet Prasad (AbhiPrasad)
enabled auto-merge (squash)
July 17, 2026 16:14
Abhijeet Prasad (AbhiPrasad)
deleted the
fix/cohere-drop-redundant-tool-call-serialization
branch
July 17, 2026 16:19
3 tasks
Abhijeet Prasad (AbhiPrasad)
pushed a commit
that referenced
this pull request
Jul 17, 2026
## Summary
Audit of `py/src/braintrust/integrations/huggingface_hub/` against
`.agents/skills/sdk-integrations/SKILL.md`. Two related fixes:
- **Drop eager `_try_to_dict` in stream tool-call merge.**
`_merge_tool_call_delta` was running `_try_to_dict(entry)` (Pydantic
`model_dump("python")`) on every streaming tool-call delta chunk just to
read `index` / `id` / `type` / `function` fields.
`SpanImpl.log_internal` already runs `bt_safe_deep_copy` at send time,
so the eager conversion was a redundant recursive walk on the hot path.
Same pattern as #586 (cohere) and #590 (google_genai).
- **Remove the `_get_field(obj, key)` dict-or-attr helper.**
HuggingFace's `BaseInferenceType` (parent of every response type and
stream chunk on both matrix pins — `latest`=1.23.0 and floor=`0.32.0`)
inherits from `dict`:
```
>>> from huggingface_hub.inference._generated.types.base import
BaseInferenceType
>>> BaseInferenceType.__mro__
(BaseInferenceType, dict, object)
```
So `.get()` works uniformly on every response, chunk, delta, and nested
field — the dict-or-attr dispatch was dead cleverness. The two places
that read from an `InferenceClient` instance now use `getattr(instance,
key, None)` directly.
Net diff: **60 insertions / 89 deletions**, one file. Also picked up two
small invariants that were previously implicit via `_get_field`:
- Tool-call-delta loop now checks `isinstance(entry, dict)` (tighter
than `is None`) and drops the corresponding `isinstance(incoming_fn,
dict)` re-check inside.
- `_text_generation_extra_metadata` gets an explicit `if details is
None: return {}` guard at the top.
**Everything else was already SKILL-aligned** and needed no change:
- `metadata.provider` always set (kwarg > instance > `"huggingface"`
fallback), `metadata.model` set when known.
- Tool definitions routed to `metadata.tools` via `_CHAT_METADATA_KEYS`
allowlist, not into `input`.
- All metadata extraction is allowlist-based — no denylists anywhere in
the file.
- `context.span_origin.instrumentation.name = "huggingface-hub-auto"`
via local `start_span` helper.
- Only spec-listed metric keys.
- One accumulated span per stream, errors propagate, setup +
`wrap_huggingface_hub` idempotent.
## Test plan
- [x] `CI=1 BRAINTRUST_TEST_PACKAGE_VERSION=latest pytest
src/braintrust/integrations/huggingface_hub/test_huggingface_hub.py` —
24/24 passed (huggingface-hub 1.23.0)
- [x] `CI=1 BRAINTRUST_TEST_PACKAGE_VERSION=0.32.0 pytest
src/braintrust/integrations/huggingface_hub/test_huggingface_hub.py` —
22 passed, 2 pre-existing skips for text_generation on that pin
(`_skip_if_text_generation_unavailable`)
- [x] **No cassettes re-recorded, no new mocks/fakes** — used the
existing VCR-backed suite.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- sfk:created-approved-by -->
Created by abhijeet
<!-- sfk:slack-thread -->
[Slack
thread](https://starfolkai.slack.com/archives/C0AQDETAVT3/p1784312976815939?thread_ts=1784312976.815939&cid=C0AQDETAVT3)
Co-authored-by: Starfolk <noreply@starfolk.ai>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_iter_tool_callsran_try_to_dict(output)(Pydanticmodel_dump("python")) on every chat/stream response just to probe fortool_calls, even when none were present — an extra recursive walk on top of the oneSpanImpl.log_internalalready runs viabt_safe_deep_copy._get_field(output, "tool_calls"), which already handles both dict and Pydantic-object shapes. Downstream tool-call helpers (_tool_call_name,_tool_call_input,_tool_call_metadata) all read via_get_field, so behavior is preserved.sdk-integrationsskill guidance ("Do not over-serialize. Braintrust serializes at send/log time.") and mirrors fix(anthropic): drop redundant bt_safe_deep_copy passes in tracing #583 (anthropic).Test plan
nox -s "test_cohere(latest)"(cohere==7.0.5) — 21 passednox -s "test_cohere(5.0.0)"— 10 passed, 11 legitimately skipped for v2/audio surfaces not present on 5.0.0test_cohere.py, which already asserts tool-call span parent/child structure for both v1 (test_wrap_cohere_chat_v1_tool_call_spans) and v2 (test_wrap_cohere_chat_v2_tool_call_spans).🤖 Generated with Claude Code
Created by abhijeet
Slack thread