Fix dataset fingerprinting in experimental KTO and TPO trainers#6233
Closed
DaoyuanLi2816 wants to merge 1 commit into
Closed
Fix dataset fingerprinting in experimental KTO and TPO trainers#6233DaoyuanLi2816 wants to merge 1 commit into
DaoyuanLi2816 wants to merge 1 commit into
Conversation
Follow-up to huggingface#6206, which fixed the same issue in DPO/SFT/Reward. `KTOTrainer` and `TPOTrainer` tokenize their datasets with a `map` function that closes over `self` (it calls `self._tokenize(...)`). A closure over `self` — which holds the model and other unpicklable state — can't be hashed stably, so `datasets` falls back to a random fingerprint and silently disables caching of the tokenized dataset. Mirror huggingface#6206: make `_tokenize` a `@staticmethod` and bind it to a local before defining the map function, so the closure no longer captures `self`. KTO passes the VLM flag through as an explicit `is_vlm` parameter; TPO's `_tokenize` does not use `self`, so it simply becomes a staticmethod. The tokenized output is unchanged.
Member
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.
Follow-up to #6206, which fixed dataset fingerprinting in the core
DPOTrainer/SFTTrainer/RewardTrainer. The two experimental trainersKTOTrainerandTPOTrainerstill have the pattern #6206 removed: their tokenization map is a closure that callsself._tokenize(...), so it capturesself. A map function that closes overself(which holds the model and other unpicklable state) can't be hashed stably, sodatasetsfalls back to a random fingerprint and silently disables caching of the tokenized dataset — re-tokenizing on every run.This applies the same fix as #6206: make
_tokenizea@staticmethodand bind it to a local (tokenize = self._tokenize) before defining the map function, so the closure no longer capturesself._tokenizeuses the VLM flag, sois_vlmbecomes an explicit parameter passed throughfn_kwargs(exactly as in Fix dataset fingerprinting in DPO/SFT tokenization #6206's DPO)._tokenizedoesn't referenceselfat all, so it simply becomes a@staticmethod.No behavior change: the tokenized output is identical; only the map function's hashability (and therefore cache reuse across runs) changes.
CPOTrainerandORPOTrainershare the same pattern (aself-boundtokenize_row) but are intentionally left out here to avoid conflicts: CPO's data pipeline is being refactored in #5837, and ORPO's_prepare_datasetis touched by #6230. They can be aligned once those land.Verification
tests/experimental/test_kto_trainer.pyandtests/experimental/test_tpo_trainer.pypass in full (tokenized output unchanged);ruff check/ruff format --checkclean. Validated on a single GPU (RTX 4080).datasetscan't hash, which the tiny CI fixtures don't exercise. The existing suites cover tokenization correctness.Note
Low Risk
Refactor limited to dataset preprocessing closures with no training-loss or tokenization logic changes; behavior is intended to be identical aside from caching.
Overview
Aligns experimental
KTOTrainerandTPOTrainerwith the dataset-fingerprint fix from #6206 sodataset.mapduring preprocessing can be cached across runs instead of re-tokenizing every time.In both trainers,
_tokenizeis now a@staticmethod, andtokenize_fncalls a localtokenize = self._tokenizeso the map callback no longer closes overself(which held the model and broke stable hashing). KTO also threadsis_vlmas an explicit argument viafn_kwargs, matching DPO’s VLM handling.Tokenized outputs are unchanged; only cache fingerprint stability improves.
Reviewed by Cursor Bugbot for commit bf7b8e4. Bugbot is set up for automated code reviews on this repo. Configure here.