-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Add lora test workflow (WIP) #9058
Add lora test workflow (WIP) #9058
Conversation
6e1237e
to
1413c49
Compare
-DLLAMA_CURL=ON \ | ||
-DCMAKE_BUILD_TYPE=${{ matrix.build_type }} \ | ||
-DLLAMA_SANITIZE_${{ matrix.sanitizer }}=ON ; | ||
cmake --build build --config ${{ matrix.build_type }} -j $(nproc) --target llama-server |
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.
Should I remove this line and the cmake
dependency above?
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.
Or should I use it to add lora tests that involve the server?
Co-authored-by: Xuan Son Nguyen <[email protected]>
Co-authored-by: Xuan Son Nguyen <[email protected]>
ce6dd7a
to
42f65b0
Compare
7e64e01
to
fc6abde
Compare
@ngxson Hey 😃 ! Are you able to take a look a this PR next week? |
Sorry for the delay. This is not my current priority, but I'll see if I have time next week. |
@ngxson do you have time to look at this, or can you point me to another maintainer that can help here? |
I think we don't really need this anymore, as this is not a very important part and running test script locally works fine in most cases (we don't have much changes related to lora anyway) We also don't want to add too much workflows to the CI. Every 1-2 minutes added to the CI will make it slow overtime, let alone the build step of this script that can easily take 5 minutes of the CI. So I think it's better not to continue implementing this for now. |
@ngxson Yes no problem! Should we merge only the modification in the test script to clone from a specific commit, so there is not risk in with updates to the HF test models repo? (See updated diff) |
Yes that looks good. Btw next time you can use add a new PR (if you want). The current PR can be kept as draft for reference in the future. For open-source projects, it's common to close a PR or to keep a non-functioning PR as draft. |
Yes, let's do that! Closed as not planning to implement the CI workflow |
Context
Partially solving Issue #8708.
Running the tests introduced in PR #8957 in a new git workflow (inspired by
.github/workflows/server.yml
).Todo
Test if workflow runs successfully.Should I add a test for the feature in server : add lora hotswap endpoint #8857 too?Do we want to refactor test models storage in HF to 1 architecture per model repo? This is an example of HuggingFace approach.Checks