[tests] Review tests for PR #5167#93
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
![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 constructors that lack support for distributed and quantization operations. The changes include the patching logic in unsloth/import_fixes.py, its integration in unsloth/init.py, and a new test suite. Feedback suggests using variable arguments in the wrapper function to avoid signature mismatches with older PEFT versions and removing the hasattr check when setting unsupported attributes to ensure they are available on the instance.
| def _build_peft_weight_mapping_compat( | ||
| weight_conversions, adapter_name, peft_config = None, | ||
| ): | ||
| if not weight_conversions: | ||
| return original_build(weight_conversions, adapter_name, peft_config) | ||
|
|
||
| patched_classes = [] | ||
| with patch_lock: | ||
| try: | ||
| _patch_weight_converter_ctors(weight_conversions, patched_classes) | ||
| return original_build(weight_conversions, adapter_name, peft_config) | ||
| finally: | ||
| for conversion_cls, original_init in patched_classes: | ||
| conversion_cls.__init__ = original_init |
There was a problem hiding this comment.
The wrapper _build_peft_weight_mapping_compat explicitly defines a third argument peft_config and passes it to original_build. If the installed version of peft has a build_peft_weight_mapping function that only accepts two arguments (legacy signature), this will cause a TypeError. It's safer to use *args and **kwargs to pass through all arguments to the original function, ensuring compatibility regardless of the underlying peft version.
| def _build_peft_weight_mapping_compat( | |
| weight_conversions, adapter_name, peft_config = None, | |
| ): | |
| if not weight_conversions: | |
| return original_build(weight_conversions, adapter_name, peft_config) | |
| patched_classes = [] | |
| with patch_lock: | |
| try: | |
| _patch_weight_converter_ctors(weight_conversions, patched_classes) | |
| return original_build(weight_conversions, adapter_name, peft_config) | |
| finally: | |
| for conversion_cls, original_init in patched_classes: | |
| conversion_cls.__init__ = original_init | |
| def _build_peft_weight_mapping_compat( | |
| weight_conversions, adapter_name, *args, **kwargs, | |
| ): | |
| if not weight_conversions: | |
| return original_build(weight_conversions, adapter_name, *args, **kwargs) | |
| patched_classes = [] | |
| with patch_lock: | |
| try: | |
| _patch_weight_converter_ctors(weight_conversions, patched_classes) | |
| return original_build(weight_conversions, adapter_name, *args, **kwargs) | |
| finally: | |
| for conversion_cls, original_init in patched_classes: | |
| conversion_cls.__init__ = original_init |
| if hasattr(self, name): | ||
| setattr(self, name, value) |
There was a problem hiding this comment.
The hasattr(self, name) check prevents setting distributed_operation or quantization_operation on the instance if they aren't already defined in the class. If the goal is compatibility with modern PEFT code that expects these attributes to be present on the converter instances, it is better to set them regardless of whether they were pre-defined. If a legacy converter doesn't define these fields at all, subsequent code accessing them will raise an AttributeError.
setattr(self, name, value)
danielhanchen
force-pushed
the
pr-5167-tests
branch
from
aaf8ecf to
d5bc435
Compare
|
Fixes pushed to unslothai#5167. |
3 participants
Automated test files from review process