[Tinker API] Restore variable-length result order - #2043
Conversation
d900e34 to
e2dcd27
Compare
8e9a895 to
20bd159
Compare
20bd159 to
614d03c
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces support for the GSPO loss function, including batch normalization, row selection, and pruning metrics. It also implements sequence-based scheduling using sequence IDs to ensure requests are processed in order, alongside corresponding API validation and unit tests. The review feedback highlights three key issues: a potential ValueError when zipping outputs if per_sample_outputs is empty, a potential permanent hang during sequence bootstrapping if the database is reset and a non-zero sequence ID is used, and a potential AttributeError if metrics is None in pipeline parallel setups.
erictang000
left a comment
There was a problem hiding this comment.
some initial comments, need to look closer at the seq_id/engine logic changes as well!
150c2d8 to
0e3f075
Compare
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 0e3f075. Configure here.
0e3f075 to
fdc3307
Compare
Signed-off-by: Dian Ang <23232359+yapdianang@users.noreply.github.com>
fdc3307 to
ee28351
Compare

Summary
Before
Token-based batching returned results in packed microbatch order. A client could pair one example's mask with another example's log probabilities and fail before the optimizer step.
After
SkyRL maps each result back to the original example index before it returns
loss_fn_outputs. The change applies to Trajectory custom GSPO and any other client that reads per-example results.Testing
18 passed.