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: add lora fine tuning for llama 3.2 #958

Merged
merged 11 commits into from
Dec 19, 2024
Merged

feat: add lora fine tuning for llama 3.2 #958

merged 11 commits into from
Dec 19, 2024

Conversation

jfrery
Copy link
Collaborator

@jfrery jfrery commented Dec 10, 2024

No description provided.

@cla-bot cla-bot bot added the cla-signed label Dec 10, 2024
@jfrery jfrery marked this pull request as ready for review December 11, 2024 11:13
@jfrery jfrery requested a review from a team as a code owner December 11, 2024 11:13
Copy link
Collaborator

@kcelia kcelia left a comment

Choose a reason for hiding this comment

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

Thanks for your PR.

Some comments:

  • if we want to go for LoRA, maybe we should add it in the forbidden list, I stopped spamming you with my LoRA comments lol
  • The new Lora API is very cool
  • GPT2 and LLAma notebooks follow the same logic and share same utility functions, maybe we can create a utils file for them.
  • In GPT2 notebook, I think you don't use the full potential of the new LoRA API, or maybe you wanted to highlight what's happening behind the scene and I did not get it
  • In the 3 notebooks, I think it's not clear for the reader, if we are using FHE only for the inference or for adapters as well, maybe you should explicitly specify it in the conclusion or the introduction.

@jfrery
Copy link
Collaborator Author

jfrery commented Dec 16, 2024

GPT2 and LLAma notebooks follow the same logic and share same utility functions, maybe we can create a utils file for them.

I think they share a few function already with the utils file. GPT2 uses the previous API version without the LoraTrainer so a bit more complicated but more flexible as well.

In GPT2 notebook, I think you don't use the full potential of the new LoRA API, or maybe you wanted to highlight what's happening behind the scene and I did not get it

Yes I kept GPT2 without LoraTrainer to show that one could use its own training method but it implies defining the hybrid model / remote layers and so on.

In the 3 notebooks, I think it's not clear for the reader, if we are using FHE only for the inference or for adapters as well, maybe you should explicitly specify it in the conclusion or the introduction.

I will add a sentence at the beginning to make sure what we do here is clear.

Copy link
Collaborator

@kcelia kcelia left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

It would be nice to specify if the weights are encrypted too.

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[True-True-CNN_conv1d-relu]- tests/torch/test_compile_torch.py::test_compile_torch_or_onnx_conv_networks[False-True-CNN_grouped-relu]

kcelia
kcelia previously approved these changes Dec 18, 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.

please fix the gpt2 notebook convergence

- fix wrong unpacking of inputs in LoraTraining + add check
- add optimizer step in gpt2
- typo in llama notebook
- update version in requirements
Copy link

Coverage passed ✅

Coverage details

---------- coverage: platform linux, python 3.8.18-final-0 -----------
Name    Stmts   Miss  Cover   Missing
-------------------------------------
TOTAL    8490      0   100%

63 files skipped due to complete coverage.

@jfrery jfrery merged commit ef602d9 into main Dec 19, 2024
17 checks passed
@jfrery jfrery deleted the llama_fine_tuning branch December 19, 2024 15:33
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