[Suggestions] Updates suggested for helper.rescale_adapter_scale

[ariG23498]

Linked: #1940

CC: @BenjaminBossan

[BenjaminBossan]

Thanks for working on this PR so quickly.

Before going into details, I wanted to discuss something else that came to my mind, namely the argument name alpha. I wonder if we should rename it. Right now, the scaling factor is calculated as config.lora_alpha / config.r. I'm afraid that users could think that if they call, say, rescale_adapter_scale(model, alpha=config.r), they would get a scaling of 1, as they might think that this alpha actually sets the lora_alpha. I'm not sure what a better name would be, as something like scale could also be confusing. WDYT? Also pinging @SimJeg.

[ariG23498]

Would something like scale_multiplier make sense? Essentially we do the exact operation.

[SimJeg]

I agree using alpha is confusing. I proposed this based on the wise-ft paper but it will be confusing for users. I like scale_multiplier. @BenjaminBossan is there any other similar variable in PEFT to have something harmonized ?

[BenjaminBossan]

We have this in PEFT:

peft/src/peft/tuners/lora/layer.py

Line 276 in b926030

def set_scale(self, adapter, scale):

But as mentioned, I think "scale" is ambiguous, as it could be interpreted as the absolute value. However, I do like scale_multiplier, or even just multiplier, as I think that leaves less room for misinterpretation.

[ariG23498]

Renamed the function and the parameter.

[SimJeg]

@BenjaminBossan should we add some warning if device == "mps" ?

[BenjaminBossan]

should we add some warning if device == "mps" ?

I'd prefer to add a sentence about this in the docstring until we know better what the issue really is, something along the lines of: "Warning: It has been reported that when using Apple's MPS backend for PyTorch, it is necessary to add a short sleep time after exiting the context before the scales are fully restored".

At the moment, we don't even know if this replicates for other ARM Macs and all PyTorch versions. If it's an extreme edge case, the warning could do more harm than good.

[SimJeg]

Ok, once @ariG23498 add this to the docstring, I have no more comments :)

[BenjaminBossan]

@ariG23498 Could you please merge with/rebase on the latest main branch? Otherwise, some CI will fail. Sorry for the inconvenience.

[ariG23498]

Apologies for the delay! I have rebased just now.

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[BenjaminBossan]

LGTM. Thanks for the updates @ariG23498 and thanks @SimJeg for your valuable feedback.