[feat][SFT] Ingest pretokenized datasets - #1927
Conversation
… (S3/GCS) Adds a pretokenized data path to the SFT trainer so offline-tokenized datasets can be trained on directly, skipping apply_chat_template: - New skyrl/train/dataset/pretokenized.py: loads parquet/JSONL/arrow files or HF save_to_disk directories from a local path, s3://, gs://, or gcs:// URI (downloads via the shared fsspec io layer with S3 retry/credential-refresh handling; cached locally under cache_dir). - Rows carry input_ids plus a loss target in one of three schemas (native num_actions [+ window loss_mask], full-sequence loss_mask, or HF-style labels with -100) and are normalized to the trainer's internal format; max_length truncation matches the online path. - New SFTConfig fields pretokenized_dataset_path and eval_pretokenized_dataset_path, taking precedence over dataset_name / eval_dataset_name; eval validation accepts a pretokenized eval store. - CPU tests for format detection, schema normalization, truncation, cloud download/caching, trainer routing, and config validation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
…add VLM ingestion Reworks the pretokenized ingestion input schema per design review: - Rows now carry exactly input_ids + a full-sequence 0/1 loss_mask; num_actions is inferred from the first nonzero mask entry (window-form masks, num_actions columns, and HF-style labels are rejected with errors explaining how to convert offline). attention_mask remains optional and must be all-ones; padding stays collation-internal. - VLM rows (pixel_values + image_grid_thw) pass through to the collator's TensorList path. Over-length VLM rows are dropped rather than truncated (mirrors the online VLM path); mixed text+VLM stores drop the null image columns parquet materializes on text rows. - Rename load_pretokenized_dataset -> load_from_pretokenized (and the trainer method to _load_from_pretokenized) to mirror _load_and_tokenize, which returns the same list[dict] shape. - Update tests (33 passing), README schema docs, and config docstrings. Also reword docs to reference the tokenize functions being skipped instead of apply_chat_template. Text path validated e2e on GPU from an S3 parquet store (wandb run sft-pretokenized-s3-e2e-fullseq-mask); VLM path covered at the unit/collation level only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
…ovaSky-AI#1883) Resolves conflicts with the multi-dataset PR and extends pretokenized ingestion to match its list-based workflow: - pretokenized_dataset_path -> pretokenized_dataset_paths (List[str]): multiple stores are concatenated and mixed per train_dataset_weights via DataMixingSampler, exactly like train_datasets. Exclusive with train_datasets (explicit error instead of silent precedence). - eval_pretokenized_dataset_path -> eval_pretokenized_dataset_paths: each store becomes one eval set logged under eval/{name}/, named by eval_dataset_names (default: path basenames, collision -> error). - Config normalization validates weights/names against the pretokenized lists; eval_interval/eval_before_train accept pretokenized eval. - load_dataset() returns (tokenized, dataset_lengths) per the new API; load_eval_datasets() emits (name, rows) pairs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
…ct check Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
There was a problem hiding this comment.
Code Review
This pull request introduces support for pretokenized datasets (local or S3/GCS) in SFT training, allowing users to bypass online tokenization by pointing directly to pretokenized stores in Parquet, JSONL, Arrow, or HuggingFace formats. The changes include a new dataset loading module, updated configuration options in SFTConfig, integration in SFTTrainer, and comprehensive unit tests. The review feedback highlights two important improvements: addressing a potential race condition in multi-node environments during concurrent dataset downloads by using unique temporary directories, and skipping hidden files and directories during dataset file collection to prevent loading errors.
SumanthRH
left a comment
There was a problem hiding this comment.
Overall comments:
- Adding support for cloud paths can be a separate PR - we can actually support this for text datasets and pretokenized datasets in that PR.
- The implementation for handling pretokenized dataset seems good! There are can be more tests for handling different formats.
| # always re-inferred from ``loss_mask``, and HF-style ``labels`` are not a | ||
| # supported loss target (convert to a 0/1 ``loss_mask`` offline). All other | ||
| # columns pass through untouched (e.g. ``pixel_values`` / ``image_grid_thw``). | ||
| _CONSUMED_KEYS = frozenset({"input_ids", "attention_mask", "loss_mask", "num_actions", "labels"}) |
There was a problem hiding this comment.
Nit: Why should we have special handling for a labels field here?
…llow-up Per review: cloud-path (S3/GCS) ingestion will land in a follow-up PR covering both pretokenized stores and text-format datasets, so this PR supports local files/directories only. Removes the io-backed download/ cache machinery from skyrl/train/dataset/pretokenized.py (and the cache_dir/force_redownload knobs from load_from_pretokenized), plus the S3 tests and doc references. Also per review: over-length VLM rows now log a per-row drop warning, matching the online VLM tokenization path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
Config normalization (validate_sft_cfg) fills the equal-mixing default for the random sampler on every shipped construction path, so the trainer-side fallback never runs. Pass cfg.train_dataset_weights directly; DataMixingSampler's own validation covers misuse. Dataloader tests that exercised the fallback now pass explicit weights. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
Training stores are concatenated into one dataset (mixed per
train_dataset_weights) so they never need per-store names; only eval
stores do, for the eval/{name}/ metric namespace. Documented in the
docstring.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
…lumn The column's values are never used (the row-level mask is regenerated as all-ones; padding happens at collation), so surface that to the user. Logged once per process rather than per store/row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
Per review: the trailing action-window conversion (num_actions + window mask) is an RL legacy; a position-aligned full-sequence mask is the more natural worker interface for SFT and would remove this adapter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
Per review: no circular import here (unlike the collators import in this file, which stays lazy), so the guard was unnecessary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
The wrapper only bound max_length after the cloud-path split removed the caching knobs; call load_from_pretokenized directly at the two call sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
Completes the format matrix per review: single + multi file for each of parquet/jsonl/arrow, plus save_to_disk, mixed-format, and empty/missing path cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
Per review: focus the schema docs on the two required fields with concrete single-turn and multi-turn examples. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
.ipynb_checkpoints/ can hold stale shard copies that silently duplicate training rows, and macOS ._* AppleDouble sidecars are not valid data files and crash the load. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
Adds a "Pretokenized datasets" section to the SFT overview covering pretokenized_dataset_paths / eval_pretokenized_dataset_paths: supported formats, the input_ids/loss_mask row schema (incl. VLM tensors), max_length truncation, exclusivity with train_datasets, and the local-paths-only / matching-chat-template caveats. Follows PR NovaSky-AI#1927. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for loading pretokenized SFT datasets offline, skipping online tokenization entirely. It adds new configuration options (pretokenized_dataset_paths and eval_pretokenized_dataset_paths), a new pretokenized.py dataset module to handle loading and normalizing various on-disk formats (Parquet, JSONL, Arrow, HF Dataset), and integrates this loading mechanism into the SFTTrainer. Comprehensive tests are also added. The review feedback highlights two key improvements: first, validating that loss_mask values are strictly numeric 0 or 1 before converting to integers to prevent silent truncation of fractional values; second, changing the individual over-length VLM row warning to a debug log to avoid flooding the console during dataset loading.
…atasets Follow-up to NovaSky-AI#1927 (merged): factor cloud materialization into skyrl/train/dataset/remote_storage.py and use it from both SFT data paths: - pretokenized stores: pretokenized_dataset_paths / eval_pretokenized_dataset_paths accept s3:// gs:// gcs:// URIs (context-managed download; rows are materialized in memory before an uncached temp download is cleaned up). - text-format datasets: train_datasets / eval_datasets entries may be cloud URIs; they are resolved to a persistent local copy before load_dataset (persistent because parallel tokenization workers re-open the dataset by path). The original URI stays the tokenization cache key so caches are stable across nodes. Downloads cache under cache_dir keyed by remote path with atomic tmp-then-rename (NFS-safe); disable_cache / force_recache honored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Avi Basnet <avigyabb@stanford.edu>
What
Adds a pretokenized data path to the SFT trainer: point
pretokenized_dataset_pathsat local store(s) of already-tokenized rows and train directly, skipping online tokenization (tokenize_chat_example/tokenize_sft_example) entirely. Useful when a data pipeline tokenizes offline (e.g. on a Spark/Ray data cluster).How it works
New module
skyrl/train/dataset/pretokenized.py—load_from_pretokenized(path, max_length), the pretokenized counterpart ofSFTTrainer._load_and_tokenize(samelist[dict]return shape, so everything downstream — collators, sequence packing, samplers, checkpoint/resume — works unchanged):Dataset.save_to_diskdirectory.input_idsplus a full-sequence 0/1loss_mask(same length).num_actionsis inferred from the first nonzero mask entry; window-form masks,num_actionscolumns, and HF-stylelabelsare rejected with clear errors.attention_maskis optional and must be all-ones (padding stays collation-internal). The mask form covers instruction-following (1s on the response) and multi-turn conversational data (1s on every assistant turn).pixel_values+image_grid_thwpass through to the collator'sTensorListpath. Over-length VLM rows are dropped with a warning rather than truncated (consistent with the online VLM path); mixed text+VLM stores handle the null image columns parquet materializes on text rows.max_lengthtruncation mirrors the online path (prompt prefix kept, action window shrinks, empty-loss rows dropped).Config (
SFTConfig) — integrated with multi-dataset SFT (#1883):pretokenized_dataset_paths: List[str]— multiple stores are concatenated and mixed pertrain_dataset_weightsviaDataMixingSampler, exactly liketrain_datasets. Exclusive withtrain_datasets/train_dataset_splits(explicit error, no silent precedence): a pretokenized store is the output of a preprocessing job, so the user is assumed to have pre-split/filtered it — HF split syntax does not apply.eval_pretokenized_dataset_paths: List[str]— each store becomes one eval set logged undereval/{name}/, named byeval_dataset_names(default: path basenames; collisions error).eval_interval/eval_before_trainaccept pretokenized eval stores.Testing
tests/train/test_sft_pretokenized.pycovering format detection, schema validation/normalization, truncation, VLM pass-through + collation, multi-store concatenation + mixing-sampler wiring, and config validation. Full SFT suite passes alongside.Notes / limitations
🤖 Generated with Claude Code