Skip to content

[Brook G2] Converted transfer types nested in JSON values. - #368

Open
moetemp wants to merge 1 commit into
moe/brook/review/basefrom
moe/brook/review/g2-nested-transfer-types
Open

moetemp wants to merge 1 commit into
moe/brook/review/basefrom
moe/brook/review/g2-nested-transfer-types

Conversation

@moetemp

@moetemp moetemp commented Oct 25, 2026 •

Copy link
Copy Markdown
Owner

What was changed

This PR makes the JSON converter convert transfer type values that are nested in a JSON value, such as a list item or a dataclass field. The extra lookup costs about 3% on a decode of many small dataclasses.

  • A nested proto transfer type is written as its proto JSON.
  • The decode uses the converter of the type hint's class, and passes a generic hint such as NestedValue[int] whole.
  • A dataclass with its own transfer type stays as its fields inside a JSON value, so payloads written since 1.31.0 still decode.

Why?

Today the converter runs only at the top level of a payload, so a nested transfer type value fails to encode. Stream handles (#373) are the first users, and big-payload handles may be next. Stream handles and big-payload handles has the discussion.

Part of the Brook review series.

Checklist

  1. How was this tested:
    • test_transfer_type_values_nested_in_json, test_proto_transfer_type_nested_in_json_is_its_proto_json and test_transfer_type_dataclass_in_a_top_level_list_keeps_its_fields in tests/test_converter.py.

The JSON converter used a `transfer_type_convertible` converter only at
the top level of a payload, so such a value in a list or a dataclass
field failed to encode. A nested dataclass is still written as fields.
The lookup runs after the primitive checks, so it doesn't slow them.
@moetemp
moetemp force-pushed the moe/brook/review/g2-nested-transfer-types branch from 7d10c5a to 480f581 Compare October 28, 2026 07:54
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.

1 participant