Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add support for FSDP+QLoRA and DeepSpeed ZeRO3+QLoRA #29587
Add support for FSDP+QLoRA and DeepSpeed ZeRO3+QLoRA #29587
Changes from all commits
78d6dcc
2f51a20
5df8a65
595a16b
00312cf
4511a2c
574b371
b195b28
5081937
3da40ee
78e06e6
32f8c83
1401b73
3c56930
4840515
bef438b
4a6596b
ac6ddec
e554934
4c82852
a43d49d
f5fc519
6973569
7cde578
c40c767
73bda72
6f1eb11
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there any reason we couldn't have a
is_quantized
property of the model, which is by defaultFalse
? Having to constantly defineis_quantized
within the methods isn't ideal, as it requires updating in many different places if the criteria changeThere 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.
we already have property
hf_quantizer
for the model, storingis_quantized
would duplicate the same information. Earlier, I was directly usingself.hf_quantizer is None
in checks but there were suggestions above to improve readability usingis_quantized
, and as such, I made the changes accordingly.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.
Just to make sure I've understood - we don't need this anymore as we move creating quantized params when loading in the state dict?
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.
hf_quantizer is None
, i.e., not quantized will always be False as the conditional logicif is_fsdp_enabled() and not is_local_dist_rank_0() and not is_quantized
has this and as such this inner conditional is no longer required. The quantized parameters are initilaized in the else logic on line 3950