Skip to content

Overhaul ci/run.sh and enable tests with openllama model files #4586

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Jan 26, 2024
Merged

Overhaul ci/run.sh and enable tests with openllama model files #4586

merged 21 commits into from
Jan 26, 2024

Conversation

crasm
Copy link
Contributor

@crasm crasm commented Dec 22, 2023 •
edited
Loading

The existing ci/run.sh script is currently inconvenient to use locally, has lots of repetitive logic (that does not otherwise improve clarity), compiles llama.cpp more times than strictly necessary, and requires manual cleanup between runs.

This PR aims to address the above, improve readability and maintainability, and make it possible to run ctests against the downloaded openllama model.

@crasm crasm added the testing Everything test related label Dec 22, 2023
@crasm crasm self-assigned this Jan 18, 2024
@crasm
Copy link
Contributor Author

crasm commented Jan 18, 2024 •
edited
Loading

@ggerganov Can you take a look at the stub ci-run.sh script? Just want to make sure this approach is OK with you before I continue in earnest.

I think directly specifying what gets run will ultimately end up being easier to reason about than using flags for VRAM_GB, LOW_PERFORMANCE, etc.

@crasm
Copy link
Contributor Author

crasm commented Jan 18, 2024

(previous branch)

crasm added 2 commits January 18, 2024 18:21
@crasm
scripts : switch to PascalCase for functions
a34648d 
This looks a little odd at first, but I find it very useful as a
convention to know if a command is part of our code vs a builtin.
@crasm
scripts : add some fancy conversion from snake_case to PascalCase
9a11611
@ggerganov ggerganov self-requested a review January 19, 2024 12:13
@ggerganov
Copy link
Member

Hm, not sure about the last 2 commits - I can see the intent, but we already have some Bash code that I don't plan to change. Use the style in https://github.com/ggml-org/ci as a reference. It can be improved with small steps when necessary

Will continue to look at the other changes

@ggerganov
Copy link
Member

I think for now we should focus on just adding more tests to the existing ci/run.sh. I'm happy with it's functionality atm and I feel if we start refactoring it, it will be much effort for not a lot of gains.

The repetitive builds can be largely mitigated with the incoming ccache changes: #5002

@crasm crasm marked this pull request as ready for review January 20, 2024 19:50
@crasm
Copy link
Contributor Author

crasm commented Jan 21, 2024

@ggerganov this is ready. Unfortunately, I'm adding to the copy-paste structure in ci/run.sh rather than reducing it. However, it does work.

My biggest issues were high IO usage (SSD wear) and the need for manual cleanup after running it. I addressed this by wrapping it with a convenience script that isolates it to a separate directory, ideally on a ramdisk, and stores the persistent model/wikitext/etc files in ~/.cache/llama.cpp.

ggerganov
ggerganov approved these changes Jan 22, 2024
View reviewed changes
Copy link
Member

@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.

It seems that that running ctest without a label would fail. I guess it's fine, but would be nice to workaround somehow

@crasm
Copy link
Contributor Author

crasm commented Jan 23, 2024 •
edited
Loading

It seems that that running ctest without a label would fail. I guess it's fine, but would be nice to workaround somehow

If you just mean for the ergonomics of running ctest in terminal, exporting GG_RUN_CTEST_MODELFILE in e.g. ~/.bashrc would suffice.

(This actually makes me realize that we don't need to check if we're running in make. We just need to check if the env variable is set or not. Then the model tests will work in make!)

@crasm crasm force-pushed the revamp-ci-run-sh branch 2 times, most recently from 201a110 to 3e4871a Compare January 23, 2024 21:34
crasm added 2 commits January 23, 2024 16:35
@crasm
Fixes
25f1d79 
* GG_RUN_CTEST_MODELFILE => LLAMACPP_TESTMODELFILE
* Always show warning rather than failing if model file variable is not
  set
@crasm
scripts : update usage text for ci-run.sh
62ca49e
@crasm
Copy link
Contributor Author

crasm commented Jan 23, 2024

@ggerganov I've changed it so that the test outputs a warning if the LLAMACPP_TEST_MODELFILE variable is not set, rather than failing. Make and ctest will output warnings if run without that variable, and setting that variable in your local environment will allow those tests to run.

This shouldn't pose an issue for usage in ci/run.sh ...unless some bug is introduced later where the environment variable isn't set properly and nobody pays attention to the warning.

@crasm
Copy link
Contributor Author

crasm commented Jan 25, 2024

If there are no objections or concerns, I will merge this tomorrow

@ggerganov ggerganov merged commit 413e7b0 into master Jan 26, 2024
jordankanter pushed a commit to jordankanter/llama.cpp that referenced this pull request Feb 3, 2024
@crasm @jordankanter
ci : add model tests + script wrapper (ggml-org#4586)
3d3a0b2 
* scripts : add lib.sh and lib_test.sh

* scripts : stub out new ci-run.sh script

* scripts : switch to PascalCase for functions

This looks a little odd at first, but I find it very useful as a
convention to know if a command is part of our code vs a builtin.

* scripts : add some fancy conversion from snake_case to PascalCase

* Add venv to ci/run.sh

* Revert scripts work

* scripts : add wrapper script for local use of ci/run.sh

* Simplify .gitignore for tests, clang-tidy fixes

* Label all ctest tests

* ci : ctest uses -L main

* Attempt at writing ctest_with_model

* Update test-model-load-cancel

* ci : add ctest_with_model for debug and release

ggml-ci

* Fix gg_get_model function

ggml-ci

* got stuck on CMake

* Add get_model.cpp to tests/CMakeLists.txt

ggml-ci

* Fix README.md output for ctest_with_model

ggml-ci

* workflows : use `-L main` for all ctest

ggml-ci

* Fixes

* GG_RUN_CTEST_MODELFILE => LLAMACPP_TESTMODELFILE
* Always show warning rather than failing if model file variable is not
  set

* scripts : update usage text for ci-run.sh
@crasm crasm deleted the revamp-ci-run-sh branch March 1, 2024 21:56
hodlen pushed a commit to hodlen/llama.cpp that referenced this pull request Apr 1, 2024
@crasm @hodlen
ci : add model tests + script wrapper (ggml-org#4586)
b5c088a 
* scripts : add lib.sh and lib_test.sh

* scripts : stub out new ci-run.sh script

* scripts : switch to PascalCase for functions

This looks a little odd at first, but I find it very useful as a
convention to know if a command is part of our code vs a builtin.

* scripts : add some fancy conversion from snake_case to PascalCase

* Add venv to ci/run.sh

* Revert scripts work

* scripts : add wrapper script for local use of ci/run.sh

* Simplify .gitignore for tests, clang-tidy fixes

* Label all ctest tests

* ci : ctest uses -L main

* Attempt at writing ctest_with_model

* Update test-model-load-cancel

* ci : add ctest_with_model for debug and release

ggml-ci

* Fix gg_get_model function

ggml-ci

* got stuck on CMake

* Add get_model.cpp to tests/CMakeLists.txt

ggml-ci

* Fix README.md output for ctest_with_model

ggml-ci

* workflows : use `-L main` for all ctest

ggml-ci

* Fixes

* GG_RUN_CTEST_MODELFILE => LLAMACPP_TESTMODELFILE
* Always show warning rather than failing if model file variable is not
  set

* scripts : update usage text for ci-run.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggerganov ggerganov ggerganov approved these changes

Assignees

@crasm crasm

Labels
testing Everything test related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@crasm @ggerganov