FourierFT Support #1838

Phoveran · 2024-06-09T17:44:03Z

Paper Link: https://arxiv.org/abs/2405.03003

Thanks for reviewing!

BenjaminBossan · 2024-06-10T09:33:23Z

Thanks a lot for adding this new method to PEFT. The method looks quite interesting. Based on your name, I assume that you're one of the paper authors.

I haven't done a full review yet. Could you add an example that we can run to see FourierFT in action? Maybe something based on LoRA, so that we can get a comparison of the effectiveness.

For a full PR, we also need to add documentations and tests. Would that be something you could work on? If you need help with those, always feel free to ask.

Phoveran · 2024-06-10T11:40:55Z

Thank you for the valuable comments!
I will be working on examples and documents. For tests, could you please give me some instruction on that?

BenjaminBossan · 2024-06-10T11:52:40Z

Thanks a lot. Let's start with the example first and then we can take a look at docs and tests.

Regarding the tests, you could for instance check past PRs that add new methods. E.g. in the VeRA PR, you can see how the tests have been extended.

To get a quick start to testing, I would, however, recommend to start by adding FourierFT to this test matrix:

peft/tests/test_custom_models.py

Line 59 in 683db0f

TEST_CASES = [

This should already cover like 90% of the relevant tests. Then run the tests locally with pytest tests/test_custom_models.py -k "ftfourier" and see if the tests pass.

Phoveran · 2024-06-10T11:59:47Z

Got it. Working on it :D

review-notebook-app · 2024-06-11T13:54:22Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

Phoveran · 2024-06-11T14:14:55Z

Hi @BenjaminBossan , we have finished the examples, tests and docs. Feel free to reach us if anything is needed!

HuggingFaceDocBuilderDev · 2024-06-11T15:20:58Z

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 · 2024-06-12T09:19:52Z

Could you please run make style? As is, the CI is failing because of ruff.

BenjaminBossan · 2024-06-12T11:47:40Z

Thanks for the updates. Code style checks are still failing, possible you have a different version of ruff on your system (CI uses v0.2.2).

Anyway, it should be easy to fix by either installing the same version of ruff and re-running, or by doing the following:

Reset the changes in lycoris_utils.py
Reset the changes in router.py (in general, you shouldn't have to update files that your PR didn't touch)
This line is too long, put the last word "model" on the next line.

BenjaminBossan · 2024-06-12T13:09:05Z

We're still seeing a failure mentioned in this comment:

This line is too long, put the last word "model" on the next line.

You can run doc-builder style src/peft tests docs/source --max_len 119 locally to check it.

Also, I updated PEFT to use the latest ruff version 0.4.8. So if you want to use that one instead, feel free to merge the latest main branch and upgrade your local ruff. But that's optional, you can also just keep the PR as is.

src/peft/tuners/fourierft/model.py

Co-authored-by: Benjamin Bossan <[email protected]>

Phoveran · 2024-06-12T14:02:46Z

Thanks! I just did not found the doc-builder... Anyway, should be OK now? Thank you a lot for the effort!

BenjaminBossan · 2024-06-12T14:42:03Z

I see that you already added a check for the n_frequency argument, nice. However, the tests will still fail because the custom model used for the test is very small. Could you please update the n_frequency argument here to be < 200? Just add an entry to the dict with the arguments.

By the way, you should be able to run the tests locally with pytest tests/ -k fourier. That way, you don't have to wait for the CI to run in order to see if the tests pass.

Phoveran · 2024-06-13T07:42:46Z

Thanks! We have passed the tests. Feel free to reach us if anything is needed!

BenjaminBossan · 2024-06-20T08:33:39Z

Thanks for the fixes. There are still some issues with running ruff on the CI, could you please run make style, or, alternatively:

ruff check --fix src tests examples docs
ruff format src tests examples docs
doc-builder style src/peft tests docs/source --max_len 119

The ruff version is v0.4.9.

zqgao22 · 2024-06-20T08:51:09Z

Thanks for the fixes. There are still some issues with running ruff on the CI, could you please run make style, or, alternatively:
ruff check --fix src tests examples docs
ruff format src tests examples docs
doc-builder style src/peft tests docs/source --max_len 119
The ruff version is v0.4.9.

Hi, thanks for the suggestions and we have further fixed style errors based on ruff version 0.4.9.

BenjaminBossan · 2024-06-20T09:17:38Z

Did you also run doc-builder style src/peft tests docs/source --max_len 119? It seems like there are still issues.

zqgao22 · 2024-06-20T09:59:25Z

Did you also run doc-builder style src/peft tests docs/source --max_len 119? It seems like there are still issues.

Hi, the newly pushed codes have undergone the doc-builder style check with hf-doc-builder=0.6.0.dev0.
Thanks for your further review.

BenjaminBossan

Thanks a lot for working on the remaining points. The PR is very advanced now and there are only some smaller issues left. Please check my comments.

On top, could you please also enable the adapter deletion tests for FourierFT:

peft/tests/testing_common.py

Line 1043 in d37dde6

supported_peft_types = [
peft/tests/testing_common.py

Line 1089 in d37dde6

supported_peft_types = [PeftType.LORA, PeftType.LOHA, PeftType.LOKR, PeftType.IA3, PeftType.OFT, PeftType.BOFT]

I think with the fix to random_loc_seed that I describe in one of my comments, these tests should pass.

I did not have the opportunity yet to review the notebook yet, will do so next week.

BenjaminBossan · 2024-06-20T11:45:37Z

src/peft/tuners/tuners_utils.py

+        print(self.other_param_names)
        for attr in self.adapter_layer_names + self.other_param_names:
+            print(attr)


Delete please

src/peft/tuners/fourierft/config.py

BenjaminBossan · 2024-06-20T11:54:16Z

src/peft/tuners/fourierft/layer.py

+        # Mark the weight as unmerged
+        self._disable_adapters = False
+        self.merged_adapters = []
+        self.fourierft_random_loc_seed = kwargs.pop("random_loc_seed")


Suggested change

self.fourierft_random_loc_seed = kwargs.pop("random_loc_seed")

self.fourierft_random_loc_seed = {}

The logic needs to be changed a bit here: As is, there would be only one value for this, even if there are multiple adapters. Instead, this should be a dict with one value per adapter and the adapter_name as key. So basically, treat it the same as e.g. fourierft_scaling. The update_layer method also needs to be updated to set the correct value for random_loc_seed.

src/peft/utils/constants.py

BenjaminBossan · 2024-06-20T12:02:51Z

tests/test_custom_models.py

@@ -865,7 +884,8 @@ def test_disable_adapters(self, test_name, model_id, config_cls, config_kwargs):
        )
        model = get_peft_model(model, config)
        model.eval()
-        outputs_before = model(**X)
+        with model.disable_adapter():


This change still needs to be reverted.

BenjaminBossan · 2024-06-20T12:04:12Z

src/peft/tuners/fourierft/layer.py

+        return delta_weight
+
+
+#  ------------------------------------------------------------------------------------------


This comment can be removed.

BenjaminBossan · 2024-06-20T12:05:54Z

src/peft/tuners/fourierft/config.py

+                "The initialization of the Fourier weights. Set this to False if the spectrum are initialized to a standard normal distribution."
+                "Set this to True if the spectrum are initialized to zeros."


Suggested change

"The initialization of the Fourier weights. Set this to False if the spectrum are initialized to a standard normal distribution."

"Set this to True if the spectrum are initialized to zeros."

"The initialization of the Fourier weights. Set this to False if the spectrum should be initialized to a standard normal distribution."

"Set this to True if the spectrum should be initialized to zeros."

Also, out of curiosity: Did you run some tests with weights initialized as zeros? If yes, did you find a noticeable performance drop?

Hi, thanks very much for reviewing. In our latest PR version, we add the recommended tests and correct the errors. Regarding this curiosity, we found that the two initialization schemes both don't have an absolute advantage. But overall, the default Gaussian initialization scheme in our paper (init_weights=False in codes) leads in more cases and converges faster.

Best.

Thanks for checking this.

Could you please still update the text to use "should be", which I think is clearer. Please update the docstring above accordingly and ensure that the character limit is respected.

BenjaminBossan · 2024-06-20T12:09:10Z

src/peft/tuners/fourierft/layer.py

+        n_frequency: int = 1000,
+        scaling: float = 150.0,
+        fan_in_fan_out: bool = False,  # Set this to True if the layer to replace stores weight like (fan_in, fan_out)
+        init_weights: Union[bool, str] = True,


Should the default be False, like in the config?

zqgao22 · 2024-07-01T04:23:20Z

Thanks a lot for working on the remaining points. The PR is very advanced now and there are only some smaller issues left. Please check my comments.

On top, could you please also enable the adapter deletion tests for FourierFT:

peft/tests/testing_common.py

Line 1043 in d37dde6

supported_peft_types = [

peft/tests/testing_common.py

Line 1089 in d37dde6

supported_peft_types = [PeftType.LORA, PeftType.LOHA, PeftType.LOKR, PeftType.IA3, PeftType.OFT, PeftType.BOFT]

I think with the fix to random_loc_seed that I describe in one of my comments, these tests should pass.

I did not have the opportunity yet to review the notebook yet, will do so next week.

Yes, these two tests are successfully performed after we complete the fix to random_loc_seed.

BenjaminBossan

Thanks so much for the recent changes. We're 99% there, I only found a few tiny issues left, which I think should be quickly fixed.

BenjaminBossan · 2024-07-01T12:36:19Z

src/peft/tuners/fourierft/config.py

+            The following examples of settings regarding 'n_frequency' can be used as reference for users. For NLU
+            tasks with RoBERTa-base and RoBERTa-large models, adopting 'n_frequency': 100 can almost achieve similar
+            results as 'r': 8 in LoRA. For image classification tasks with ViT-base and Vit-large models, adopting
+            'n_frequency': 3000 can almost achieve similar results as 'r': 16 in LoRA.


Thanks a lot for providing this detailed description, which should really help users pick a good value. I would add a little bit extra: As a naive user, when I read that n_frequency=3000 is roughly the same as r=16 in LoRA, I would think that FourierFT is much less parameter efficient, because 3000 >> 16. But of course this is an incorrect impression. So I would add a sentence to clarify the amount of trainable parameters.

src/peft/tuners/fourierft/config.py

BenjaminBossan · 2024-07-01T12:43:47Z

src/peft/tuners/fourierft/layer.py

+        # Mark the weight as unmerged
+        self._disable_adapters = False
+        self.merged_adapters = []
+        self.fourierft_random_loc_seed = {}


Let's move this a few lines up to be next to the other fourier-specific params.

BenjaminBossan · 2024-07-01T12:44:18Z

src/peft/tuners/fourierft/layer.py

+
+
+class FourierFTLinear(nn.Module, FourierFTLayer):
+    # Lora implemented in a dense layer


Suggested change

# Lora implemented in a dense layer

# FourierFT implemented in a dense layer

BenjaminBossan · 2024-07-01T12:46:01Z

src/peft/tuners/fourierft/model.py

+            "random_loc_seed": fourierft_config.random_loc_seed,
+        }
+        kwargs["bias"] = bias
+        if isinstance(target, FourierFTLinear):


How about:

Suggested change

if isinstance(target, FourierFTLinear):

if isinstance(target, FourierFTLayer):

in case that new layer types are added in the future.

src/peft/utils/constants.py

BenjaminBossan · 2024-07-01T12:48:52Z

examples/sequence_classification/FourierFT.ipynb

+    "model_name_or_path = \"roberta-base\"\n",
+    "task = \"mrpc\"\n",
+    "peft_type = PeftType.FOURIERFT\n",
+    "device = \"cuda:1\"\n",


Let's not hard-code this. How about: device = "cuda" if torch.cuda.is_available() else "cpu"?

BenjaminBossan · 2024-07-01T12:56:26Z

examples/sequence_classification/FourierFT.ipynb

+    "peft_type = PeftType.FOURIERFT\n",
+    "device = \"cuda:1\"\n",
+    "num_epochs = 5  # for better results, increase this number\n",
+    "n_frequency = 1000        # for better results, increase this number\n",


Let's also put scaling here, so that all hyper-params are in one place.

BenjaminBossan · 2024-07-01T12:56:41Z

examples/sequence_classification/FourierFT.ipynb

+    "    target_modules=[\"query\", \"value\"],\n",
+    "    scaling = 150.0,\n",
+    ")\n",
+    "head_lr = 6e-3\n",


Could you please add a comment about these two lr values?

BenjaminBossan · 2024-07-01T12:59:07Z

examples/sequence_classification/FourierFT.ipynb

@@ -0,0 +1,554 @@
+{


Let's mention that this notebook requires evaluate and scikit-learn to run, as those are not PEFT dependencies.

BenjaminBossan · 2024-07-02T09:18:33Z

@zqgao22 @Phoveran Let me know if this is ready for review.

zqgao22 · 2024-07-02T09:53:33Z

@zqgao22 @Phoveran Let me know if this is ready for review.

Yes, this current PR version is ready for review.

Best

BenjaminBossan

Thanks for the recent updates, we're almost finished now. Just some small issues left, please check.

BenjaminBossan · 2024-07-02T11:41:10Z

src/peft/tuners/fourierft/config.py

+                "The initialization of the Fourier weights. Set this to False if the spectrum are initialized to a standard normal distribution."
+                "Set this to True if the spectrum are initialized to zeros."


Thanks for checking this.

Could you please still update the text to use "should be", which I think is clearer. Please update the docstring above accordingly and ensure that the character limit is respected.

BenjaminBossan · 2024-07-02T11:49:41Z

examples/sequence_classification/FourierFT.ipynb

+    }
+   ],
+   "source": [
+    "#  To run this notebook, you can use `pip install scikit-learn evaluate` to install additional dependencies out of the PEFT.\n",


sklearn is no longer needed, is it?

Suggested change

"# To run this notebook, you can use `pip install scikit-learn evaluate` to install additional dependencies out of the PEFT.\n",

"# To run this notebook, please run `pip install evaluate` to install additional dependencies not covered by PEFT.\n",

zqgao22 · 2024-07-03T02:31:55Z

Hi @BenjaminBossan, you can review the latest version of my PR, titled small changes, ready for review. Thanks a lot.

Best.

src/peft/tuners/fourierft/model.py

BenjaminBossan · 2024-07-05T09:48:03Z

src/peft/tuners/fourierft/model.py

@@ -205,6 +205,8 @@ def __getattr__(self, name: str):
        try:
            return super().__getattr__(name)  # defer to nn.Module's logic
        except AttributeError:
+            if name == "base_model":


This should be if name == "model":. The idea is to prevent an infinite recursion in the line below in case that self.model is not yet set.

Thanks very much for your instruction, and we have fixed such recursion.

BenjaminBossan · 2024-07-05T13:22:09Z

Thanks for fixing the recursion issue. We just merged another method, which results in a merge conflict. Could you please take care of it. The changes should be super easy.

zqgao22 · 2024-07-05T14:12:56Z

Thanks for fixing the recursion issue. We just merged another method, which results in a merge conflict. Could you please take care of it. The changes should be super easy.

Yeah, we have resolved the (six) conflicts and you can kindly review the current version.

Best.

BenjaminBossan

Thanks for these updates, this PR is in an excellent state. Just a small thing I found is that we can extend the test coverage a bit further by adding multi-adapter tests. That's quite easy, just add the following lines:

MULTIPLE_ACTIVE_ADAPTERS_TEST_CASES = [
    ...
    (
        "FourierFT Same",
        "fourierft",
        FourierFTConfig,
        {"n_frequency": 10, "target_modules": ["lin0"]},
        {"n_frequency": 10, "target_modules": ["lin0"]},
    ),
    (
        "FourierFT Different",
        "fourierft",
        FourierFTConfig,
        {"n_frequency": 10, "target_modules": ["lin0"]},
        {"n_frequency": 10, "target_modules": ["lin1"]},
    ),
]

to the list defined here. I already checked this locally and the new tests pass.

zqgao22 · 2024-07-09T06:58:26Z

Thanks for these updates, this PR is in an excellent state. Just a small thing I found is that we can extend the test coverage a bit further by adding multi-adapter tests. That's quite easy, just add the following lines:
MULTIPLE_ACTIVE_ADAPTERS_TEST_CASES = [
    ...
    (
        "FourierFT Same",
        "fourierft",
        FourierFTConfig,
        {"n_frequency": 10, "target_modules": ["lin0"]},
        {"n_frequency": 10, "target_modules": ["lin0"]},
    ),
    (
        "FourierFT Different",
        "fourierft",
        FourierFTConfig,
        {"n_frequency": 10, "target_modules": ["lin0"]},
        {"n_frequency": 10, "target_modules": ["lin1"]},
    ),
]
to the list defined here. I already checked this locally and the new tests pass.

These tests have been included for our method, and the PR is ready for review. The new tests passed also in my local machine.

Best.

BenjaminBossan

Thanks a lot for this great PR, FourierFT looks very promising. All looks good to me, thanks for patiently incorporating my feedback.

Before merging, could you let me know whom to put on the co-author list, I see:

Co-authored-by: Chaos96 <[email protected]>
Co-authored-by: zqgao22 <[email protected]>
Co-authored-by: qichaoswang <[email protected]>
Co-authored-by: zgaoat <[email protected]>

zqgao22 · 2024-07-09T10:13:31Z

Before merging, could you let me know whom to put on the co-author list, I see:

Hi, thank you very much for your recent guidance, and we are glad to make this PR.

As for the co-author list, may it include the following four:

Best.

add fourier FT

e4f7995

Phoveran and others added 4 commits June 10, 2024 20:44

add docs, save and load dict

a20154f

add test

ddeeb5c

minor fix

7c2672c

annotation correction

763384a

zqgao22 force-pushed the main branch from 763384a to a494055 Compare June 11, 2024 13:54

Chaos96 force-pushed the main branch from a494055 to 763384a Compare June 11, 2024 14:06

Chaos96 added 3 commits June 11, 2024 22:08

add examples

4da3f2d

add examples

9a10d03

add examples

db598b2

code style fix

b2c3e59

Phoveran force-pushed the main branch from ed2c4d5 to b2c3e59 Compare June 12, 2024 13:40

fix line too long

ccefdd1

BenjaminBossan reviewed Jun 12, 2024

View reviewed changes

src/peft/tuners/fourierft/model.py Outdated Show resolved Hide resolved

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

749ebbb

Co-authored-by: Benjamin Bossan <[email protected]>

raise error if n is too large

ac55cb4

fix all errors in tests/

6953ca5

further style fixes

7196b16

pass doc-builder style check

6663f78

BenjaminBossan requested changes Jun 20, 2024

View reviewed changes

add tests and fix errors like random_loc_seed

a7fca70

BenjaminBossan requested changes Jul 1, 2024

View reviewed changes

modify the notebook, the config and some styles

45adafd

BenjaminBossan requested changes Jul 2, 2024

View reviewed changes

qichaoswang and others added 2 commits July 2, 2024 21:12

fix typo error and text expression

a82b816

small changes, ready for review

a19e2c5

BenjaminBossan reviewed Jul 3, 2024

View reviewed changes

src/peft/tuners/fourierft/model.py Show resolved Hide resolved

fix getattr bugs

f8571e0

BenjaminBossan reviewed Jul 5, 2024

View reviewed changes

fix recursion

b6c8d0b

Merge branch 'main' into main

2fa587a

BenjaminBossan reviewed Jul 8, 2024

View reviewed changes

added multi-adapter tests

de9c793

BenjaminBossan approved these changes Jul 9, 2024

View reviewed changes

BenjaminBossan merged commit e72a96f into huggingface:main Jul 9, 2024
14 checks passed

	self.fourierft_random_loc_seed = kwargs.pop("random_loc_seed")
	self.fourierft_random_loc_seed = {}

		return delta_weight


		# ------------------------------------------------------------------------------------------

		"The initialization of the Fourier weights. Set this to False if the spectrum are initialized to a standard normal distribution."
		"Set this to True if the spectrum are initialized to zeros."



		class FourierFTLinear(nn.Module, FourierFTLayer):
		# Lora implemented in a dense layer

	# Lora implemented in a dense layer
	# FourierFT implemented in a dense layer

	if isinstance(target, FourierFTLinear):
	if isinstance(target, FourierFTLayer):

	"# To run this notebook, you can use `pip install scikit-learn evaluate` to install additional dependencies out of the PEFT.\n",
	"# To run this notebook, please run `pip install evaluate` to install additional dependencies not covered by PEFT.\n",

FourierFT Support #1838

FourierFT Support #1838

Conversation

Phoveran commented Jun 9, 2024

BenjaminBossan commented Jun 10, 2024

Phoveran commented Jun 10, 2024

BenjaminBossan commented Jun 10, 2024

Phoveran commented Jun 10, 2024

review-notebook-app bot commented Jun 11, 2024

Phoveran commented Jun 11, 2024

HuggingFaceDocBuilderDev commented Jun 11, 2024

BenjaminBossan commented Jun 12, 2024

BenjaminBossan commented Jun 12, 2024

BenjaminBossan commented Jun 12, 2024

Phoveran commented Jun 12, 2024

BenjaminBossan commented Jun 12, 2024

Phoveran commented Jun 13, 2024

BenjaminBossan commented Jun 20, 2024

zqgao22 commented Jun 20, 2024

BenjaminBossan commented Jun 20, 2024

zqgao22 commented Jun 20, 2024

BenjaminBossan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zqgao22 commented Jul 1, 2024

BenjaminBossan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminBossan commented Jul 2, 2024

zqgao22 commented Jul 2, 2024

BenjaminBossan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zqgao22 commented Jul 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminBossan commented Jul 5, 2024

zqgao22 commented Jul 5, 2024

BenjaminBossan left a comment

Choose a reason for hiding this comment

zqgao22 commented Jul 9, 2024

BenjaminBossan left a comment • edited Loading

Choose a reason for hiding this comment

zqgao22 commented Jul 9, 2024

BenjaminBossan left a comment •

edited

Loading