-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[Low-level-API
] Add docs about LLAPI
#836
[Low-level-API
] Add docs about LLAPI
#836
Conversation
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the docs and tests. They're very clear, I only have minor comments. Please take a look.
README.md
Outdated
@@ -355,6 +355,42 @@ any GPU memory savings. Please refer issue [[FSDP] FSDP with CPU offload consume | |||
|
|||
2. When using ZeRO3 with zero3_init_flag=True, if you find the gpu memory increase with training steps. we might need to update deepspeed after [deepspeed commit 42858a9891422abc](https://github.com/microsoft/DeepSpeed/commit/42858a9891422abcecaa12c1bd432d28d33eb0d4) . The related issue is [[BUG] Peft Training with Zero.Init() and Zero3 will increase GPU memory every forward step ](https://github.com/microsoft/DeepSpeed/issues/3002) | |||
|
|||
## 🤗 PEFT as a utility library | |||
|
|||
Inject trainable adapters on any `torch` model using `inject_adapter_in_model` method: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure exactly about the wording, but I think it's worth highlighting here that calling this function will only inject the adapters but make no further changes to the model. Otherwise, users may be confused why they should use this and not get_peft_model
.
README.md
Outdated
) | ||
|
||
model = DummyModel() | ||
model = inject_adapter_in_model(lora_config, model, "default") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I wonder now why we did not have adapter_name="default"
as a default argument? I think it would help here. If we don't want it, at least I would pass it as keyword argument in this example, not positional, to make it clear what the meaning of "default"
is.
If we change the function to make the argument a default argument (ugh, so confusing, default vs "default"
), the docs below also need to change a little bit ("that takes 3 arguments ...").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great, will address this change
from peft import LoraConfig, get_peft_model_state_dict, inject_adapter_in_model | ||
|
||
|
||
class DummyModel(torch.nn.Module): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that you added tests.
I think the tests could also be added to test_custom_models.py
. The disadvantage of that change would be that as is, the tests are very clear and straightforward. The advantage would be that the tests in test_custom_models.py
can be easily parametrized with different custom modules and configs, so the test coverage is better.
I would be okay if you want to keep it as is, it's just a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see ok thanks for explaining! I would say maybe let's keep it as it is since the test is aimed to only test the tiny snippet of the README and the docs so I want to keep it very simple and minimal for now
- Works for any torch module, and any modality (vision, text, multi-modal) | ||
|
||
Cons: | ||
- You need to manually writing saving and loading utility methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You need to manually writing saving and loading utility methods | |
- You need to manually write saving and loading utility methods |
I think this could be confusing. I guess what you mean is stuff like from_pretrained
etc. But people can still use the normal torch.save
and torch.load
that they already know. Saying they have to "manually" write the methods probably sounds worse than it actually is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm correct, let me rephrase this a bit then
|
||
Cons: | ||
- You need to manually writing saving and loading utility methods | ||
- You cannot use any of the utility method provided by `PeftModel` such as disabling adapters, merging adapters, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a link to this section: https://huggingface.co/docs/peft/conceptual_guides/lora#utils-for-lora
Also, I took a look at some of the methods that are currently used for merging, unloading etc. and I think that with only a few changes, we can make them standalone functions (like inject_adapter_in_model
) that don't require a PeftModel
/ LoraModel
etc. At least for LoRA that should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great, we can do that in a follow up PR!
Thanks for the extensive review @BenjaminBossan ! Should have addressed the comments by now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic, thanks for making the changes.
What does this PR do?
As discussed internally, this PR adds nice documentation and simple tests about the recent low-level API from #749
cc @BenjaminBossan @pacman100