perf(sample-support): carry the captured support to the trainer as one packed field - #2083
perf(sample-support): carry the captured support to the trainer as one packed field#2083dyurk-lila wants to merge 17 commits into
Conversation
Extract the single-request HTTP generation path out of RemoteInferenceClient into a standalone RemoteGenerateClient plus a RemoteGenerateResult dataclass, so routed-expert results can be obtained without constructing the full inference/control-plane client. RemoteInferenceClient now owns an internal RemoteGenerateClient and delegates session management, _post, and _generate_single to it. Endpoint routing, retry/backoff, cache_salt handling, serialization, and lifecycle behavior are unchanged.
For multi-turn rollouts, build routed-expert (R3) trace data incrementally as the conversation grows instead of re-gathering the whole conversation's routes on every turn. - Add `routed_experts_prompt_starts` to `InferenceEngineInput` and thread a per-request `routed_experts_prompt_start` through `RemoteInferenceClient` and `RemoteInferenceGenerator.generate()`, forwarded as a sampling param so the engine only returns routes for the newly generated suffix. - Introduce `TokenMetadataTrace` (token-aligned array accumulator) and `RoutedExpertTrace`, which records each generation's routes and finalizes a full per-token routed-expert array with loss-mask-aware terminal padding. - Track a `RoutedExpertTrace` on `AgentLoopState` and record routes per turn, replacing the previous whole-conversation re-gather in `SkyRLGymGenerator.agent_loop`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a packed side-channel mechanism to efficiently transmit and handle per-token metadata (such as MoE routing indices and sampler support sets) during training and generation, replacing padded representations with a PackedTensor layout to optimize memory usage. Feedback on these changes highlights three key improvement opportunities: catching ValueError alongside JSONDecodeError in the remote inference client to allow retries on transient errors while immediately raising permanent layout drift errors; returning an empty array instead of None in skyrl_gym_generator.py when the sample support trace has zero rows to prevent batch concatenation failures; and optimizing PackedTensor.cat to return the single batch directly when only one element is provided.
| try: | ||
| raw = await resp.read() | ||
| body = load_packed_body(raw) if packed_side_channels else orjson.loads(raw) | ||
| except orjson.JSONDecodeError as exc: |
There was a problem hiding this comment.
If load_packed_body raises a ValueError (e.g., due to a truncated response or unterminated base64), it will not be caught by except orjson.JSONDecodeError, causing the request to fail immediately without retrying. We should catch ValueError as well, but re-raise it if it is a permanent layout or drift error to preserve the intended behavior for permanent failures.
except (orjson.JSONDecodeError, ValueError) as exc:
if isinstance(exc, ValueError) and not isinstance(exc, orjson.JSONDecodeError):
if any(msg in str(exc) for msg in ("drifted", "unspliced", "found no envelope")):
raise| if agent_loop_state.sample_support_trace is not None and agent_loop_state.sample_support_trace.num_rows: | ||
| rollout_sample_support_out = agent_loop_state.sample_support_trace.finalize( | ||
| token_count=len(response_ids), | ||
| extra_rows=final_observation_token_count, | ||
| ) |
There was a problem hiding this comment.
If agent_loop_state.sample_support_trace is not None but num_rows is 0 (e.g., if the environment starts in a terminal state and the loop is skipped), rollout_sample_support_out remains None. This will cause concatenate_generator_outputs to fail with a ValueError due to a mix of None and np.ndarray values in the batch. We should return an empty array of shape (0, sample_support_width) when num_rows is 0 to ensure consistency.
| if agent_loop_state.sample_support_trace is not None and agent_loop_state.sample_support_trace.num_rows: | |
| rollout_sample_support_out = agent_loop_state.sample_support_trace.finalize( | |
| token_count=len(response_ids), | |
| extra_rows=final_observation_token_count, | |
| ) | |
| if agent_loop_state.sample_support_trace is not None: | |
| if agent_loop_state.sample_support_trace.num_rows: | |
| rollout_sample_support_out = agent_loop_state.sample_support_trace.finalize( | |
| token_count=len(response_ids), | |
| extra_rows=final_observation_token_count, | |
| ) | |
| else: | |
| rollout_sample_support_out = np.empty((0, sample_support_width), dtype=SAMPLE_SUPPORT_DTYPE) |
| def cat(batches: Sequence["PackedTensor"]) -> "PackedTensor": | ||
| if not batches: | ||
| raise ValueError("cannot cat an empty list of packed batches") | ||
| lengths = torch.cat([batch.sequence_lengths for batch in batches]) | ||
| return PackedTensor( | ||
| torch.cat([batch.values for batch in batches], dim=0), | ||
| cu_seqlens_from_lengths(lengths, device=batches[0].device), | ||
| ) |
There was a problem hiding this comment.
When batches contains only a single PackedTensor, we can avoid the overhead of torch.cat and cu_seqlens_from_lengths by returning the single batch directly. This is a simple and effective performance optimization.
@staticmethod
def cat(batches: Sequence["PackedTensor"]) -> "PackedTensor":
if not batches:
raise ValueError("cannot cat an empty list of packed batches")
if len(batches) == 1:
return batches[0]
lengths = torch.cat([batch.sequence_lengths for batch in batches])
return PackedTensor(
torch.cat([batch.values for batch in batches], dim=0),
cu_seqlens_from_lengths(lengths, device=batches[0].device),
)There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit ec57e6f. Configure here.
| int(sampled[rows[0]]), | ||
| int(rows[0]), | ||
| ) | ||
| return sampled_logprobs, support_ids |
There was a problem hiding this comment.
Unclamped logprobs break sample-support wire
High Severity
When return_sample_support is on, sampled logprobs are taken straight from flat processed_logprobs and never passed through build_logprobs_content. Non-finite sampled scores that vLLM emits intermittently therefore reach orjson.dumps, which rejects them and fails the /skyrl/v1/generate response. That breaks the capture path that training relies on for support-aware logprob renormalization.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ec57e6f. Configure here.


Problem
PR 30 produces one
[response_tokens, top_k]support array per trajectory. Expanding those arrays to[batch, sequence_length, top_k]would allocate a large prompt region containing only-1padding and repeat the same copies that packed routed-expert transport removed.This PR carries support from generator output to trainer workers as one packed response-token field, without yet changing model scores.
Training representation
rollout_sample_supportis aPackedTensorwith:values:[sum(response_lengths), top_k]int32 support IDs; andcu_seqlens: one response-segment boundary per trajectory.Unlike routed experts, which cover real prompt and response tokens, sample support covers response tokens only. Preprocessing requires exactly one support row per response token and a common support width across the batch. It performs one pooled fill into the final packed allocation and retains PR 29’s zero-copy object-store transport.
The field is threaded through generator-output concatenation and step-wise merging, training-input construction, slicing and chunking, replay-buffer storage, device transfer, pinning, and both sample- and token-based batch iterators.
Model-position alignment
The packed support values are not padded or shifted through every intermediate layout. Instead, each microbatch derives a cheap int64 row-ID channel:
values;rat the model position whose logits predict that response token;-1where no support row exists; andRow IDs are derived after slicing because
PackedTensor.chunk, padding, and microbatch selection rebase the packed row space. Persisting IDs from the global batch would point at the wrong support rows.Padding rules
Packed fields have explicit, field-specific padding rules:
The rules are registered centrally so adding another packed field cannot silently inherit routed-expert semantics. Ordinary batch padding copies the source segment length where appropriate, while data-parallel equalization uses the field’s synthetic-row rule.
Testing
Note
Medium Risk
Changes how per-token training side channels are collated, padded, sliced, and aligned for Megatron layouts. Incorrect packing or padding would silently misalign support rows with response tokens.
Overview
Carries captured sampler support into training as one packed field instead of a dense
[batch, seq, top_k]rectangle.rollout_sample_supportis aPackedTensorof[sum(response_lengths), top_k]IDs pluscu_seqlens, covering response tokens only (unlike packed MoE routes, which cover real prompt+response tokens).Preprocessing validates a shared top-k width and exact response lengths, then fills the packed buffer with the same pooled first-touch path used for routes. Zero-copy object-store transport is reused. Packed fields now have explicit padding rules: dummy distinct-expert rows for R3, empty segments for synthetic support rows, so a new packed field cannot inherit router-replay padding.
Support is not expanded through every layout. Each microbatch derives int64 row IDs after slice/pad (because packing rebases the row space), placing response row
rat the logit that predicts that token and-1elsewhere. Scoring is not wired up yet.Reviewed by Cursor Bugbot for commit ec57e6f. Bugbot is set up for automated code reviews on this repo. Configure here.