fix(data): fix _pack_wrapped corrupting data on sliced input tables - #6733
Open
AuthRan wants to merge 1 commit into
Open
fix(data): fix _pack_wrapped corrupting data on sliced input tables#6733AuthRan wants to merge 1 commit into
AuthRan wants to merge 1 commit into
Conversation
`_pack_wrapped` computed a fresh set of 0-based offsets sized to the current table's own slice of `columns[0]`'s child buffer, then paired those offsets with every column's raw, unsliced `.values` buffer. If the incoming table was itself a zero-copy slice of a larger buffer (e.g. one batch of a `dataset.map(..., batched=True)` call), each column's real data starts at some non-zero offset into that raw buffer, so pairing it with 0-based offsets silently duplicated earlier rows and dropped the actual ones -- with no error. Slice each column's `.values` down to its own offset bounds before building the new column, mirroring what was already done for `columns[0]` to compute `num_elements`. Regression from huggingface#5189. Only the "wrapped" strategy was affected; "bfd"/"bfd_split" build fresh arrays via `pc.take` and were not impacted. Closes huggingface#6669
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_wrapped(the"wrapped"strategy ofpack_dataset) computesa fresh set of 0-based offsets sized to
columns[0]'s own slice ofits child buffer, then reuses those offsets against every column's
raw, unsliced
.valuesbuffer:If the table handed to
_pack_wrappedis itself a zero-copy slice ofa larger underlying buffer — which happens routinely, e.g. once
dataset.map(..., batched=True)moves past the first batch — eachcolumn's real data starts at some non-zero offset into the raw
buffer. Pairing that raw buffer with 0-based offsets silently
duplicates earlier rows and drops the real ones, with no error or
warning:
This is a regression from #5189. Only
"wrapped"is affected;"bfd"/"bfd_split"build fresh arrays viapc.takeand don't sharethis issue.
The fix
Slice each column's
.valuesdown to its own offset bounds beforepairing it with the new offsets, the same way it was already (only)
done for
columns[0]to computenum_elements.Before submitting
pack_dataset(..., strategy="wrapped", map_kwargs={"batch_size": 3})); confirmed it fails against the pre-fix code and passes against the fix.ruff check/ruff formaton the changed files.TestPackDatasetWrapped/TestPackDatasetBfdtests still pass.Note
Medium Risk
Fixes incorrect training data for wrapped packing under multi-batch map; scope is narrow but wrong packed tokens could have affected downstream LM training.
Overview
Fixes silent data corruption in
pack_datasetwhen using thewrappedstrategy on batchedmapinput (e.g.map_kwargs={"batch_size": 3}). After the first batch, Hugging Facedatasetsoften passes zero-copy slices of a larger Arrow buffer;_pack_wrappedbuilt 0-based offsets for that slice but still attached them to each column’s full, unsliced.valuesbuffer, which duplicated earlier rows and dropped the real ones with no error.The change slices every column’s
.valuesto[offsets[0]:offsets[-1]]beforefrom_arrays, matching what was already done forcolumns[0]when computingnum_elements.bfd/bfd_splitare unchanged.Adds
test_with_multiple_map_batchesto lock in correct packing across multiple map batches.Reviewed by Cursor Bugbot for commit ff265d8. Bugbot is set up for automated code reviews on this repo. Configure here.