fix: remove lm_head post processing #333
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Abhishek <[email protected]>
Abhishek-TAMU
requested review from
anhuong,
Ssukriti and
alex-jw-brooks
as code owners
anhuong
changed the title
anhuong
reviewed
Sep 5, 2024
build/Dockerfile
Outdated
| @@ -105,7 +105,7 @@ FROM cuda-devel AS python-installations | |||
| ARG WHEEL_VERSION | |||
| ARG USER | |||
| ARG USER_UID | |||
| ARG ENABLE_FMS_ACCELERATION=false | |||
| ARG ENABLE_FMS_ACCELERATION=true | |||
There was a problem hiding this comment.
Note that with this change, fms-acceleration can be installed by default thus enabling QLoRA support by default. The lm_head removal was blocking QLoRA enablement.
2 tasks
anhuong
reviewed
Sep 5, 2024
|
After testing, found that accelerate version is not working as expected. New logic intorduced in get_state_dict, also removes the top-level FSDP wrapper from the model. So then since FSDP keeps flattened params, all the parameters managed by the top-level wrapper will now remained flattened when model.state_dict is called. The other child FSDP wrappers will protect their parameters, since when the state_dict call recurses to them, they will use the FSDP version of state_dict to unwrap the wrappers. This results in error: |
anhuong
changed the title
Signed-off-by: Anh Uong <[email protected]>
Signed-off-by: Anh Uong <[email protected]>
anhuong
approved these changes
Sep 13, 2024
aluu317
pushed a commit
to aluu317/fms-hf-tuning
that referenced
this pull request
Sep 13, 2024
* fix: Removal of lm head hack Signed-off-by: Abhishek <[email protected]> * set fms_accelerate to true by default Signed-off-by: Anh Uong <[email protected]> --------- Signed-off-by: Abhishek <[email protected]> Signed-off-by: Anh Uong <[email protected]> Co-authored-by: Anh Uong <[email protected]> Signed-off-by: Angel Luu <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of the change
Removal of lm_head hack which was made to fix lm_head issue and now fixed with newer vllm versions, the change coming in as of v0.5.4
Related issue number
#1166
How to verify the PR
Running LoRA and full fine tuning of granite-3b and llama-8b model without removal of lm_head able to run inference on.
Was the PR tested