add GLORA #2568

not-lain · 2025-06-03T14:10:35Z

based on main...Arnav0400:peft:main
and #1510 as well as #780

githubnemo

Thanks for the PR, this looks already quite mature. I especially like that you've gone through the trouble to add tests! I've added some comments on things that I think can be improved still.

I think it would make sense to also add a test case in test_custom_models.py since this will give a good coverage over the existing functionality in PEFT. Feel free to ask if you need assistance in doing so.

As I understand the paper they use random sampling + evolutionary search to select the best configuration for the different parameters (i.e., how to structure, say, A: LoRA, vector, scalar, none). PEFT aims to not interfere with the training process, as such I would suggest to not use random sampling of this configuration during init but to give the user the option to set the config once (via GloraConfig, similar to how eval_config works). If the user is keen to try search methods, such as evolutionary search, they can still use either GloraModel.set_adapter_config or, this would need to be added, something like glora.Linear.set_adapter_config, so that the user can do this for each layer individually (to support the ES case). Maybe there's a better name instead of set_adapter_config to set it apart from PEFT adapters. WDYT?

githubnemo · 2025-06-05T12:51:45Z

src/peft/tuners/glora/layer.py

+            try:
+                rank = int(config.split("_")[1])
+            except Exception:
+                rank = 4


This should simply raise an exception since it indicates an user error. Assuming a default will lead to unexpected behavior.

Suggested change

try:

rank = int(config.split("_")[1])

except Exception:

rank = 4

rank = int(config.split("_")[1])

(leaving this comment for future me to reflect on this one)
IMO rank should be extracted from config["r"] I may need to go back to the original implementation and check this in more depth

githubnemo · 2025-06-05T12:54:23Z

src/peft/tuners/glora/layer.py

+        if self.eval_config is None:
+            warnings.warn("eval_config not set for GLora layer, using a random config for merge.")
+            path_config = random.choice(self.configs)
+        else:
+            path_config = self.eval_config


I think this should raise an error explaining what to do to fix it instead of simply assuming a random config which might or might not work.

Ideally there's a test in tests/test_glora.py to test this behavior.

I think it would be better if we move eval_config to the GLoraConfig class and set a default value for that over there

githubnemo · 2025-06-05T12:55:45Z

tests/test_glora.py

+    def tearDown(self):
+        gc.collect()
+        torch.cuda.empty_cache()
+        gc.collect()


We have a test file explicitly for GPU/accelerator tests - all other tests assume to run on CPU, therefore this is not necessary.

githubnemo · 2025-06-05T13:00:18Z

tests/test_glora.py

+    @unittest.skipIf(
+        not torch.cuda.is_available()
+        or not hasattr(torch.cuda, "is_bf16_supported")
+        or not torch.cuda.is_bf16_supported(),
+        "BF16 not supported or no CUDA",
+    )


To also support XPU/Intel:

from accelerate.utils.imports import is_xpu_available from accelerate.utils.imports import is_bf16_available [...] @unittest.skipIf( not torch.cuda.is_available() or not is_xpu_available() or not is_bf16_available() "BF16 not supported or no CUDA/XPU", )

githubnemo · 2025-06-05T13:03:05Z

tests/test_glora.py

+        return ["decoded text"]
+
+
+class GLORATester(unittest.TestCase):


Great that you added tests! We're in the process of migrating existing tests to pytest. It's totally OK to keep these as is but if you want to convert these already to pytest I wouldn't complain :)

githubnemo · 2025-06-05T14:57:03Z

src/peft/tuners/glora/model.py

+        except AttributeError:
+            return getattr(self.model, name)


In LoRA we catch this case in reference to #1892, I think this code is susceptible to this as well.

Suggested change

except AttributeError:

return getattr(self.model, name)

except AttributeError:

if name == "model":

raise

return getattr(self.model, name)

githubnemo · 2025-06-05T15:08:33Z

src/peft/tuners/glora/model.py

+            if isinstance(target, GLoraLinear):
+                if target.eval_config is None:
+                    raise ValueError(
+                        f"eval_config not set for GLoraLinear layer {key}. Cannot merge deterministically. Please call model.set_adapter_eval_config(...) before merging."
+                    )
+
+                target.merge()
+                new_module = nn.Linear(target.in_features, target.out_features, bias=(target.bias is not None))
+                new_module.weight.data = target.weight.data.clone()  # Get merged weight
+                if target.bias is not None:
+                    new_module.bias.data = target.bias.data.clone()  # Get merged bias
+
+                self._replace_module(parent, target_name, new_module.to(target.weight.device), target)
+
+            if isinstance(target, ModulesToSaveWrapper):
+                pass


I think it makes sense to do implement this similar to how LoRA does it:

if hasattr(target, "unload_and_optionally_merge_module"): # if layers have special unloading method, like MultiheadAttention, use that unloaded_module = target.unload_and_optionally_merge_module( merge=merge, safe_merge=safe_merge, adapter_names=adapter_names ) self._replace_module(parent, target_name, unloaded_module, target) elif hasattr(target, "base_layer"): if merge: target.merge(safe_merge=safe_merge, adapter_names=adapter_names) self._replace_module(parent, target_name, target.get_base_layer(), target)

Of course this assumes that the target (e.g., glora.Linear) implements unload_and_optionally_merge_module but I would have suggested to implement it anyway since it is a good interface to have.

githubnemo · 2025-06-05T15:09:15Z

src/peft/tuners/glora/model.py

+    def get_peft_config_as_dict(self, inference: bool = False) -> dict[str, Any]:
+        config_dict = {}
+        for adapter_name, peft_config_obj in self.peft_config.items():
+            config = asdict(peft_config_obj)
+            if inference:
+                config["inference_mode"] = True
+            for k, v in config.items():
+                if isinstance(v, Enum):
+                    config[k] = v.value
+            config_dict[adapter_name] = config
+        return config_dict


I think this should be discussed in another PR, as it's not part of the BaseTuner and is part of each model in the library.
IMO we should make a different PR to shift this method to the BaseTuner.

src/peft/tuners/glora/model.py

githubnemo · 2025-06-05T15:11:55Z

src/peft/tuners/glora/model.py

+    def _find_and_replace(self, adapter_name: str):
+        glora_config = self.peft_config[adapter_name]
+        is_target_modules_in_base_model = False
+        key_list = [key for key, _ in self.model.named_modules()]  # Cache keys
+
+        for key in key_list:
+            if not self._check_target_module_exists(glora_config, key):
+                continue
+
+            is_target_modules_in_base_model = True
+            parent, target, target_name = _get_submodules(self.model, key)
+
+            if isinstance(target, GLoraLinear):
+                warnings.warn(
+                    f"Module {key} is already a GLoraLinear. Skipping replacement for new adapter '{adapter_name}'. Multiple GLORA adapters on the same layer might need explicit support in GLoraLinear."
+                )
+            elif isinstance(target, nn.Linear):
+                new_module = self._create_new_module(glora_config, adapter_name, target)
+                self._replace_module(parent, target_name, new_module, target)
+
+        if not is_target_modules_in_base_model:
+            raise ValueError(
+                f"Target modules {glora_config.target_modules} not found in the base model. "
+                f"Please check the target modules and try again."
+            )


This functionality is already implemented in BaseTuner.inject_adapter. I think _find_and_replace as well as add_adapter can be removed entirely?

not-lain

thanks a lot for the review!
will ping you once I resolve all the pointers

not-lain · 2025-06-22T21:08:07Z

src/peft/tuners/glora/layer.py

+            try:
+                rank = int(config.split("_")[1])
+            except Exception:
+                rank = 4


Suggested change

try:

rank = int(config.split("_")[1])

except Exception:

rank = 4

rank = int(config.split("_")[1])

src/peft/tuners/glora/model.py

src/peft/tuners/glora/layer.py

not-lain · 2025-07-05T18:13:28Z

src/peft/tuners/glora/model.py

+    def get_peft_config_as_dict(self, inference: bool = False) -> dict[str, Any]:
+        config_dict = {}
+        for adapter_name, peft_config_obj in self.peft_config.items():
+            config = asdict(peft_config_obj)
+            if inference:
+                config["inference_mode"] = True
+            for k, v in config.items():
+                if isinstance(v, Enum):
+                    config[k] = v.value
+            config_dict[adapter_name] = config
+        return config_dict


I think this should be discussed in another PR, as it's not part of the BaseTuner and is part of each model in the library.
IMO we should make a different PR to shift this method to the BaseTuner.

src/peft/tuners/glora/config.py

not-lain · 2025-07-05T19:27:25Z

src/peft/utils/constants.py

+TRANSFORMERS_MODELS_TO_GLORA_TARGET_MODULES_MAPPING = (
+    TRANSFORMERS_MODELS_TO_LORA_TARGET_MODULES_MAPPING  # need to check this later
+)


I did not test this yet, I was waiting for the workflow to run and see how it goes, this choice so far is based on intuition and nothing more, will inspect this in more detail further in the future.

not-lain and others added 3 commits June 3, 2025 15:05

add glora

57d1198

remove unecessary print statements

d264314

Merge branch 'huggingface:main' into glora

fc7e604

githubnemo reviewed Jun 5, 2025

View reviewed changes

Merge branch 'main' into glora

6b2556e

not-lain commented Jul 5, 2025

View reviewed changes

not-lain added 4 commits July 5, 2025 22:04

Update src/peft/tuners/glora/layer.py

8081b76

Update src/peft/tuners/glora/model.py

1fd49b4

Update src/peft/tuners/glora/layer.py

f45aee4

Update src/peft/tuners/glora/config.py

c78ad93

		return ["decoded text"]


		class GLORATester(unittest.TestCase):

add GLORA #2568

Are you sure you want to change the base?

add GLORA #2568

Uh oh!

Conversation

not-lain commented Jun 3, 2025

Uh oh!

githubnemo 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not-lain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!