-
Notifications
You must be signed in to change notification settings - Fork 6.4k
[CI] updates to the CI report naming, and accelerate installation
#9429
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
Conversation
accelerate installation
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: ${{ matrix.config.report }}_test_reports | ||
| name: tests_onnx_cuda_reports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we don't have a matrix here.
| python -m pytest -n 4 --max-worker-restart=0 --dist=loadfile \ | ||
| -s -v \ | ||
| --make-reports=tests_${{ matrix.config.report }} \ | ||
| --make-reports=tests_${{ matrix.lib-versions }} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change:
- Removes the faulty
matrix.config.reportbecause there's nothing called config in the matrix. - Adds
lib-version(which is in thematrix) to make the names unique.
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: pr_${{ matrix.config.report }}_test_reports | ||
| name: pr_${{ matrix.lib-versions }}_test_reports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that the artifact is uploaded with a unique name.
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: pr_${{ matrix.config.report }}_test_reports | ||
| name: pr_${{ matrix.config.framework }}_${{ matrix.config.report }}_test_reports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To preserve uniqueness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one seems to be ok - config.report already named nicely with framework taken into account
|
@yiyixuxu can you give this a review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: pr_${{ matrix.config.report }}_test_reports | ||
| name: pr_${{ matrix.config.framework }}_${{ matrix.config.report }}_test_reports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one seems to be ok - config.report already named nicely with framework taken into account
…uggingface#9429) * chore: id accordingly to avoid duplicates. * update properly. * updates * updates * empty * updates * changing order helps?
…9429) * chore: id accordingly to avoid duplicates. * update properly. * updates * updates * empty * updates * changing order helps?
What does this PR do?
accelerateis updated properly when it's installed from the source. Can confirm that is resolve the bugs we where seeing in_get_named_parameters()seems to have met a regression accelerate#3087.Running https://github.com/huggingface/diffusers/actions/runs/10843159254/job/30089905695 to see things went alright. The purpose of the run is to make sure the artifacts get correctly uploaded.