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: add FreeBSD & simplify CUDA windows #3053

Merged
merged 9 commits into from Sep 14, 2023

Conversation

alonfaraj
Copy link
Contributor

@alonfaraj alonfaraj commented Sep 7, 2023

Lllama.cpp allegedly officially support FreeBSD as stated in the README, I think it recommended to cover it in the CI.

This PR:

  • Add FreeBSD to CI
  • Simplify and unify the "Copy and pack Cuda runtime" step to be identical for all CUDA versions
  • Install only necessary CUDA sub packages
  • bump actions/checkout usages to v3

@ggerganov I'm wondering if we should move forward to the latest CUDA - 12.2?


- uses: Jimver/cuda-toolkit@v0.2.10
id: cuda-toolkit
with:
cuda: ${{ matrix.cuda }}
# TODO(green-sky): _dev seems to fail, and non dev are not enought
#sub-packages: '["nvcc", "cudart", "cublas", "cudart_dev", "cublas_dev"]'
sub-packages: '["nvcc", "cudart", "cublas", "cublas_dev", "thrust", "visual_studio_integration"]'
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you get this to work? also, can you create a manual release for this pr in your repo?

Copy link
Contributor Author

@alonfaraj alonfaraj Sep 10, 2023

Choose a reason for hiding this comment

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

The build passed successfully.
I created a release:
https://github.com/alonfaraj/llama.cpp/releases
I just fixed the folder name and will check it when it done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh god, the cuda runtime for windows grew another 40mb

@@ -413,6 +396,22 @@ jobs:
path: |
cudart-llama-bin-win-cu${{ matrix.cuda }}-x64.zip

freeBSD-latest:
runs-on: macos-12
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it run on macos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The documentation specify the latest supported macOS version is 12.
https://github.com/cross-platform-actions/action#runners

But it also support Linux, I guess we can try Ubuntu latest.
Do you think it worth change to linux?

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, it should be fine, since this is an PUBLIC opensource project. If it where PRIVATE, we would have to pay 5x the compute minutes. 😆

@Green-Sky
Copy link
Collaborator

I'm wondering if we should move forward to the latest CUDA - 12.2?

i very recently (1 month ago~) got an upgrade to nvidia 525, which provides 12.0 . before that i was on 515 (iirc) which only provided 11.7 . I'm still on ubuntu 20.04 LTS, which is often used on servers. so yea, maybe move to 12.2, but i suggest to also keep the 11.7 around for some time longer.

... also, I should really rework and update my pr to provide linux binaries.

@alonfaraj
Copy link
Contributor Author

alonfaraj commented Sep 10, 2023

so yea, maybe move to 12.2, but i suggest to also keep the 11.7 around for some time longer.

Yup, that's what I meant.
Have latest CUDA 12.x version, and maybe even latest 11.x which is 11.8, unless it better staying with 7.11 for some reason.

@alonfaraj
Copy link
Contributor Author

alonfaraj commented Sep 10, 2023

Also, is there a reason we trigger the CI only for specific paths?
here and here

Why not enable it for everything?
For example - a PR like this one, which change the build workflow not trigger the workflow itself..

@Green-Sky
Copy link
Collaborator

Also, is there a reason we trigger the CI only for specific paths? here and here

Why not enable it for everything? For example - a PR like this one, which change the build workflow not trigger the workflow itself..

Not all changes are code changes. Also, yea, it is somewhat annoying, but it also is supposed to protect from arbitrary code execution (somewhat).

Copy link
Owner

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

@Green-Sky Merge if you think it is OK

@Green-Sky Green-Sky merged commit 83a53b7 into ggerganov:master Sep 14, 2023
5 checks passed
@Green-Sky Green-Sky changed the title CI: add FreeBSD & simplify CUDA windwos CI: add FreeBSD & simplify CUDA windows Sep 14, 2023
@Green-Sky
Copy link
Collaborator

Green-Sky commented Sep 14, 2023

looks like the long running windows cuda builds are currently choking the ci. hope this pr improves it.

(despite adding one extra task 😄 )

@Green-Sky
Copy link
Collaborator

@alonfaraj the freebsd ci runner is getting stuck in/after build sometimes for some reason.

@Green-Sky
Copy link
Collaborator

see eg https://github.com/ggerganov/llama.cpp/actions/runs/6226790431/job/16900055948

i suggest we implement a global timeout/limit for up to 1h as a save guard.

@alonfaraj alonfaraj mentioned this pull request Sep 19, 2023
pkrmf pushed a commit to morlockstudios-com/llama.cpp that referenced this pull request Sep 26, 2023
* add freebsd to ci

* bump actions/checkout to v3
* bump cuda 12.1.0 -> 12.2.0
* bump Jimver/cuda-toolkit version

* unify and simplify "Copy and pack Cuda runtime"
* install only necessary cuda sub packages
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.

None yet

3 participants