KEMBAR78
[CI] updates to the CI report naming, and `accelerate` installation by sayakpaul · Pull Request #9429 · huggingface/diffusers · GitHub
Skip to content

Conversation

@sayakpaul
Copy link
Member

@sayakpaul sayakpaul commented Sep 12, 2024

What does this PR do?

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.

@sayakpaul sayakpaul marked this pull request as draft September 12, 2024 15:30
@sayakpaul sayakpaul removed the request for review from yiyixuxu September 12, 2024 15:30
@sayakpaul sayakpaul changed the title [Tests] chore: id accordingly to avoid duplicates. [CI] updates to the CI report naming, and accelerate installation Sep 13, 2024
uses: actions/upload-artifact@v4
with:
name: ${{ matrix.config.report }}_test_reports
name: tests_onnx_cuda_reports
Copy link
Member Author

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 }} \
Copy link
Member Author

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.report because there's nothing called config in the matrix.
  • Adds lib-version (which is in the matrix) 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
Copy link
Member Author

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
Copy link
Member Author

Choose a reason for hiding this comment

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

To preserve uniqueness.

Copy link
Collaborator

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

@sayakpaul sayakpaul marked this pull request as ready for review September 13, 2024 04:00
@sayakpaul
Copy link
Member Author

@yiyixuxu can you give this a review?

Copy link
Collaborator

@yiyixuxu yiyixuxu left a 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
Copy link
Collaborator

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 yiyixuxu merged commit 5440cbd into main Sep 16, 2024
10 checks passed
@yiyixuxu yiyixuxu deleted the avoid-duplicate-artifact-ids branch September 16, 2024 21:29
leisuzz pushed a commit to leisuzz/diffusers that referenced this pull request Oct 11, 2024
…uggingface#9429)

* chore: id accordingly to avoid duplicates.

* update properly.

* updates

* updates

* empty

* updates

* changing order helps?
sayakpaul added a commit that referenced this pull request Dec 23, 2024
…9429)

* chore: id accordingly to avoid duplicates.

* update properly.

* updates

* updates

* empty

* updates

* changing order helps?
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.

3 participants