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

[Templates] Remove unnecessary requirements and intro flash attn #42169

Conversation

ArturNiederfahrenhorst
Copy link
Contributor

@ArturNiederfahrenhorst ArturNiederfahrenhorst commented Jan 3, 2024

Why are these changes needed?

As @scottsun94 noted, the requirements that this PR removes are not needed to execute the llm finetuning template.
This PR also adds flash attention to the template and pins deepspeed.

@scottsun94
Copy link
Contributor

I didn't run the lora part. But for 7B, 13B, and 70B full parameter fine-tuning, I can run the template code without these 2 dependencies. I didn't compare the performances, etc.

Someone should confirm whether they are needed:

  • for lora
  • for better performance
  • etc.

@ArturNiederfahrenhorst
Copy link
Contributor Author

ArturNiederfahrenhorst commented Jan 4, 2024

Thanks.

@ArturNiederfahrenhorst ArturNiederfahrenhorst marked this pull request as ready for review January 8, 2024 22:54
Copy link
Contributor

Choose a reason for hiding this comment

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

we should pin deepspeed to the right version as well.

sentencepiece==0.1.99 \
"urllib3<1.27" \
git+https://github.com/huggingface/transformers.git@d0c1aeb \
git+https://github.com/huggingface/peft.git@08368a1fba16de09756f067637ff326c71598fb3
pip3 install -U flash-attn --no-build-isolation
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we pin a version please?

peft==0.7.0
flash-attn --global-option="--no-build-isolation"
Copy link
Collaborator

Choose a reason for hiding this comment

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

pin a version?

@ArturNiederfahrenhorst ArturNiederfahrenhorst changed the title [Templates] Remove unnecessary requirements [Templates] Remove unnecessary requirements and intro flash attn Jan 8, 2024
Signed-off-by: Artur Niederfahrenhorst <attaismyname@googlemail.com>
@ArturNiederfahrenhorst
Copy link
Contributor Author

I requested review by @justinvyu , who'll review today.

@@ -1,4 +1,4 @@
deepspeed
Copy link
Contributor

Choose a reason for hiding this comment

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

does this template work on 0.12.3? Last time we tried it used to have some problems above 0.10.3. Am I remembering a different context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0.12.3 is the one that we install in the ray release byod. So I'd expect it to work.

Copy link
Contributor

@justinvyu justinvyu left a comment

Choose a reason for hiding this comment

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

One question, but otherwise lgtm.

@@ -10,6 +10,7 @@ RUN pip install --upgrade pip
RUN pip install -U -r requirements.txt
RUN pip install torch==2.1.1 --index-url https://download.pytorch.org/whl/cu121
RUN pip uninstall bitsandbytes -y
RUN pip install flash-attn==2.4.2 --global-option="--no-build-isolation"
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the purpose of this --no-build-isolation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, no clue. It's the recommended way of installing flash attention according to their github repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-01-10 at 22 09 31

@ArturNiederfahrenhorst
Copy link
Contributor Author

Screenshot 2024-01-15 at 15 57 51

Release tests succeeded on AWS.

Copy link
Collaborator

@can-anyscale can-anyscale left a comment

Choose a reason for hiding this comment

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

@ArturNiederfahrenhorst ArturNiederfahrenhorst merged commit 53e9b33 into ray-project:master Jan 15, 2024
8 of 9 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.

6 participants