fix the test_hotswapping_compiled_model_xxx case failure in xpu#13535
fix the test_hotswapping_compiled_model_xxx case failure in xpu#13535sywangyi wants to merge 1 commit intohuggingface:mainfrom
Conversation
Signed-off-by: Wang, Yi <yi.a.wang@intel.com>
|
@sayakpaul please help review |
|
@claude could you review this? |
|
Claude finished @sayakpaul's task in 4m 15s —— View job Review of PR #13535
SummaryThis PR fixes The root cause is well understood and the approach is reasonable. When Issues1. Attribute leak after cleanup ( _orig_compiled_call_impl = getattr(self, "_compiled_call_impl", None)
# ...
finally:
self._compiled_call_impl = _orig_compiled_call_impl # sets to None if attr didn't existIf Suggested fix — restore properly: _had_compiled_call_impl = hasattr(self, "_compiled_call_impl")
_orig_compiled_call_impl = getattr(self, "_compiled_call_impl", None)
if _supports_compiled_lora_hotswap and not hasattr(self, "_orig_mod"):
self._compiled_call_impl = True
try:
hotswap_adapter_from_state_dict(...)
finally:
if _had_compiled_call_impl:
self._compiled_call_impl = _orig_compiled_call_impl
elif hasattr(self, "_compiled_call_impl"):
delattr(self, "_compiled_call_impl")2. Scope may be too broad for non-compiled models The condition Consider narrowing the condition to only apply when the model is actually used with # Only force the compiled path when enable_lora_hotswap was called AND
# the model is actually inside a compile wrapper (self is the unwrapped inner model).
# We can check this by looking for _compiled_call_impl on the outer wrapper,
# or by tracking an additional flag.This is a nit — if the existing tests pass across all backends with the broader condition, it's acceptable as-is. But worth noting for maintainability. 3. Consider upstreaming to peft This workaround compensates for peft's inability to detect that the model is inside a What looks good
VerdictThe fix is correct in intent and should resolve the XPU test failures. Issue #1 (attribute leak) should be addressed before merge. Issue #2 is optional. Issue #3 is a longer-term suggestion. |
BenjaminBossan
left a comment
There was a problem hiding this comment.
I'm agreeing with Claude's 3rd point, namely that I'd rather like to see this fixed in PEFT. Is there a technical reason why it's not possible? Otherwise, a similar patch would need to be rolled out to transformers and to every user of PEFT hotswapping with a similar use case and XPU.
| adapter_name=adapter_name, | ||
| config=lora_config, | ||
| ) | ||
| # When enable_lora_hotswap was called, weights were padded to target_rank so that |
There was a problem hiding this comment.
If we keep things like this, how about turning this into a context manager to keep the code inside of load_lora_adapter more readable? It would also allow to re-use the context manager if it's ever needed.
|
@BenjaminBossan thanks for the review, I put the fix in peft. huggingface/peft#3183 |
|
this WA fix could be close and put the fix in peft |
following case fail
tests/models/transformers/test_models_transformer_bria.py::BriaTransformerLoRAHotSwapTests::test_hotswapping_compiled_model_linear_2
tests/models/transformers/test_models_transformer_bria.py::BriaTransformerLoRAHotSwapTests::test_hotswapping_compiled_model_both_linear_and_other_2
tests/models/transformers/test_models_transformer_chroma.py::ChromaTransformerLoRAHotSwapTests::test_hotswapping_compiled_model_linear_2
tests/models/unets/test_models_unet_2d_condition.py::UNet2DConditionModelLoRAHotSwapTests::test_hotswapping_compiled_model_conv2d_2
tests/models/unets/test_models_unet_2d_condition.py::UNet2DConditionModelLoRAHotSwapTests::test_hotswapping_compiled_model_both_linear_and_conv2d_2
tests/models/unets/test_models_unet_2d_condition.py::UNet2DConditionModelLoRAHotSwapTests::test_hotswapping_compiled_model_both_linear_and_other_2
tests/models/unets/test_models_unet_2d_condition.py::UNet2DConditionModelLoRAHotSwapTests::test_hotswapping_compiled_model_linear_2
tests/models/transformers/test_models_transformer_chroma.py::ChromaTransformerLoRAHotSwapTests::test_hotswapping_compiled_model_both_linear_and_other_2