Skip to content

feat: client-side embedding training & multi-turn rollout support#247

Draft
Yunnglin wants to merge 1 commit into
mainfrom
feat/client-embedding-multiturn-support
Draft

feat: client-side embedding training & multi-turn rollout support#247
Yunnglin wants to merge 1 commit into
mainfrom
feat/client-embedding-multiturn-support

Conversation

@Yunnglin

Copy link
Copy Markdown
Collaborator

Summary

Exposes the core-library embedding training (InfoNCE) and trainable multi-turn rollout capabilities through twinkle_client, and cleans up the client_generator technical debt. tinker client stays untouched — only documented for its Gateway-based indirect (generation-only) multi-turn path.

Technical debt cleanup

  • Remove client_tools/client_generator.py; convert affected client modules (dataloader/dataset/processor/model/sampler) to hand-maintained sources by stripping their AUTO-GENERATED headers (byte-for-byte otherwise unchanged).

Shared bridge function

  • Extract bridge-token stitching into the shared pure function twinkle_agentic/rollout/bridge.py::extend_with_bridge. MultiTurnRollout now calls it instead of maintaining its own copy, so the core-lib and client paths cannot drift.

Client multi-turn rollout

  • Add ClientMultiTurnRollout (src/twinkle_client/rollout/) driving HTTP sampling via vLLMSampler.sample(), reusing ToolManager and `extend_with_brid

Technical debt cleanup

  • Remove client_tools/client_generator.py; convert affected client modules (dataloader/dataset/processor/model/sampler) to hand-maintained sources be. Backward compatible by default.

Verification

  • Embedding: e2e scaffolding + local mock protocol-link validation (Property 1); GPU-gated r

Shared bridge function

  • Extract bridge-token stitching into the shared pure function twinkle_agentic/rollout/bridge.py::extend_with_bridge. MultiTurnRollout now calls it instead of maintaining its own copy, so the core-lib and cU-g- Extract bridge-token stme

Client multi-turn rollout

  • Add ClientMultiTurnRollout (src/twinkle_client/rollout/) driving HTTP sampling via vLLMSampler.sample(), reusing ToolManager and `extend_with_brid

Technical debt cleanup

  • Remove client_totib- Add ClientMultiTurnRollou
    -### Technical debt cleanup
  • Remove client_tools/client_generator.py; convert affected client modules (dataloader/dataset/processor/model/samplega- Remove client_tools/clGP

Verification

  • Embedding: e2e scaffolding + local mock protocol-link validation (Property 1); GPU-gated r

Shared bridge function

  • Extract bridge-token stitching into the shared pure fser- Embedding: e2nn### Shared bridge function
  • EOF

'

Notes

  • GPU-gated e2e cases require a running server (tests/server/start_e2e_server.py) and cannot be validated locally.

Expose the core-library embedding training (InfoNCE) and trainable
multi-turn rollout capabilities through twinkle_client, and clean up the
client_generator technical debt.

- Remove client_tools/client_generator.py; convert affected client modules
  (dataloader/dataset/processor/model/sampler) to hand-maintained sources
  by stripping their AUTO-GENERATED headers.
- Extract bridge-token stitching into the shared pure function
  twinkle_agentic/rollout/bridge.py::extend_with_bridge; MultiTurnRollout
  now calls it instead of duplicating the logic.
- Add ClientMultiTurnRollout (src/twinkle_client/rollout/) driving HTTP
  sampling via vLLMSampler, reusing ToolManager and extend_with_bridge.
- Enhance MockSampler with opt-in multi-turn knobs (new_input_feature,
  configurable stop_reason / tool_call_text / tool_call_turns) for local
  CPU-only multi-turn e2e; backward compatible by default.
- Add embedding e2e scaffolding + local mock protocol-link validation and
  GPU-gated numeric/SP-CP tests; add multi-turn property/unit tests, local
  mock e2e, and GPU-gated alignment / Gateway-logprobs tests.
- Document client embedding call sequence and the Tinker Gateway indirect
  multi-turn approach with its capability boundaries.

Local CPU suite: 98 passed, GPU-gated cases skip cleanly.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces client-side multi-turn agentic rollout orchestration (ClientMultiTurnRollout) and adds documentation and tests for embedding training and multi-turn tool calling. Feedback focuses on ensuring proper JSON serialization of PyTorch tensors and template outputs in the HTTP client path, as well as removing an unused variable in the mock sampler.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +27 to +28
if isinstance(obj, np.ndarray):
return obj.tolist()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The _to_plain function recursively converts numpy arrays to lists, but it does not handle PyTorch Tensor objects. In multimodal scenarios, mm_token_type_ids is padded and concatenated as a torch.Tensor (lines 139-147), which remains a Tensor after _to_plain is called. When the client attempts to serialize this to JSON to send over HTTP, it will raise a TypeError. We should update _to_plain to also convert PyTorch tensors to lists if they are present, using a safe string-based type check to avoid importing torch in environments where it is not installed.

Suggested change
if isinstance(obj, np.ndarray):
return obj.tolist()
if isinstance(obj, np.ndarray):
return obj.tolist()
if type(obj).__name__ == 'Tensor' and hasattr(obj, 'tolist'):
return obj.tolist()

from twinkle.data_format import Trajectory
from twinkle.data_format.sampling import SamplingParams
from twinkle.template.base import Template
from twinkle_agentic.rollout.bridge import extend_with_bridge

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Import _to_plain from twinkle_agentic.rollout.bridge so that we can convert the initial pif returned by self.template.encode into a plain dictionary with serializable types before sending it over HTTP.

Suggested change
from twinkle_agentic.rollout.bridge import extend_with_bridge
from twinkle_agentic.rollout.bridge import extend_with_bridge, _to_plain

Comment on lines +113 to +114
pif = self.template.encode(traj, add_generation_prompt=True)
pif.setdefault('messages', list(traj.get('messages', [])))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The initial pif returned by self.template.encode is not run through _to_plain before being used. If the template returns an InputFeature object containing numpy arrays or PyTorch tensors (which is standard for real templates), calling .setdefault might fail if it's not a dict, and sending it over HTTP on line 139 will cause a JSON serialization error. We should convert the initial pif to a plain dictionary using _to_plain just like the core-library MultiTurnRollout does.

Suggested change
pif = self.template.encode(traj, add_generation_prompt=True)
pif.setdefault('messages', list(traj.get('messages', [])))
pif = self.template.encode(traj, add_generation_prompt=True)
pif = _to_plain(pif)
pif.setdefault('messages', list(traj.get('messages', [])))

Comment on lines +26 to +29
# Recognised ctor / per-call knobs that tune multi-turn control flow. Kept in a
# set so the ctor can log only *genuinely* unknown kwargs (a real backend
# signature drift) while still accepting these opt-in multi-turn knobs.
_MULTI_TURN_KNOBS = ('stop_reason', 'tool_call_text', 'tool_call_turns')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The _MULTI_TURN_KNOBS tuple is defined here but never used anywhere in the codebase. Furthermore, the comment mentions it is 'Kept in a set', but it is defined as a tuple. Since the constructor explicitly defines stop_reason, tool_call_text, and tool_call_turns as keyword-only arguments, Python's native argument binding handles them automatically, making this variable completely redundant. We should remove this unused variable and its comment.

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