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

CI maintenance: Update checkout, setup-python and upload-artifact to latest versions #6456

Merged
merged 3 commits into from
Apr 3, 2024

Conversation

EwoutH
Copy link
Contributor

@EwoutH EwoutH commented Apr 3, 2024

A bit of CI maintenance, which updates the following actions to their latest versions:

For each, the main change is an update from Node 16 LTS to Node 20 LTS.

If preferred, I can add a Dependabot configuration which opens these PRs automatically in the future (in another PR).

@phymbert
Copy link
Collaborator

phymbert commented Apr 3, 2024

Thanks, have you tested on your fork that it works ? If yes please send links

@EwoutH
Copy link
Contributor Author

EwoutH commented Apr 3, 2024

@phymbert here are the runs on my fork: EwoutH#1

@phymbert
Copy link
Collaborator

phymbert commented Apr 3, 2024

@EwoutH
Copy link
Contributor Author

EwoutH commented Apr 3, 2024

Right, missed that one, triggered it here: https://github.com/EwoutH/llama.cpp/actions/runs/8539946210

@phymbert
Copy link
Collaborator

phymbert commented Apr 3, 2024

I am prudent because the way the repo is cloned has an impact on the build-info.o object.

@EwoutH
Copy link
Contributor Author

EwoutH commented Apr 3, 2024

I understand.

All the runs have passed successfully.

Copy link
Collaborator

@phymbert phymbert 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 having looking at this, please wait for @ggerganov approval

@phymbert phymbert requested a review from ggerganov April 3, 2024 17:58
@ggerganov ggerganov merged commit 9f62c01 into ggerganov:master Apr 3, 2024
0 of 8 checks passed
@EwoutH
Copy link
Contributor Author

EwoutH commented Apr 3, 2024

Thanks for merging!

Would a Dependabot configuration be useful, which opens such PRs automatically in the future?

@phymbert
Copy link
Collaborator

phymbert commented Apr 4, 2024

@ggerganov @EwoutH I guess it blocks master as now artifact name are duplicated:
https://github.com/ggerganov/llama.cpp/actions/runs/8551108117/job/23429429188

I suggest to revert and wait for a solution

@EwoutH
Copy link
Contributor Author

EwoutH commented Apr 4, 2024

Give me a sec, looking into it: https://github.com/actions/upload-artifact/blob/main/docs/MIGRATION.md#migration

If I understand correctly it just requires naming artifacts differently.

@EwoutH
Copy link
Contributor Author

EwoutH commented Apr 4, 2024

Basically there are two options:

  1. Keeping the single artifact artifact, with all different ZIP files (AvX, AVX2, Arm, etc.) in that single file. This requires adding a merge step at the end of the run.
  2. Upload each generated ZIP as it’s own artifact (so we would get artifact_avx, artifact_avx2, etc.

What’s preferred?

@phymbert
Copy link
Collaborator

phymbert commented Apr 4, 2024

Let's keep it simple, one artifact per job run

@ggerganov
Copy link
Owner

I don't have a preference and I'm not sure what is useful for most people. We can do the one artifact per job as suggested and if we see it is not optimal for some reason we can update later

@phymbert
Copy link
Collaborator

phymbert commented Apr 4, 2024

I doubt people are using it. Previously, it was merged:
https://github.com/ggerganov/llama.cpp/actions/runs/8541354320

But IMHO, separating per backend is easier than downloading a 1GB file. As you say, we can improve later on if it hurts for some reason.

@EwoutH
Copy link
Contributor Author

EwoutH commented Apr 4, 2024

#6482 should solve the artifact naming conflict. It will upload multiple smaller artifacts instead of one big one, which I think is more useful for most users and developers.

tybalex pushed a commit to rubra-ai/tools.cpp that referenced this pull request Apr 17, 2024
…rganov#6456)

* CI: Update actions/checkout to v4

* CI: Update actions/setup-python to v5

* CI: Update actions/upload-artifact to v4
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