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.

Sign up for GitHub

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

Jump to bottom

Changes to support fsdp+qlora and dsz3+qlora #1550

Merged
merged 11 commits into from
Mar 13, 2024
Merged

Changes to support fsdp+qlora and dsz3+qlora #1550

merged 11 commits into from
Mar 13, 2024

Conversation

pacman100
Copy link
Contributor

What does this PR do?

  1. With the latest bitsandbytes version, the storage data type of 4-bit params is no longer fixed to unit8 as users can now use half or full precision in scenarios like FSDP+QLoRA. As such, this PR fixes the calculation of the number of parameters.

@HuggingFaceDocBuilderDev
Copy link

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.

@pacman100 pacman100 mentioned this pull request Mar 11, 2024
Merged
@pacman100 pacman100 marked this pull request as ready for review March 12, 2024 10:04
@pacman100 pacman100 mentioned this pull request Mar 12, 2024
Merged
@pacman100 pacman100 marked this pull request as draft March 12, 2024 12:10
younesbelkada
younesbelkada approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Great work thanks @pacman100 ! 🚀

younesbelkada
younesbelkada reviewed Mar 12, 2024
View reviewed changes
Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Left few nits on the docs

docs/source/accelerate/fsdp.md Outdated Show resolved Hide resolved
docs/source/accelerate/fsdp.md Outdated Show resolved Hide resolved
@pacman100 pacman100 marked this pull request as ready for review March 12, 2024 16:10
BenjaminBossan
BenjaminBossan requested changes Mar 12, 2024
View reviewed changes
Copy link
Member

@BenjaminBossan BenjaminBossan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot for the amount of work you put into this, Sourab, to show how users can get DS Z3 to work with QLoRA.

I think there are a few issues in the descriptions, but nothing major, please take a look. Also, this requires the most recent version of bnb, right? Also, not sure, but probably requires accelerate, trl and transformers from source? Could be useful to mention the version requirements in the docs.

docs/source/accelerate/deepspeed.md Outdated Show resolved Hide resolved
docs/source/accelerate/deepspeed.md Outdated Show resolved Hide resolved
docs/source/accelerate/deepspeed.md Outdated Show resolved Hide resolved
docs/source/accelerate/deepspeed.md Outdated Show resolved Hide resolved
docs/source/accelerate/fsdp.md Outdated Show resolved Hide resolved
docs/source/accelerate/fsdp.md Outdated Show resolved Hide resolved
docs/source/accelerate/fsdp.md Outdated Show resolved Hide resolved
docs/source/accelerate/fsdp.md Outdated Show resolved Hide resolved
docs/source/accelerate/fsdp.md Outdated Show resolved Hide resolved
docs/source/accelerate/deepspeed.md Outdated Show resolved Hide resolved
pacman100 and others added 2 commits March 13, 2024 13:06
@pacman100 @BenjaminBossan @younesbelkada
Apply suggestions from code review
8497e83 
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
Co-authored-by: Younes Belkada <49240599+younesbelkada@users.noreply.github.com>
@pacman100
address comments
dc70837
@pacman100
Copy link
Contributor Author

Also, this requires the most recent version of bnb, right? Also, not sure, but probably requires accelerate, trl and transformers from source? Could be useful to mention the version requirements in the docs.

Addressed this in the latest commit.

BenjaminBossan
BenjaminBossan approved these changes Mar 13, 2024
View reviewed changes
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 addressing the comments, this looks great.

younesbelkada
younesbelkada approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Huge work ! 🚀

@pacman100 pacman100 merged commit 6008f27 into main Mar 13, 2024
14 checks passed
BenjaminBossan added a commit to BenjaminBossan/peft that referenced this pull request Mar 15, 2024
@pacman100 @BenjaminBossan @younesbelkada
Changes to support fsdp+qlora and dsz3+qlora (huggingface#1550)
2cc70ca 
* changes to support fsdp+qlora and dsz3+qlora

* address comments

* add example and start docs

* quality

* deepspeed fixes

* dsz3+qlora docs

* section link fix

* add fsdp+qlora docs

* Apply suggestions from code review

Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
Co-authored-by: Younes Belkada <49240599+younesbelkada@users.noreply.github.com>

* address comments

---------

Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
Co-authored-by: Younes Belkada <49240599+younesbelkada@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@younesbelkada younesbelkada younesbelkada approved these changes

@BenjaminBossan BenjaminBossan BenjaminBossan approved these changes

@Titus-von-Koeller Titus-von-Koeller Awaiting requested review from Titus-von-Koeller

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pacman100 @HuggingFaceDocBuilderDev @BenjaminBossan @younesbelkada