Patch checkpoint reload init functions to strip unsupported args#5167
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a compatibility patch for PEFT weight converters to handle legacy constructor signatures by dynamically wrapping init methods. The review feedback suggests improving the robustness of the patching logic by adding proper exception handling and logging to prevent silent failures and ensure consistent state restoration.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c22e43a70
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def _build_peft_weight_mapping_compat( | ||
| weight_conversions, adapter_name, peft_config | ||
| ): |
There was a problem hiding this comment.
build_peft_weight_mapping in PEFT accepts peft_config=None, but the monkeypatched replacement makes peft_config mandatory. After this patch runs, any existing call path that relies on the original default (e.g. calling with only weight_conversions and adapter_name) will now raise TypeError, which is a runtime API regression introduced by this commit. Keep the default (and ideally forward *args/**kwargs) so the wrapper remains signature-compatible with the original function.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c07ae2e2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _build_peft_weight_mapping_compat( | ||
| weight_conversions, adapter_name, peft_config | ||
| ): |
There was a problem hiding this comment.
The monkeypatched _build_peft_weight_mapping_compat no longer matches upstream build_peft_weight_mapping call semantics: it requires peft_config instead of keeping the original default (None). After this patch is applied, any call site that previously invoked the function with only weight_conversions and adapter_name will raise TypeError at runtime, which is an API regression in checkpoint conversion paths.
Useful? React with 👍 / 👎.
Three small fixes to patch_peft_weight_converter_compatibility: - Restore peft_config=None default and @functools.wraps on the build_peft_weight_mapping wrapper so the upstream coordinated signature (weight_conversions, adapter_name, peft_config=None) is preserved. Without the default, callers using the documented two-argument form raise TypeError after import unsloth. - Serialize the temporary class-init patch/restore behind a threading.RLock. The previous unsynchronized window let two concurrent build_peft_weight_mapping calls (e.g. dynamic LoRA serving) re-expose the original distributed_operation TypeError when one thread restored a class while another was still inside original_build. - Hand _patch_weight_converter_ctors a caller-owned accumulator list and append in place. If signature inspection ever raises mid-loop, the finally block now sees the partial list and restores already-patched classes instead of leaving them with the compat init permanently installed.
for more information, see https://pre-commit.ci
danielhanchen
added
auto-approved
|
Auto-review verdict: Approved PR adds an import-time monkey patch around peft.utils.transformers_weight_conversion.build_peft_weight_mapping that strips distributed_operation/quantization_operation kwargs from legacy WeightConverter constructors so PEFT 0.19.x can rebuild adapter weight mappings on transformers 5.7.x where those kwargs are not yet accepted, fixing the TypeError that breaks Qwen3.6 (and other MoE) checkpoint resume reported in #5165. Reason: All three accepted findings fixed in f3ab842; subsequent two review rounds yielded no real issues; 9-test suite passes against fix and fails against pre-fix code |
Extend the cross-version compat canary to catch ~80% of upstream drift before a user hits it. Static checks only (GitHub raw fetch + grep), CPU-only, runs PR-time + daily cron. 906 pass, 73 skipped. TRL coverage extended: - TRL_TAGS expanded from 12 to 28 (every stable release >=0.18.2, including the broken 0.19.0, plus main). Anchors: 0.22.2 / 0.27.1 / 1.0.0 marked. - Fix `__version__` parser to handle the TRL 0.22.x pattern (`__version__ = f.read()` from sibling VERSION file). - Fix `has_def` in _fetch.py to allow indented matches so class methods are detected (the original anchored ^def only matched module-scope definitions). - New tests for symbols the audit found we touch but didn't check: is_conversational, sft_trainer module + neftune_post_forward_hook, dpo_trainer module + MODEL_FOR_VISION_2_SEQ_MAPPING_NAMES, trl.trainer.utils.ConstantLengthDataset (gated), trl.models.utils.disable_gradient_checkpointing (gated >=1.0.0), trl.import_utils + _*_available cache pattern, trl.experimental.openenv.utils generators (one of two names), GRPOTrainer required methods (_prepare_inputs, _generate_and_score_completions, compute_loss; per-token-logps legacy/new dispatch), GRPOTrainer source must contain torch.inference_mode + accelerator.unwrap_model fingerprints, KTOTrainer.get_batch_logps (now lives at trl.experimental.kto on TRL 0.27+ — accept either path), SFTTrainer class existence, DPOTrainer methods (informational), chat-template propagation (legacy maybe_apply_chat_template OR successor apply_chat_template + chat_template_kwargs), truncate_with_protected_tokens informational. - Tighten test_unwrap_model_for_generation_either_path to mirror the prod fallback exactly (drop unused trl/extras/profiling.py candidate). - Replace test_trl_generation_vllm_generation_gated symbol set with the actual unsloth dependency (VLLMGeneration class + _init_vllm / sync_weights / generate methods, not VLLMClient/etc). PEFT coverage extended (driven by the 8 PR audit unsloth#5015, #5167, #5036, #4807 + unsloth-zoo#618, #596, #482, #430): - VARIANT_KWARG_KEYS const (peft 0.18+; injected by zoo#430) - ParamWrapper class + members (peft 0.18+; needed by zoo#618) - LoraConfig.target_parameters (peft 0.19+) - LoraModel._create_and_replace (signature pin for unsloth#4807) - transformers_weight_conversion module + build_peft_weight_mapping (unsloth#5167 wraps this) - integrations.dequantize_module_weight (3 callsites) - PeftType.LORA (vllm_utils.py:2520) - ModulesToSaveWrapper (both peft.utils.* paths) - PeftModel.from_pretrained method exists - peft.__version__ parseable Transformers coverage added (driven by the 16-PR audit): - New file test_transformers_pinned_symbols.py with 19 test categories x 12 transformers tags (4.57.6 floor + 5.0..5.8 + main). Anchors: 4.57.6 + 5.5.0. - Trainer surface (compute_loss num_items_in_batch param, training_step grad-accum fingerprints, get_batch_samples num_items contract, inner_training_loop _tr_loss inplace v5) - modeling_utils.checkpoint alias for unsloth-zoo#549 - PushToHubMixin._create_repo presence (unsloth-zoo#393) - integrations.bitsandbytes module + Linear4bit reference - quantizers.should_convert_module signature (zoo#491/#488) - FP8Linear bias/has_bias rename (zoo#572) - processing_utils.Unpack importable (zoo#583/584) - gemma3 Gemma3Attention class + gpt_oss GptOssModel class - auto_factory _LazyAutoMapping private API (unsloth#5155) - configuration_utils PretrainedConfig/PreTrainedConfig alias - tokenization_utils_base.apply_chat_template - modeling_attn_mask_utils symbols - cache_utils Cache + DynamicCache classes - training_args.ParallelMode importable Wire the new transformers job into version-compat-ci.yml (matrix of 5 PR-time symbol jobs + zoo-imports under spoof + daily fresh- fetch cron). Local smoke: 906 pass, 73 skipped (gated optional features) across vLLM + TRL + PEFT + ST + bnb + transformers suites.
3 participants
Fixes: #5165