Repository navigation
Fix wrapped packing duplicating data when the input table is sliced - #6670
Merged
qgallouedec merged 2 commits intoSep 17, 2026
Merged
Conversation
qgallouedec
approved these changes
Sep 17, 2026
qgallouedec
left a comment
Member
There was a problem hiding this comment.
Thanks, verified independently
One thing to correct. The perf claim is not noise:
| main | this PR | |
|---|---|---|
_pack_wrapped, 500k rows |
0.114 / 0.115 ms | 0.138 / 0.135 ms |
_pack_wrapped, 2M rows |
0.296 / 0.305 ms | 0.346 / 0.342 ms |
pack_dataset, 200k rows, bs=1000 |
36.6 / 37.0 ms | 41.3 / 40.9 ms |
pack_dataset, 200k rows, bs=10000 |
15.7 / 15.7 ms | 16.8 / 16.9 ms |
That is +7% to +19%, and your own 0.08 vs 0.09 ms is the same +12%. It works out to a roughly constant 25 us per call, so 4 ms on a 200k-row dataset, once, and cached afterwards. Not a reason to hold this though.
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes #6669
_pack_wrappedsizes the output from the array's sliced view but reads from the raw child buffer, so when the input table is a slice it re-reads from the start of the buffer instead of from the slice.datasets.map(batched=True)passes eachbatch as a slice, so every batch after the first can come back holding data from the beginning of the dataset. Row and token counts are unchanged, so nothing errors.
The correction was already computed into a local, but #5189 rewrote the loop as a list comprehension over all columns and passed
column.values, which left that local unused. This switches tocolumn.flatten(), which respects the list array's own offset, per column so each keeps its own offsets.Only
wrappedis affected.bfdandbfd_splithit the same pattern but are protected by thepc.filterandpc.takethat run before it.Output is byte-identical to current behavior when the table is not sliced, so no existing correct result changes. Also checked on
large_listwith int64 offsets.Tests
Added
TestPackDatasetWrapped::test_with_multiple_map_batches, which packs across twomapbatches. It fails on main with the duplication above and passes here. The two existingwrappedtests use 3-row datasets, so they only ever exercise a single batch, which is why this was not caught.Beyond the added test, I checked
_pack_wrappedagainst an independent reference (concatenate each column's rows in order, then re-chunk) over 600 randomized shapes plus targeted edge cases: slices at every offset,seq_lengthof 1 andequal to and greater than the total, ragged and empty rows, zero-row tables, multi-chunk tables, 1 to 3 columns,
listandlarge_list, int32 and int64 offsets. All pass with the fix and 127 of them fail without it.pack_datasetalso round-trips end to end across batch sizes 1 to 777,num_proc2 and 4, and all three strategies, withinput_ids/attention_mask/labelsstaying aligned.Since #5189 was a performance PR, I benchmarked this too.
flatten()is metadata work likevalues, so throughput is unchanged (at 500k rows x 16 tokens the median of 7 runs is 0.08 ms before and 0.09 ms after, which is within noise).Before submitting
AI writing disclosure
Who can review?
@qgallouedec @albertvillanova
Note
Medium Risk
The change alters training preprocessing for wrapped packing on multi-batch maps (silent wrong tokens before); unsliced inputs should stay byte-identical per the PR.
Overview
Fixes wrapped sequence packing when
pack_datasetruns through batchedDataset.map: batches after the first arrive as PyArrow table slices, and_pack_wrappedwas sizing output from the slice but reading token data fromcolumn.values(the full child buffer), so later batches could repeat tokens from the start of the dataset instead of the current batch.The implementation now uses
column.flatten()per column so list data honors each array’s offset, then rebuilds packed list columns from those flattened values.bfd/bfd_splitare unchanged in this diff.Adds
test_with_multiple_map_batchesto pack withmap_kwargs={"batch_size": 3}across two map batches and assertinput_ids/attention_maskstay correct.Reviewed by Cursor Bugbot for commit 34ad2ad. Bugbot is set up for automated code reviews on this repo. Configure here.