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

tests: disable GGUF test for bad value size #10886

Merged

Conversation

JohannesGaessler
Copy link
Collaborator

See #10830 (comment) .

This PR makes the test failure with a bad value size deterministic (as in independent of the seed). The way this test causes a failure is not really well-defined since it just writes a random wrong number of wrong, random bytes to the file. So depending on the RNG you can get a random, uncaught failure in a calloc call. This PR makes the test failure deterministic to prevent false positives in the CI until I have the C++ refactor ready.

@ggerganov where in llama.cpp is the sanitizer logic defined? I usually use Valgrind for my local development but I'd like to assert that there aren't any more random failures with the method used in the CI.

@ggerganov
Copy link
Owner

You can enable sanitizers like this:

# address + undefined sanitizers enabled
cmake [...] -DLLAMA_SANITIZE_ADDRESS=ON -DLLAMA_SANITIZE_UNDEFINED=ON

# thread sanitizer enabled (this one is not compatible with the address sanitizer)
cmake [...] -DLLAMA_SANITIZE_THREAD=ON

@JohannesGaessler
Copy link
Collaborator Author

It seems that this problem is caused by the allocation attempt exceeding the maximum allowed allocation size of the sanitizer (1 TiB) but with a size which is still below the threshold of what could possibly be allocated given the address space (this case can be detected prior to allocation). When running the code in production what should happen is that the allocation will fail due to hardware constraints, this is then detected as an error and NULL is returned. So I think for now we should just disable this test and I'll look into how to fix it for the C++ refactor.

@JohannesGaessler JohannesGaessler changed the title tests: make bad GGUF value size test deterministic tests: disable GGUF test for bad value size Dec 18, 2024
@github-actions github-actions bot added the testing Everything test related label Dec 18, 2024
@JohannesGaessler JohannesGaessler merged commit cd920d0 into ggerganov:master Dec 19, 2024
48 checks passed
@ymcki
Copy link
Contributor

ymcki commented Dec 19, 2024

Thanks for the fix. I too got the mysterious calloc crash during test-gguf

arthw pushed a commit to arthw/llama.cpp that referenced this pull request Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Everything test related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants