[CI] Update actions/upload-artifact to v4

[matthewfeickert]

Types of changes

• actions/upload-artifact v3 will be deprecated as of 2024-12-05 and will result in workflow failures afterwards.
• Use unique names to avoid overwrite.

Motivation and context / Related issue

I received this email from GitHub which applies to POT:

Artifact actions v3 will be deprecated by December 5, 2024. You are receiving this email because you have GitHub Actions workflows using v3 of actions/upload-artifact or actions/download-artifact. After this date using v3 of these actions will result in a workflow failure. Artifacts within their retention period will remain accessible from the UI or REST API regardless of the version used to upload.

To raise awareness of the upcoming removal, we will temporarily fail jobs using v3 of actions/upload-artifact or actions/download-artifact. Builds that are scheduled to run during the brownout periods will fail. The brownouts are scheduled for the following dates and times:

• November 14, 12pm - 1pm EST
• November 21, 9am - 5pm EST

What you need to do
Update workflows to begin using v4 of the artifact actions as soon as possible. While v4 improves upload and download speeds by up to 98% and includes several new features, there are key differences from previous versions that may require updates to your workflows. Please see the documentation in the project repositories for guidance on how to migrate your workflows.

How has this been tested (if it applies)

PR checklist

• I have read the CONTRIBUTING document.
• [N/A] The documentation is up-to-date with the changes I made (check build artifacts).
• [N/A] All tests passed, and additional code has been covered with new tests.
• [N/A] I have added the PR and Issue fix to the RELEASES.md file.

[matthewfeickert]

@cedricvincentcuaz (tagging you given bd56809) this is ready for review.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.73%. Comparing base (38922c0) to head (9e96ec3).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #684      +/-   ##
==========================================
- Coverage   97.03%   96.73%   -0.30%     
==========================================
  Files          98       98              
  Lines       19639    19639              
==========================================
- Hits        19056    18998      -58     
- Misses        583      641      +58     

[rflamary]

Thanks for the PR

Small comment and we need to run the build wheels before merging

[rflamary]

maybe matrix.os instead of job-index is easier to parse?

Also could you please commit with message 'build wheels' to trigger the build and check that they run properly?

[matthewfeickert]

Ah, yes I remember why this pattern is needed now. If you just do matrix.os this isn't sufficiently unique in .github/workflows/build_wheels.yml as both

POT/.github/workflows/build_wheels.yml

Lines 11 to 18 in 38922c0

	jobs:
	build_wheels:
	name: ${{ matrix.os }}
	runs-on: ${{ matrix.os }}
	if: "contains(github.event.head_commit.message, 'build wheels')"
	strategy:
	matrix:
	os: [ubuntu-latest, macos-latest, windows-latest]

and

POT/.github/workflows/build_wheels.yml

Lines 47 to 53 in 38922c0

	build_all_wheels:
	name: ${{ matrix.os }}
	runs-on: ${{ matrix.os }}
	if: "contains(github.event.head_commit.message, 'build all wheels')"
	strategy:
	matrix:
	os: [ubuntu-latest, macos-latest, windows-latest]

will have the same name and so will result in errors of

Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run

So you need something more unique. In boost-histogram we use wheels-${{ strategy.job-index }} and it works well.

[rflamary]

I see. on the other hand both build wheels and build all wheels should not run simultaneously because of the condition. I don't know how you managed that ;)

Also the only one that run on master after a merge is in build_wheels_weekly and this one do not have this kinf of problems. Let us use job-index in build_wheels to avoid the problem and matrix.ox in the weeky file (that also run for all merge on master) so that we have easier to handle filenames when releasing maybe?

[rflamary]

haha clearly my trigger does not work... (that another thing to debug ;) )

Otherwise LGTM. We can probably merge and see what happens (this does not impact the POT code anyways)

[matthewfeickert]

haha clearly my trigger does not work... (that another thing to debug ;) )

Otherwise LGTM. We can probably merge and see what happens (this does not impact the POT code anyways)

Just to make sure that I don't accidentally break your code because of something stupid I did let me build the wheels on a different branch on my fork and report back.

edit: I will go offline before these finish, but I've started a run over on https://github.com/matthewfeickert/POT/actions/runs/11699691668. If these pass, I'll rebase and squash these commits into a single one and then ping for review again.

[matthewfeickert]

(Sorry for going AFK for a bit — other work distractions came up). @rflamary @cedricvincentcuaz if these changes introduce any pain points or confusion later let me know and I can try to help mitigate this.

[cedricvincentcuaz]

Thank you @matthewfeickert for your PR, everything went smoothly during the merge so no worries :)