Skip to content

[qoc] Make concatenate_generator_outputs linear instead of O(K^2)#1535

Merged
CharlieFRuan merged 3 commits intomainfrom
charlie/fix-concat-generator-outputs
Apr 20, 2026
Merged

[qoc] Make concatenate_generator_outputs linear instead of O(K^2)#1535
CharlieFRuan merged 3 commits intomainfrom
charlie/fix-concat-generator-outputs

Conversation

@CharlieFRuan
Copy link
Copy Markdown
Member

@CharlieFRuan CharlieFRuan commented Apr 19, 2026

Summary

`concatenate_generator_outputs` used the `sum([go[key] for go in gens], [])` pattern to flatten each list-valued field, which is O(K² · L̄) — every `+` copies the running result. Replace with an explicit extend loop (O(N_total)). No signature change; no behavior change (existing test passes unmodified).

Also move related tests to tests/train/generators/test_generator_output_utils.py

Benchmark

Config: 8 trajectories per GeneratorOutput, 64k-token `response_ids`/`loss_masks`/`rollout_logprobs`, six flatten calls per concat (as currently done).

K (GeneratorOutputs) Total trajectories Old (sum,[]) New (extend) Speedup
128 1,024 0.6 ms 0.1 ms 7.6×
512 4,096 8.6 ms 0.2 ms 49.8×

Speedup grows quadratically with K. This matters when concat is called on per-trajectory chunks — e.g. the prefix-aware merging work in #1532 calls `concatenate_generator_outputs` with K = number of trajectories (~2560 in a typical SearchR1 run), which would extrapolate to ~200 ms under the old path and sub-millisecond under the new one.

Test plan

  • `pytest tests/train/generators/test_skyrl_gym_generator.py::test_generator_output_concatenation` — passes unchanged (no signature or behavior change).
  • `pytest tests/train/generators/ tests/train/test_generator_postprocess.py tests/train/test_trainer_utils.py` — 137/137 pass.

🤖 Generated with Claude Code


Open in Devin Review

gemini-code-assist[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

CharlieFRuan and others added 3 commits April 20, 2026 00:17
The previous ``sum([go[key] for go in gens], [])`` pattern repeatedly
rebuilds the running concatenation, making the flattening step
O(K^2 * L̄) in the number of GeneratorOutputs. Replace with an
explicit extend loop (O(N_total)). No behavior change, no signature
change.

Benchmarked with 8 trajectories per GeneratorOutput, 64k-token
response_ids / loss_masks / rollout_logprobs, six flatten calls
per concat:

  K=128 (1024 trajectories): 0.6ms -> 0.1ms  ( 7.6x)
  K=512 (4096 trajectories): 8.6ms -> 0.2ms  (49.8x)

The speedup grows quadratically with K, which matters when concat
is called on per-trajectory chunks (e.g. the prefix-aware merging
in #1532).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@CharlieFRuan CharlieFRuan force-pushed the charlie/fix-concat-generator-outputs branch from c8d3259 to 4854b93 Compare April 20, 2026 00:20
@CharlieFRuan CharlieFRuan merged commit bb6a730 into main Apr 20, 2026
6 of 7 checks passed
@CharlieFRuan CharlieFRuan deleted the charlie/fix-concat-generator-outputs branch April 20, 2026 00:31
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