Skip to content

[feat][SFT] Serve the tokenized-dataset cache memory-mapped - #1970

Open
avigyabb wants to merge 2 commits into
NovaSky-AI:mainfrom
avigyabb:sft-text-mmap
Open

[feat][SFT] Serve the tokenized-dataset cache memory-mapped#1970
avigyabb wants to merge 2 commits into
NovaSky-AI:mainfrom
avigyabb:sft-text-mmap

Conversation

@avigyabb

@avigyabb avigyabb commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

What

The tokenize-on-load path (train_datasets) no longer materializes the tokenized dataset in memory for training. Follow-up to #1961 flagged in review: "TextDataset currently materializes the entire dataset in memory, but it doesn't have to be."

How

One observation makes this small: the tokenized-dataset cache is already an arrow-backed HF Dataset on disk in the trainer's internal row form -- it is, in effect, a pretokenized store. But _load_from_cache immediately materialized it back into a list[dict] via .to_list() (O(dataset) RAM, re-pickled into every spawn dataloader worker). This PR serves it through the same map-style mmap wrapper the pretokenized path uses, with no transform attached (cached rows are already normalized):

  • _load_from_cache returns MemoryMappedDataset(Dataset.load_from_disk(...), lengths) -- memory-mapped, rows page in per accessed batch through the existing dataloader prefetch. Per-row lengths come from arrow offsets (sequence_lengths_from_arrow, chunked scan, nothing materialized).
  • Fresh tokenization round-trips through the cache in both the sequential and parallel paths (_save_to_cache -> reload mmap'd), so cold runs also train from the mmap. Tokenization cost is unchanged -- it was always one-time; only the residency of the results changes.
  • disable_cache=True keeps today's behavior (in-memory list wrapped in TextDataset): with no arrow file on disk there is nothing to map.

This also resolves the old TODO at the cache-save site ("migrate to HF datasets + dataloader so we don't materialize the full dataset in memory").

Rename: PretokenizedDataset -> MemoryMappedDataset

Since the class now serves two roles (pretokenized stores and the tokenized cache), the old name was wrong for one of them: it is a map-style view over any validated arrow store in (or normalizable to) the trainer's internal row form. Renamed now, before anything external depends on the name (#1961 merged it one release ago). The module keeps the pretokenized.py name for diff locality; moving the class to sft_dataset.py could be a later cleanup.

Known limitation

Cache-miss tokenization still materializes the full list[dict] transiently (tokenize -> save -> reload mmap'd); the list dies at function exit, so this bounds first-run peak memory, not steady state. Closing the transient too would mean chunked tokenize-and-append arrow writing -- a possible follow-up.

Effect

With caching enabled (the default), the text path now has the same memory profile as pretokenized stores: resident memory is the OS page cache (reclaimable) instead of the dataset, and dataloader workers pickle a file reference instead of receiving a full copy -- the old peak was (dataloader_num_workers + 1) x the dataset.

Tests

  • New: cache-backed _load_and_tokenize returns a memory-mapped SFTDataset with arrow-derived lengths, row-parity against the materialized (disable_cache=True) path.
  • Existing cache round-trip / parallel-vs-serial parity tests pass unchanged (they compare rows by iteration, which the map-style dataset satisfies).
  • Full tests/train suite green: 884 passed.

🤖 Generated with Claude Code

The tokenized cache is already an arrow-backed HF Dataset on disk in the
trainer's internal row form, but _load_from_cache materialized it back
into a list[dict] (O(dataset) RAM, re-pickled into every spawn dataloader
worker). Serve it through the same map-style mmap wrapper as pretokenized
stores instead, with no transform attached (cached rows are already
normalized):

- _load_from_cache returns PretokenizedDataset(load_from_disk(...),
  lengths), with per-row lengths from arrow offsets via the new
  sequence_lengths_from_arrow helper (chunked, no row materialization).
- Fresh tokenization round-trips through the cache in both the
  sequential and parallel paths, so cold runs also train memory-mapped.
- disable_cache=True keeps the in-memory list (wrapped in TextDataset);
  with no arrow file on disk there is nothing to map.

Tokenization cost is unchanged; only the residency of the results
changes: the text path now has the same memory profile as pretokenized
stores (O(page cache), workers pickle a file reference).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
@avigyabb
avigyabb marked this pull request as ready for review August 3, 2026 04:03

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request optimizes memory usage during dataset loading and tokenization by serving cached tokenized datasets as memory-mapped PretokenizedDataset instances instead of fully materializing them in memory as lists. This is achieved by extracting sequence lengths directly from arrow offsets and returning the memory-mapped dataset when caching is enabled. A new test has been added to verify this behavior. I have no feedback to provide.

@avigyabb
avigyabb marked this pull request as draft August 3, 2026 04:06
Since the tokenized-dataset cache is now served through the same class,
the old name was wrong for one of its two roles: the class is a
map-style view over any validated arrow store in (or normalizable to)
the trainer's internal row form. Rename before anything external
depends on the name (NovaSky-AI#1961 merged it one release ago).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Avi Basnet <avigyabb@stanford.edu>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the tokenization caching mechanism to serve cached datasets as memory-mapped MemoryMappedDataset (formerly PretokenizedDataset) instances instead of fully materializing them in memory as lists of dicts, significantly reducing memory overhead. Feedback suggests optimizing the sequence length extraction in sequence_lengths_from_arrow by specifying only the "input_ids" column when calling with_format on the dataset to avoid loading unnecessary columns.

tokenized-dataset cache), lengths are the only load-time scan needed --
validation and truncation already happened when the rows were produced.
"""
arrow_ds = dataset.with_format("arrow")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To optimize performance and reduce memory overhead, specify columns=["input_ids"] when calling with_format. This prevents Hugging Face Dataset from loading, formatting, or copying other columns (such as large VLM image tensors like pixel_values) when we only need to scan the sequence lengths of input_ids.

Suggested change
arrow_ds = dataset.with_format("arrow")
arrow_ds = dataset.with_format("arrow", columns=["input_ids"])

@avigyabb
avigyabb requested a review from SumanthRH August 3, 2026 19:27
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