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

[bnb] Add bnb nightly workflow #1282

Merged
merged 28 commits into from
Dec 20, 2023
Merged

[bnb] Add bnb nightly workflow #1282

merged 28 commits into from
Dec 20, 2023

Conversation

younesbelkada
Copy link
Contributor

As per title, this PR adds the possibility to run peft tests on bitsandbytes compiled from source. It is a follow-up work from #1275

@younesbelkada
Copy link
Contributor Author

cc @Titus-von-Koeller FYI

on:
push:
branches:
- add-bnb-tests
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

@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

Tests are all passing! https://github.com/huggingface/peft/actions/runs/7263175979/job/19787891361
This PR is ready for review

@younesbelkada younesbelkada marked this pull request as ready for review December 19, 2023 15:04
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 adding the bnb tests to the CI. I left a couple of comments, please check if they make sense.

.github/workflows/nightly-bnb.yml Outdated Show resolved Hide resolved
.github/workflows/nightly-bnb.yml Outdated Show resolved Hide resolved
.github/workflows/nightly.yml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
tests_core_single_gpu_bnb:
python -m pytest -m "single_gpu_tests and bitsandbytes" tests/test_common_gpu.py $(if $(IS_GITHUB_CI),--report-log "core_single_gpu.log",)

transformers_tests:
Copy link
Member

Choose a reason for hiding this comment

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

Why are we running this in PEFT? Just out of convenience or are these tests PEFT-related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is just for convenience, I can add a comment to explain the intention

scripts/log_reports.py Outdated Show resolved Hide resolved
"emoji": True
}
}
passed = []
Copy link
Member

Choose a reason for hiding this comment

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

Since I'm lazy: Did you change any of the code in the main function, or is it just indented and the rest is the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No no, I just idented it and the rest is all same

Makefile Show resolved Hide resolved
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 for adding the github workflow that runs everyday to make sure BNB integration in Transformers & PEFT are all good! LGTM 🚀.

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, LGTM

@younesbelkada younesbelkada merged commit 029dcd5 into main Dec 20, 2023
14 checks passed
@younesbelkada younesbelkada deleted the add-bnb-tests branch December 20, 2023 09:49
TEST_TYPE: "single_gpu_${{ matrix.docker-image-name }}"
container:
image: ${{ matrix.docker-image-name }}
options: --gpus all --shm-size "16gb" --ipc host -v /mnt/cache/.cache/huggingface:/mnt/cache/ -e NVIDIA_DISABLE_REQUIRE=true
Copy link

Choose a reason for hiding this comment

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

We should use the value from NVIDIA_DISABLE_REQUIRE defined at the top. If we want to specify it explicitly in each command, then we don't need the one on the top?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah correct, i think I can remove it indeed

source activate peft
make tests_examples_single_gpu_bnb

- name: Run core tests on single GPU
Copy link

Choose a reason for hiding this comment

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

this won't run if the previous step fails. Do we want it this way?

source activate peft
make tests_core_single_gpu_bnb

- name: Run transformers tests on single GPU
Copy link

Choose a reason for hiding this comment

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

same above

make transformers_tests

- name: Generate Report
if: always()
Copy link

Choose a reason for hiding this comment

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

good

@ydshieh
Copy link

ydshieh commented Dec 20, 2023

Just a few tiny nits. And if I understand correctly, this new CI doesn't have an experiment run yet (before merge) 🤫.

@younesbelkada
Copy link
Contributor Author

Thanks for reviewing @ydshieh !
I made a follow up PR in #1287

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.

5 participants