diff --git a/src/soup_cli/recipes/catalog.py b/src/soup_cli/recipes/catalog.py index b63f89f..59c0dca 100644 --- a/src/soup_cli/recipes/catalog.py +++ b/src/soup_cli/recipes/catalog.py @@ -2284,10 +2284,12 @@ output: ./output size="256M", tags=("smolvlm", "vision", "multimodal", "vlm", "sft", "tiny", "edge"), description="SmolVLM 256M vision SFT (llava format) — a tiny VLM. NOTE: " - "SmolVLM uses an Idefics3 processor; live vision SFT needs Idefics3 " - "vision-path support (the LLaVA path assumes a different processor " - "API) — tracked as a follow-up. target_modules pinned to q_proj/v_proj " - "(auto cannot infer them for Idefics3).", + "SmolVLM uses an Idefics3 processor. The processor pad_token blocker is " + "fixed (#302 — the nested tokenizer's token surface is mirrored onto the " + "processor), so setup + tokenization now run; a full training STEP still " + "needs Idefics3-aware vision collation (pixel_values + image-token " + "expansion) — parse-tested for now, tracked in #302. target_modules " + "pinned to q_proj/v_proj (auto cannot infer them for Idefics3).", yaml_str="""\ base: HuggingFaceTB/SmolVLM-256M-Instruct task: sft diff --git a/src/soup_cli/trainer/sft.py b/src/soup_cli/trainer/sft.py index 7a486c1..137b0c9 100644 --- a/src/soup_cli/trainer/sft.py +++ b/src/soup_cli/trainer/sft.py @@ -16,6 +16,64 @@ logger = logging.getLogger(__name__) console = Console() +# Text-token surface TRL's SFTTrainer reads directly off ``processing_class`` +# (trl/trainer/sft_trainer.py: ``pad_token`` / ``eos_token`` / ``eos_token_id`` +# resolution) — mirrored from a vision processor's nested tokenizer in #302. +_PROCESSOR_TOKEN_ATTRS = ( + "pad_token", + "eos_token", + "pad_token_id", + "eos_token_id", + "bos_token", + "bos_token_id", +) + + +def _ensure_vision_processor_pad_token(processor: object) -> None: + """Mirror a vision processor's nested-tokenizer token surface onto itself. + + HF vision processors (Idefics3/SmolVLM, LLaVA, Qwen2-VL, ...) keep the text + tokenizer nested at ``processor.tokenizer`` and do NOT forward token-level + attributes — ``ProcessorMixin`` has no ``__getattr__``. TRL's ``SFTTrainer`` + reads ``processing_class.pad_token`` / ``.eos_token`` / + ``.convert_tokens_to_ids`` directly, so passing such a processor as + ``processing_class`` crashes with e.g. ``'Idefics3Processor' object has no + attribute 'pad_token'`` (#302). + + Fix: when the processor exposes a nested ``.tokenizer``, set + ``pad_token = eos_token`` on that tokenizer if unset, then copy the token + surface + ``convert_tokens_to_ids`` onto the processor — but only for + attributes it does not already expose, so a processor that already behaves + like a tokenizer (or a plain tokenizer) is left untouched (no LLaVA-path + regression). Best-effort per attribute: a read-only property on either side + is skipped rather than fatal. + """ + tok = getattr(processor, "tokenizer", None) + if tok is None: + # Already tokenizer-like, or an unknown shape — nothing to mirror. + return + # A padless tokenizer trains fine once pad == eos (the standard causal-LM + # convention already used by the text path, sft.py:_setup_transformers). + if getattr(tok, "pad_token", None) is None and getattr(tok, "eos_token", None) is not None: + try: + tok.pad_token = tok.eos_token + except (AttributeError, TypeError): + pass + for attr in _PROCESSOR_TOKEN_ATTRS: + if hasattr(processor, attr): + continue # processor already exposes it — don't clobber + try: + setattr(processor, attr, getattr(tok, attr, None)) + except (AttributeError, TypeError): + pass + if not hasattr(processor, "convert_tokens_to_ids"): + inner = getattr(tok, "convert_tokens_to_ids", None) + if callable(inner): + try: + processor.convert_tokens_to_ids = inner + except (AttributeError, TypeError): + pass + def _maybe_load_pretokenized( dcfg, base: str, console_obj: Console, @@ -929,6 +987,10 @@ class SFTTrainerWrapper: self.processor = AutoProcessor.from_pretrained( cfg.base, trust_remote_code=self._trust_remote_code ) + # Idefics3/SmolVLM (and other) processors keep the text tokenizer nested + # and don't forward pad_token/eos_token — mirror them onto the processor + # so TRL's SFTTrainer processing_class access doesn't crash (#302). + _ensure_vision_processor_pad_token(self.processor) self.tokenizer = self.processor # SFTTrainer uses processing_class # Quantization (v0.71.19 #81) — unified Quant Menu loader. Replaces the diff --git a/tests/test_issue302_vision_pad_token.py b/tests/test_issue302_vision_pad_token.py new file mode 100644 index 0000000..49734a2 --- /dev/null +++ b/tests/test_issue302_vision_pad_token.py @@ -0,0 +1,214 @@ +"""Issue #302 — Idefics3 / SmolVLM vision-SFT pad_token routing. + +SmolVLM uses an ``Idefics3Processor``. The shared LLaVA vision path sets +``self.tokenizer = `` and hands it to TRL's ``SFTTrainer`` as +``processing_class``. TRL reads ``processing_class.pad_token`` / +``.eos_token`` / ``.convert_tokens_to_ids`` directly, but HF vision processors +keep the text tokenizer nested at ``processor.tokenizer`` and do NOT forward +token-level attributes (``ProcessorMixin`` has no ``__getattr__``) — so training +crashes with ``AttributeError: 'Idefics3Processor' object has no attribute +'pad_token'``. + +This suite pins ``_ensure_vision_processor_pad_token`` — it mirrors the inner +tokenizer's text-token surface onto the processor (setting pad_token = eos_token +when unset), reproducing TRL's exact ``args.pad_token or processing_class.pad_token +or processing_class.eos_token`` access. Both Idefics3 and LLaVA processors share +identical structure (``attributes = ['image_processor', 'tokenizer']``), so the +fix repairs both without regressing a processor that already exposes pad_token. +""" + +from __future__ import annotations + +import pytest + + +class _FakeTokenizer: + """Text tokenizer with the token surface TRL reads off processing_class.""" + + def __init__(self, pad_token=None, eos_token=""): + self.pad_token = pad_token + self.eos_token = eos_token + self.eos_token_id = 2 + self.bos_token = "" + self.bos_token_id = 1 + + @property + def pad_token_id(self): + # Mirrors a real tokenizer: None until pad_token is set. + return 0 if self.pad_token is not None else None + + def convert_tokens_to_ids(self, token): + return {"": 2, "": 1, "": 0}.get(token, 2) + + +class _FakeIdefics3Processor: + """Mimics Idefics3Processor: nested .tokenizer, NO pad_token forwarding.""" + + attributes = ["image_processor", "tokenizer"] + + def __init__(self, tokenizer): + self.tokenizer = tokenizer + self.image_processor = object() + + # No __getattr__ — accessing .pad_token raises AttributeError, exactly like + # the real ProcessorMixin subclass. + + +class _TokenizerLikeProcessor: + """A processing_class that already exposes the token surface (regression guard).""" + + def __init__(self): + self.pad_token = "" + self.eos_token = "" + self.tokenizer = None + + def convert_tokens_to_ids(self, token): + return 0 + + +def _trl_pad_token(processing_class, args_pad_token=None): + """Reproduce TRL SFTTrainer's pad-token resolution (sft_trainer.py:436).""" + return args_pad_token or processing_class.pad_token or processing_class.eos_token + + +class TestEnsureVisionProcessorPadToken: + def test_bare_processor_raises_before_fix(self): + # Sanity: the un-fixed processor reproduces the reported AttributeError. + proc = _FakeIdefics3Processor(_FakeTokenizer(pad_token=None)) + with pytest.raises(AttributeError): + _ = proc.pad_token + + def test_sets_pad_token_from_eos(self): + from soup_cli.trainer.sft import _ensure_vision_processor_pad_token + + proc = _FakeIdefics3Processor(_FakeTokenizer(pad_token=None, eos_token="")) + _ensure_vision_processor_pad_token(proc) + # Inner tokenizer got pad_token = eos_token + assert proc.tokenizer.pad_token == "" + # Processor now exposes the surface TRL reads + assert proc.pad_token == "" + assert proc.eos_token == "" + + def test_trl_resolution_no_longer_crashes(self): + from soup_cli.trainer.sft import _ensure_vision_processor_pad_token + + proc = _FakeIdefics3Processor(_FakeTokenizer(pad_token=None)) + _ensure_vision_processor_pad_token(proc) + pad = _trl_pad_token(proc) + assert pad == "" + # convert_tokens_to_ids delegates to the inner tokenizer + assert proc.convert_tokens_to_ids(pad) == 2 + + def test_preserves_existing_pad_token(self): + from soup_cli.trainer.sft import _ensure_vision_processor_pad_token + + tok = _FakeTokenizer(pad_token="", eos_token="") + proc = _FakeIdefics3Processor(tok) + _ensure_vision_processor_pad_token(proc) + assert proc.tokenizer.pad_token == "" # untouched + assert proc.pad_token == "" + + def test_tokenizer_like_processor_unchanged(self): + # A processing_class that already exposes pad_token must not be clobbered. + from soup_cli.trainer.sft import _ensure_vision_processor_pad_token + + proc = _TokenizerLikeProcessor() + _ensure_vision_processor_pad_token(proc) + assert proc.pad_token == "" + + def test_no_nested_tokenizer_is_noop(self): + from soup_cli.trainer.sft import _ensure_vision_processor_pad_token + + class _NoTok: + pad_token = "" + eos_token = "" + + proc = _NoTok() + _ensure_vision_processor_pad_token(proc) # must not raise + assert proc.pad_token == "" + + def test_convert_tokens_to_ids_mirrored(self): + from soup_cli.trainer.sft import _ensure_vision_processor_pad_token + + proc = _FakeIdefics3Processor(_FakeTokenizer(pad_token=None)) + _ensure_vision_processor_pad_token(proc) + assert callable(proc.convert_tokens_to_ids) + assert proc.convert_tokens_to_ids("") == 2 + + def test_eos_token_id_mirrored(self): + from soup_cli.trainer.sft import _ensure_vision_processor_pad_token + + proc = _FakeIdefics3Processor(_FakeTokenizer(pad_token=None)) + _ensure_vision_processor_pad_token(proc) + assert proc.eos_token_id == 2 + + def test_pad_token_id_mirrored(self): + from soup_cli.trainer.sft import _ensure_vision_processor_pad_token + + proc = _FakeIdefics3Processor(_FakeTokenizer(pad_token=None)) + _ensure_vision_processor_pad_token(proc) + # inner tokenizer's pad_token_id property becomes 0 once pad is set + assert proc.pad_token_id == 0 + + def test_readonly_attr_degrades_gracefully(self): + # A processor whose attributes can't be set (e.g. __slots__) must not + # make the helper raise — the try/except degrades gracefully. + from soup_cli.trainer.sft import _ensure_vision_processor_pad_token + + class _SlotsProcessor: + __slots__ = ("tokenizer",) + + def __init__(self, tok): + self.tokenizer = tok + + proc = _SlotsProcessor(_FakeTokenizer(pad_token=None)) + # Must not raise even though setattr(proc, "pad_token", ...) fails. + _ensure_vision_processor_pad_token(proc) + # Inner tokenizer was still repaired (pad = eos). + assert proc.tokenizer.pad_token == "" + + +class TestVisionSetupWiring: + def test_setup_vision_transformers_invokes_pad_token_mirror(self, monkeypatch): + # The fix is only useful if _setup_vision_transformers actually calls it. + # Mock the heavy loads; assert the processor gets pad_token mirrored. + from unittest.mock import MagicMock + + import transformers + + from soup_cli.config.loader import load_config_from_string + from soup_cli.trainer.sft import SFTTrainerWrapper + + fake_proc = _FakeIdefics3Processor(_FakeTokenizer(pad_token=None)) + monkeypatch.setattr( + transformers.AutoProcessor, "from_pretrained", + lambda *a, **k: fake_proc, + ) + monkeypatch.setattr( + transformers.AutoModelForVision2Seq, "from_pretrained", + lambda *a, **k: MagicMock(), + ) + import peft + + monkeypatch.setattr(peft, "get_peft_model", lambda model, cfg: model) + monkeypatch.setattr( + "soup_cli.utils.quant_menu.build_quantization_config_for_loader", + lambda **k: None, + ) + monkeypatch.setattr( + "soup_cli.utils.data_pipeline.apply_vocab_expansion", + lambda *a, **k: None, + ) + monkeypatch.setattr( + SFTTrainerWrapper, "_apply_quantization_aware", lambda self, tcfg: None + ) + + cfg = load_config_from_string( + "base: fake/vlm\ntask: sft\nmodality: vision\n" + "data:\n train: x.jsonl\n format: llava\n max_length: 64\n" + "training:\n quantization: none\n lora:\n target_modules: [q_proj, v_proj]\n" + ) + wrapper = SFTTrainerWrapper(cfg, device="cpu") + wrapper._setup_vision_transformers(cfg, cfg.training) + # The helper ran: the Idefics3-style processor now exposes pad_token. + assert wrapper.processor.pad_token == ""