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] EVA: ensure deterministic behavior of SVD on multi gpu setups #2225

Merged
merged 8 commits into from
Nov 21, 2024

Conversation

sirluk
Copy link
Contributor

@sirluk sirluk commented Nov 20, 2024

Deterministic behavior of SVD computations on layer inputs is important on multi gpu setups to ensure resulting EVA state dicts are identical. Currently deterministic behaviour of SVD is ensured by just setting torch.manual_seed() inside _get_eva_state_dict which is not ideal because it can interfere with the seed set by the user. I therefore moved this functionality inside the IncrementalPCA class.

I also added a new argument for get_eva_state_dict: gather_distributed_inputs which is False by default and defines if inputs should be concatenated for the SVD computation for distributed runs.

Copy link
Member

@BenjaminBossan BenjaminBossan 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 these small fixes for distributed EVA. Overall, this looks good as is but I have added some comments for potential improvement, please check.

src/peft/tuners/lora/eva.py Outdated Show resolved Hide resolved
src/peft/tuners/lora/eva.py Outdated Show resolved Hide resolved
src/peft/tuners/lora/eva.py Outdated Show resolved Hide resolved
src/peft/tuners/lora/eva.py Outdated Show resolved Hide resolved
src/peft/utils/incremental_pca.py Outdated Show resolved Hide resolved
src/peft/utils/incremental_pca.py Outdated Show resolved Hide resolved
@HuggingFaceDocBuilderDev

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.

Copy link
Member

@BenjaminBossan BenjaminBossan 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 updates. I have some nits, WDYT?

src/peft/tuners/lora/eva.py Outdated Show resolved Hide resolved
examples/eva_finetuning/eva_finetuning_multi_gpu.py Outdated Show resolved Hide resolved
@sirluk
Copy link
Contributor Author

sirluk commented Nov 21, 2024

@BenjaminBossan thanks for the suggestions, I integrated them!

Copy link
Member

@BenjaminBossan BenjaminBossan 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 these updates to EVA, LGTM.

@BenjaminBossan BenjaminBossan merged commit 029faf6 into huggingface:main Nov 21, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants