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

[Tests] Add bitsandbytes installed from source on new docker images #1275

Merged
merged 4 commits into from
Dec 18, 2023

Conversation

younesbelkada
Copy link
Contributor

What does this PR do?

As per title, this will make life much easier for bnb developpers. I propose to create two separate docker images, both with bnb installed from source and run PEFT + transformers slow tests on daily basis, similarly as what we do currently with PEFT.

I can confirm building these docker images were successful on my local VM

cc @LysandreJik @Titus-von-Koeller @pacman100 @BenjaminBossan
Will first test if the workflow works fine here on GH

- cron: "0 1 * * *"
push:
branches:
- add-peft-bnb-source-docker
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to remove after testing

@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.

@younesbelkada
Copy link
Contributor Author

I can confirm the built image points to the correct commit hash: https://github.com/huggingface/peft/actions/runs/7224973902/job/19687439574?pr=1275#step:6:6528 on bnb main

@younesbelkada younesbelkada marked this pull request as ready for review December 15, 2023 17:27
@younesbelkada
Copy link
Contributor Author

The PR is ready for review, you can find the freshly built docker images here and here

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.

Looks pretty good, thanks. There are a few minor things that could be done to improve the Docker image caching, but no blocker IMO.

name: "Latest Peft GPU + bnb source [dev]"
runs-on: ubuntu-latest
steps:
- name: Cleanup disk
Copy link
Member

Choose a reason for hiding this comment

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

I've never seen this before in a workflow file :O

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I've copied that from transformers since I used to get some disk issues: https://github.com/huggingface/transformers/blob/main/.github/workflows/build-docker-images.yml#L25

Comment on lines 59 to 65
RUN source activate peft && \
git clone https://github.com/TimDettmers/bitsandbytes && cd bitsandbytes && \
CUDA_VERSION=121 make cuda12x && \
python setup.py develop

RUN source activate peft && \
pip freeze | grep bitsandbytes
Copy link
Member

Choose a reason for hiding this comment

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

Steps like these could be combined to a single one to reduce the footprint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now refactored everthing based on your suggestions !

RUN chsh -s /bin/bash
SHELL ["/bin/bash", "-c"]
RUN source activate peft && \
python3 -m pip install --no-cache-dir optimum auto-gptq
Copy link
Member

Choose a reason for hiding this comment

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

Why a separate install of these packages here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

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.

Nice.

Copy link
Contributor

@pacman100 pacman100 left a comment

Choose a reason for hiding this comment

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

Thank you @younesbelkada, LGTM! 🚀

@younesbelkada younesbelkada merged commit 119de17 into main Dec 18, 2023
@younesbelkada younesbelkada deleted the add-peft-bnb-source-docker branch December 18, 2023 14:15
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.

4 participants