Skip to content

Fix LoRA reload path in Gemma4 and Qwen3.5 GRPO notebooks - #291

Open
danielhanchen wants to merge 2 commits into
mainfrom
fix-grpo-lora-reload-path
Open

danielhanchen wants to merge 2 commits into
mainfrom
fix-grpo-lora-reload-path

Conversation

@danielhanchen

Copy link
Copy Markdown
Member

Four GRPO notebooks save the trained LoRA adapter to one directory but try to reload it from another, so the post-training reload/inference cell always fails with FileNotFoundError on adapter_model.safetensors.

Notebook saves to reload cell reads fix
Gemma4_(E2B)_GRPO gemma_4_lora grpo_saved_lora read gemma_4_lora
Gemma4_(E2B)_Reinforcement_Learning_Sudoku_Game gemma_4_lora grpo_saved_lora read gemma_4_lora
Gemma4_(E2B)_Reinforcement_Learning_2048_Game gemma_4_lora grpo_saved_lora read gemma_4_lora
Qwen3_5_(4B)_Vision_GRPO qwen_lora grpo_lora read qwen_lora

grpo_saved_lora / grpo_lora are stale paths carried over from the canonical GRPO template (which uses model.save_lora("grpo_saved_lora")); these notebooks switched the save to model.save_pretrained("gemma_4_lora") but left the reload path unchanged, so the reload directory never exists.

Fix aligns the reload path to the actual save directory (one string per notebook). Verified by running Gemma4_(E2B)_GRPO end to end inside the Docker validation image: training completes and the reload cell now finds the saved adapter (the cell previously raised FileNotFoundError).

These notebooks live only under nb/ (no original_template), so the edit is to nb/ directly.

transformers 5.x removed the deprecated tokenizer argument from
Seq2SeqTrainer (4.x already warned: use processing_class instead), so
the notebook dies at trainer construction on current installs.
processing_class accepts the feature extractor on 4.57.6 and 5.x alike.

Applied in original_template and synced to the generated nb, kaggle and
python_scripts copies; a full regeneration was avoided on purpose since
it rewrites unrelated notebooks.
These notebooks save the trained adapter with model.save_pretrained("gemma_4_lora")
(or "qwen_lora") but the inference cell loads from "grpo_saved_lora" (or
"grpo_lora"), a stale path from the canonical GRPO template that is never
created. The reload cell always failed with FileNotFoundError on
adapter_model.safetensors. Align the load path to the save dir.

Verified: Gemma4_(E2B)_GRPO runs end to end after the fix (the reload cell
now finds the saved adapter).

@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 updates model paths and configurations across several notebooks, and replaces the deprecated tokenizer parameter with processing_class in Whisper-related training scripts. The feedback recommends passing the full processor object (tokenizer) directly to processing_class instead of just its feature extractor, ensuring both the feature extractor and tokenizer are properly available to the trainer.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread python_scripts/Whisper.py
data_collator = DataCollatorSpeechSeq2SeqWithPadding(processor = tokenizer),
eval_dataset = test_dataset,
tokenizer = tokenizer.feature_extractor,
processing_class = tokenizer.feature_extractor,

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

Since tokenizer is actually a WhisperProcessor (as indicated by its usage in DataCollatorSpeechSeq2SeqWithPadding(processor = tokenizer)), you can pass the processor itself directly to processing_class instead of just its feature extractor. This is the recommended approach in modern Transformers, and it ensures that both the feature extractor and the tokenizer are available to the trainer (e.g., for saving the processor or if predict_with_generate is enabled).

Suggested change
processing_class = tokenizer.feature_extractor,
processing_class = tokenizer,

data_collator = DataCollatorSpeechSeq2SeqWithPadding(processor = tokenizer),
eval_dataset = test_dataset,
tokenizer = tokenizer.feature_extractor,
processing_class = tokenizer.feature_extractor,

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

Since tokenizer is actually a WhisperProcessor (as indicated by its usage in DataCollatorSpeechSeq2SeqWithPadding(processor = tokenizer)), you can pass the processor itself directly to processing_class instead of just its feature extractor. This is the recommended approach in modern Transformers, and it ensures that both the feature extractor and the tokenizer are available to the trainer (e.g., for saving the processor or if predict_with_generate is enabled).

Suggested change
processing_class = tokenizer.feature_extractor,
processing_class = tokenizer,

Comment thread nb/Whisper.ipynb
" data_collator = DataCollatorSpeechSeq2SeqWithPadding(processor = tokenizer),\n",
" eval_dataset = test_dataset,\n",
" tokenizer = tokenizer.feature_extractor,\n",
" processing_class = tokenizer.feature_extractor,\n",

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

Since tokenizer is actually a WhisperProcessor (as indicated by its usage in DataCollatorSpeechSeq2SeqWithPadding(processor = tokenizer)), you can pass the processor itself directly to processing_class instead of just its feature extractor. This is the recommended approach in modern Transformers, and it ensures that both the feature extractor and the tokenizer are available to the trainer (e.g., for saving the processor or if predict_with_generate is enabled).

Suggested change
" processing_class = tokenizer.feature_extractor,\n",
" processing_class = tokenizer,\n",

Comment thread nb/Kaggle-Whisper.ipynb
" data_collator = DataCollatorSpeechSeq2SeqWithPadding(processor = tokenizer),\n",
" eval_dataset = test_dataset,\n",
" tokenizer = tokenizer.feature_extractor,\n",
" processing_class = tokenizer.feature_extractor,\n",

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

Since tokenizer is actually a WhisperProcessor (as indicated by its usage in DataCollatorSpeechSeq2SeqWithPadding(processor = tokenizer)), you can pass the processor itself directly to processing_class instead of just its feature extractor. This is the recommended approach in modern Transformers, and it ensures that both the feature extractor and the tokenizer are available to the trainer (e.g., for saving the processor or if predict_with_generate is enabled).

Suggested change
" processing_class = tokenizer.feature_extractor,\n",
" processing_class = tokenizer,\n",

" data_collator = DataCollatorSpeechSeq2SeqWithPadding(processor=tokenizer),\n",
" eval_dataset = test_dataset,\n",
" tokenizer = tokenizer.feature_extractor,\n",
" processing_class = tokenizer.feature_extractor,\n",

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

Since tokenizer is actually a WhisperProcessor (as indicated by its usage in DataCollatorSpeechSeq2SeqWithPadding(processor=tokenizer)), you can pass the processor itself directly to processing_class instead of just its feature extractor. This is the recommended approach in modern Transformers, and it ensures that both the feature extractor and the tokenizer are available to the trainer (e.g., for saving the processor or if predict_with_generate is enabled).

Suggested change
" processing_class = tokenizer.feature_extractor,\n",
" processing_class = tokenizer,\n",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant