Implements EWoRA in PEFT by HarshKohli · Pull Request #3401 · huggingface/peft

HarshKohli · 2026-07-07T20:38:21Z

Resolves #3400

BenjaminBossan

Thanks for reviving the EWoRA PR. For this review, I focused exclusively on the PEFT integration itself, keeping the rest for later.

From what I can tell, you probably started this PR based on a somewhat older version of PEFT and taking the LoRA code as a starting point. For this reason, numerous additions are not needed or are out of date. I marked the corresponding code. The good news is that we now require much less code, so overall this should lead to a simplification.

To get the majority of test cases covered, please start by adding EWoRA to the test matrix of our custom model tests. Since EWoRA does not allow merging, add it to this function too. Then run pytest tests/test_custom_models.py -k "ewora" -v and check if all tests pass.

Also, notice that we have reworked our contribution guideline, please check it out. For now, we're at the "Core integration" step, but to merge, we will need to cover the "Final PR" step too. From my perspective, it's, however, okay to finish step one first before proceeding.

BenjaminBossan · 2026-07-08T13:08:59Z

@@ -0,0 +1,70 @@
+<!--Copyright 2025 The HuggingFace Team. All rights reserved.


Suggested change

<!--Copyright 2025 The HuggingFace Team. All rights reserved.

<!--Copyright 2026 The HuggingFace Team. All rights reserved.

Please use the current year here and throughout.

BenjaminBossan · 2026-07-08T13:10:33Z

+
+    Args:
+        r (`int`):
+            Ewora expert ranks.


Let's make it clear if it's the total rank of all experts (thus comparable to the LoRA rank) or the rank per expert (in which case r * num_experts is the correct comparison to LoRA). Also mention the defaults for all EWoRA specific parameters.

BenjaminBossan · 2026-07-08T13:43:01Z

+    tuner_layer_cls = EworaLayer
+    target_module_mapping = TRANSFORMERS_MODELS_TO_EWORA_TARGET_MODULES_MAPPING
+
+    def __init__(self, model, config, adapter_name) -> None:


Method not needed, pls remove

BenjaminBossan · 2026-07-08T13:43:28Z

+    def __init__(self, model, config, adapter_name) -> None:
+        super().__init__(model, config, adapter_name)
+
+    def _check_new_adapter_config(self, config: EworaConfig) -> None:


Method not needed, pls remove

BenjaminBossan · 2026-07-08T14:02:38Z

+                x = x.to(ewora_As.dtype)
+                x = x.unsqueeze(2).expand(-1, -1, num_experts, -1)
+
+                intermediate = torch.einsum("beid, idj -> beij", dropout(x), ewora_As)


Let's add a short comment to explain the indices b, d, e, i, j

BenjaminBossan · 2026-07-08T14:02:57Z

+                # scores = F.softmax(weighting(F.relu(intermediate.reshape(bs, seq_len, -1))))
+                # scores = weighting(intermediate.reshape(bs, seq_len, -1))


BenjaminBossan · 2026-07-08T14:03:41Z

+                del intermediate
+
+                final = final * scores.unsqueeze(-1)
+                result.add_(final.sum(dim=2))


Is this better than result = result + final.sum(dim=2)?

BenjaminBossan · 2026-07-08T14:04:23Z

+                final = final * scores.unsqueeze(-1)
+                result.add_(final.sum(dim=2))
+
+                del final, scores


Regarding the manual deletions, did you measure that this has an effect? Normally, we don't have those calls in PEFT, it would only make sense if we find that they're required for proper memory management.

BenjaminBossan · 2026-07-08T14:08:50Z

This whole test file can be deleted. We already have an extensive test suite, and once EWoRA is added there, the test coverage will be pretty much complete. We don't have tests that check, for instance, if assert lin0.ewora_As["default"].shape == (num_experts, 10, r) , but that's really not needed, as this is basically checking for implementation details

HarshKohli · 2026-08-04T17:38:02Z

@BenjaminBossan - thank you for reviewing my pull request and for the feedback. I have done a re-request to review - hope I have addressed most of your concerns. Please advise on next steps.

BenjaminBossan

Thank you for the updates and for bringing the PR up-to-speed with the latest PEFT. In this review, I focused on the integration, which is not missing much; I haven't checked the docs or ran the experiments yet.

As the core integration is almost done, please consult our contribution guideline for the next steps.

BenjaminBossan · 2026-08-06T09:40:25Z

+        rank_pattern (`dict`):
+            The mapping from layer names or regexp expression to ranks which are different from the default rank
+            specified by `r`.
+        megatron_config (`Optional[dict]`):


megatron_config, megatron_core, use_dora, and layer_replication are not being used and are just leftovers from starting EWoRA based on LoRA. Please remove these arguments.

BenjaminBossan · 2026-08-06T09:40:35Z

+            The alpha parameter for Ewora scaling.
+        ewora_dropout (`float`):
+            The dropout probability for Ewora layers.
+        fan_in_fan_out (`bool`):


BenjaminBossan · 2026-08-06T09:41:11Z

+        fan_in_fan_out (`bool`):
+            Set this to True if the layer to replace stores weight like (fan_in, fan_out). For example, gpt-2 uses
+            `Conv1D` which stores weights like (fan_in, fan_out) and hence this should be set to `True`.
+        bias (`str`):


This is not a very useful option to have, let's simplify our lives and just remove it.

BenjaminBossan · 2026-08-06T09:42:34Z

+    Args:
+        r (`int`):
+            The rank of each individual expert adapter (defaults to 256). EWoRA partitions the adaptation into
+            `num_experts` independent rank-`r` experts, so the LoRA-equivalent total rank is `r * num_experts`.


So with the default settings, we basically get 8 (num_experts) LoRA adapters with rank 256, do I see that right? That is quite a huge default, compared to LoRA, which is just rank 8 by default. Does it really make sense to pick such a high number?

BenjaminBossan · 2026-08-06T09:45:46Z

+        if isinstance(base_layer, nn.Linear):
+            in_features, out_features = base_layer.in_features, base_layer.out_features
+        elif isinstance(base_layer, nn.Conv2d):
+            in_features, out_features = base_layer.in_channels, base_layer.out_channels
+        elif isinstance(base_layer, nn.Embedding):
+            in_features, out_features = base_layer.num_embeddings, base_layer.embedding_dim
+        elif isinstance(base_layer, Conv1D):
+            in_features, out_features = (
+                base_layer.weight.ds_shape if hasattr(base_layer.weight, "ds_shape") else base_layer.weight.shape
+            )
+        elif hasattr(base_layer, "infeatures") and hasattr(base_layer, "outfeatures"):
+            # QuantLinear
+            in_features, out_features = base_layer.infeatures, base_layer.outfeatures
+        elif hasattr(base_layer, "input_size") and hasattr(base_layer, "output_size"):
+            # Megatron ColumnParallelLinear,RowParallelLinear
+            in_features, out_features = base_layer.input_size, base_layer.output_size
+        elif hasattr(base_layer, "codebooks") and base_layer.__class__.__name__ == "QuantizedLinear":
+            # AQLM QuantLinear
+            in_features, out_features = base_layer.in_features, base_layer.out_features
+        elif hasattr(base_layer, "w_bit") and base_layer.__class__.__name__ == "WQLinear_GEMM":
+            # Awq layers
+            in_features, out_features = base_layer.in_features, base_layer.out_features
+        elif base_layer.__class__.__name__ == "EetqLinear":
+            # Eetq layers
+            in_features, out_features = base_layer.in_features, base_layer.out_features
+        elif hasattr(base_layer, "W_q") and base_layer.__class__.__name__ == "HQQLinear":
+            # HQQ layers
+            in_features, out_features = base_layer.in_features, base_layer.out_features


We have a utility function now that can simplify the pattern: peft.tuners.utils._get_in_out_features.

BenjaminBossan · 2026-08-06T09:46:38Z

+            ewora_dropout_layer = nn.Dropout(p=config.ewora_dropout)
+        else:
+            ewora_dropout_layer = nn.Identity()
+        self.ewora_dropout.update(nn.ModuleDict({adapter_name: ewora_dropout_layer}))


Simpler:

Suggested change

self.ewora_dropout.update(nn.ModuleDict({adapter_name: ewora_dropout_layer}))

self.ewora_dropout[adapter_name] = ewora_dropout_layer

BenjaminBossan · 2026-08-06T09:50:29Z

+        self.update_layer(adapter_name, r, config)
+        self.is_target_conv_1d_layer = is_target_conv_1d_layer
+
+    def forward(self, x: torch.Tensor, *args, **kwargs) -> torch.Tensor:


AFAICT, there is no scaling of the LoRA outputs, i.e. something that corresponds to alpha, is that intended? In the paper, you mention that "LoRA Scaling Parameter (α): 32" is "used in all our training
experiments" though it's not quite clear if that also applies to EWoRA in this case. But since it's an important hyper-parameter for LoRA, I would expect it to matter here too.

FEAT Add EWoRA tuner

b7c0577

BenjaminBossan requested changes Jul 8, 2026

View reviewed changes

HarshKohli added 2 commits August 4, 2026 13:01

addressing review feedback

f612bf2

Merge remote-tracking branch 'upstream/main' into ewora-clean

a3e8827

HarshKohli requested a review from BenjaminBossan August 4, 2026 17:35

BenjaminBossan requested changes Aug 6, 2026

View reviewed changes

		@@ -0,0 +1,70 @@
		<!--Copyright 2025 The HuggingFace Team. All rights reserved.

	<!--Copyright 2025 The HuggingFace Team. All rights reserved.
	<!--Copyright 2026 The HuggingFace Team. All rights reserved.

		# scores = F.softmax(weighting(F.relu(intermediate.reshape(bs, seq_len, -1))))
		# scores = weighting(intermediate.reshape(bs, seq_len, -1))

	self.ewora_dropout.update(nn.ModuleDict({adapter_name: ewora_dropout_layer}))
	self.ewora_dropout[adapter_name] = ewora_dropout_layer

Uh oh!

Conversation

HarshKohli commented Jul 7, 2026

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HarshKohli commented Aug 4, 2026

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants