Skip to content

Fix wrapped packing duplicating data when the input table is sliced - #6670

Merged
qgallouedec merged 2 commits into
huggingface:mainfrom
SuryanshSS1011:fix-pack-wrapped-sliced-table
Sep 17, 2026
Merged

qgallouedec merged 2 commits into
huggingface:mainfrom
SuryanshSS1011:fix-pack-wrapped-sliced-table

Conversation

@SuryanshSS1011

@SuryanshSS1011 SuryanshSS1011 commented Aug 6, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #6669

_pack_wrapped sizes 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 each
batch 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.

ds = Dataset.from_dict({"input_ids": [[1, 2], [3, 4], [5, 6], [7, 8], [9, 10], [11, 12]]})
pack_dataset(ds, seq_length=4, strategy="wrapped", map_kwargs={"batch_size": 3})["input_ids"]
# before: [[1, 2, 3, 4], [5, 6], [1, 2, 3, 4], [5, 6]]
# after:  [[1, 2, 3, 4], [5, 6], [7, 8, 9, 10], [11, 12]]

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 to column.flatten(), which respects the list array's own offset, per column so each keeps its own offsets.

Only wrapped is affected. bfd and bfd_split hit the same pattern but are protected by the pc.filter and pc.take that 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_list with int64 offsets.

Tests

Added TestPackDatasetWrapped::test_with_multiple_map_batches, which packs across two map batches. It fails on main with the duplication above and passes here. The two existing wrapped tests use 3-row datasets, so they only ever exercise a single batch, which is why this was not caught.

pytest tests/test_data_utils.py -k "Pack"
10 passed, 548 deselected

Beyond the added test, I checked _pack_wrapped against 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_length of 1 and
equal to and greater than the total, ragged and empty rows, zero-row tables, multi-chunk tables, 1 to 3 columns, list and large_list, int32 and int64 offsets. All pass with the fix and 127 of them fail without it. pack_dataset also round-trips end to end across batch sizes 1 to 777, num_proc 2 and 4, and all three strategies, with input_ids / attention_mask / labels staying aligned.

Since #5189 was a performance PR, I benchmarked this too. flatten() is metadata work like values, 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

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline, Pull Request section?
  • Was this discussed/approved via a GitHub issue? Please add a link to it if that's the case.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

AI writing disclosure

  • No AI usage: the PR was written entirely by a human.
  • AI-assisted: some parts were suggested or improved by AI, but the PR was written and reviewed by a human.
  • AI-generated: the PR was mostly or fully generated by an AI tool.

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_dataset runs through batched Dataset.map: batches after the first arrive as PyArrow table slices, and _pack_wrapped was sizing output from the slice but reading token data from column.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_split are unchanged in this diff.

Adds test_with_multiple_map_batches to pack with map_kwargs={"batch_size": 3} across two map batches and assert input_ids / attention_mask stay correct.

Reviewed by Cursor Bugbot for commit 34ad2ad. Bugbot is set up for automated code reviews on this repo. Configure here.

@qgallouedec qgallouedec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@bot-ci-comment

Copy link
Copy Markdown

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.

@qgallouedec
qgallouedec merged commit f78d649 into huggingface:main Sep 17, 2026
10 checks passed
@albertvillanova albertvillanova added the 🐛 bug Something isn't working label Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pack_dataset with strategy="wrapped" silently duplicates data across map batches

3 participants