Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: lora fine-tuning in FHE + gpt2 use case example #823

Merged
merged 32 commits into from
Sep 26, 2024

Conversation

RomanBredehoft
Copy link
Contributor

currently running the notebook with 100 epochs

refs https://github.com/zama-ai/concrete-ml-internal/issues/4522

@cla-bot cla-bot bot added the cla-signed label Aug 2, 2024
@RomanBredehoft RomanBredehoft force-pushed the docs/add_hybrid_lora_fine_tuning branch 7 times, most recently from 23ebfd9 to f20697d Compare August 6, 2024 17:21
@RomanBredehoft
Copy link
Contributor Author

RomanBredehoft commented Aug 16, 2024

timings are from my computer, it hasn't been refreshed

@RomanBredehoft RomanBredehoft changed the title docs/add_hybrid_lora_fine_tuning docs: add hybrid lora fine tuning use case Aug 16, 2024
@jfrery jfrery force-pushed the docs/add_hybrid_lora_fine_tuning branch from 6c1caeb to e63afe9 Compare September 9, 2024 12:07
@jfrery jfrery force-pushed the docs/add_hybrid_lora_fine_tuning branch from e63afe9 to 1e92c43 Compare September 23, 2024 10:46
@jfrery jfrery changed the title docs: add hybrid lora fine tuning use case feat: lora fine-tuning in FHE + gpt2 use case example Sep 23, 2024
Copy link
Collaborator

@andrei-stoian-zama andrei-stoian-zama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't reviewed everything yet.

Main issue is :

  • can we make it work for other models than GPT2 (for example the simple MLP)
  • make the LoraTraining use self.training to determine if it's doing inference or training

src/concrete/ml/torch/lora.py Outdated Show resolved Hide resolved
src/concrete/ml/torch/lora.py Show resolved Hide resolved
src/concrete/ml/torch/lora.py Outdated Show resolved Hide resolved
src/concrete/ml/torch/lora.py Show resolved Hide resolved
src/concrete/ml/torch/lora.py Outdated Show resolved Hide resolved
tests/torch/test_lora.py Outdated Show resolved Hide resolved
src/concrete/ml/torch/lora.py Show resolved Hide resolved
tests/torch/test_lora.py Outdated Show resolved Hide resolved
use_case_examples/lora_finetuning/README.md Show resolved Hide resolved
use_case_examples/lora_finetuning/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@andrei-stoian-zama andrei-stoian-zama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to remove the transformer dependency:

  • move the dep to the use case example, replace conv1d -> nn.Linear in the usecase
  • parametrize the remote module finding function in way that avoids the dep

from typing import List

import torch
from transformers import Conv1D as TransformerConv1D
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding the dependency to transformers .. we had removed it earlier

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright done

return grad_input, None, None


def get_remote_names(model: torch.nn.Module, include_embedding_layers: bool = False) -> List[str]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is too specific for a library function. e.g. for the lm_head moniker.

I suggest you add two arguments remote_layer_types and layer_reject_filter.

You can then call with remote_layer_types = [nn.Linear, nn.Embedding, transformer.Conv1D]. Thus CML does not need to know TransformerConv1D. and layer_reject_filter = ['lm_head]`

You can thus remove the include_embedding_layers flag.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but in the function we manually add CustomLinear if the user asks for nn.Linear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure here. This is very specific with the replace_layers_with_custom. I would not allow the user to select layers himself. For now we do linear layers in FHE and we have a workaround for lm_head and embedding as long as there are not fixed.

@jfrery jfrery force-pushed the docs/add_hybrid_lora_fine_tuning branch from 09a96b5 to 0379e15 Compare September 26, 2024 07:29
Copy link

⚠️ Known flaky tests have been rerun ⚠️

One or several tests initially failed but were identified as known flaky. tests. Therefore, they have been rerun and passed. See below for more details.

Failed tests details

Known flaky tests that initially failed:

  • tests/torch/test_compile_torch.py::test_compile_torch_or_onnx_conv_networks[False-True-CNN-relu]

Copy link

Coverage failed ❌

Coverage details

---------- coverage: platform linux, python 3.8.18-final-0 -----------
Name                                 Stmts   Miss  Cover   Missing
------------------------------------------------------------------
src/concrete/ml/onnx/onnx_utils.py      59     10    83%   594-620
------------------------------------------------------------------
TOTAL                                 8276     10    99%

60 files skipped due to complete coverage.

@jfrery jfrery marked this pull request as ready for review September 26, 2024 08:52
@jfrery jfrery requested a review from a team as a code owner September 26, 2024 08:52
Copy link
Collaborator

@andrei-stoian-zama andrei-stoian-zama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work here @RomanBredehoft and @jfrery !

@jfrery jfrery merged commit 4d2f2e6 into main Sep 26, 2024
15 checks passed
@jfrery jfrery deleted the docs/add_hybrid_lora_fine_tuning branch September 26, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants