[bnb-nightly] Address final comments
#1287
Conversation
| @@ -25,7 +25,7 @@ jobs: | |||
| 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 | |||
| options: --gpus all --shm-size "16gb" --ipc host -v /mnt/cache/.cache/huggingface:/mnt/cache/ | |||
There was a problem hiding this comment.
the env variable NVIDIA_DISABLE_REQUIRE is set on the very top so these are not needed
| @@ -39,16 +39,19 @@ jobs: | |||
| mkdir transformers-clone && git clone https://github.com/huggingface/transformers.git transformers-clone # rename to transformers clone to avoid modules conflict | |||
|
|
|||
| - name: Run examples on single GPU | |||
| if: always() | |||
There was a problem hiding this comment.
@ydshieh I thought the fail-fast: false already takes care of running the tests even if they fail, do you know why here we need to put always() ?
There was a problem hiding this comment.
The fail-fast: (under jobs.<job_id>.strategy) applies to the jobs in the defined matrix. But here each job in the matrix has 3 testing steps in each job, but those steps won't be controlled by jobs.<job_id>.strategy.
That is my understanding
There was a problem hiding this comment.
See this example
https://github.com/huggingface/transformers/actions/runs/7286009365/job/19853943708
all 3 jobs (in the matrix) are run. But the 3rd step in each job are not.
|
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
changed the title
bnb-nightly] Address final comments
| run: | | ||
| source activate peft | ||
| make tests_examples_single_gpu_bnb | ||
|
|
||
| - name: Run core tests on single GPU | ||
| if: always() |
There was a problem hiding this comment.
This is the quick and simple way. Just want to point out:
if: always()will make the step uncancellable. A better approach would beif: !cancelled()
There was a problem hiding this comment.
But I believe we only want to run the 3 tests only if the step Pip install succeed. The condition gets a bit more complex in this case. OK for me if you are happy to keep the current version.
There was a problem hiding this comment.
Perfect thanks! I will stick to the current plan then :D
|
Thanks a lot for your review @ydshieh ! |
What does this PR do?
Address final comments from @ydshieh on. #1282 (review)
I left few questions !
cc @ydshieh