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: add tests for GGUF #10830

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JohannesGaessler
Copy link
Collaborator

I'm currently working on a C++ refactor of the GGUF code, see discussion in #10655 . I am done with the majority of the implementation. Since GGUF is a critical component I think the correct way to go about a refactor is to write tests that assert that the behavior does not change (tests are of course useful anyways). This PR adds said tests ahead of time and defines the behavior we want the refactored code to have. To assert the correctness of the GGUF code there are three different test scenarios:

  1. A handwritten file is produced and read in with the GGUF code. It is asserted that bad inputs lead to failure and return NULL.
  2. A random GGUF context is created in program code, written to a file, and read back in. It is asserted that the contexts are equivalent.
  3. Random and empty GGUF contexts are created and the KV data is set with gguf_set_kv.

The following issues currently exist on master:

  • A GGUF file with a higher version than the current code is not an error.
  • Duplicate KV keys in a GGUF file is not an error.
  • Tensor offsets outside the file are not an error.
  • Misaligned tensor offsets in a file are not an error.
  • If the end of the file is not reached that is not treated as an error (I'm aware of only_meta).
  • If the tensors are not in RAM the GGUF write is segfaulting.

The corresponding test cases are commented out (marked with FIXME).

To make the tests work I'm exposing some internals of the GGUF code in ggml-impl.h. For the temporary files I'm using tmpfile() which returns a temporary FILE pointer that is valid until closed. Unfortunately this seems to require elevated privileges on Windows. For now I'm just skipping the tests on Windows if tmpfile() fails, with the C++ code it would I think be possible to use a stringstream to simulate file I/O without having to touch the actual file system (the externally visible GGUF interface only passes file names).

@github-actions github-actions bot added testing Everything test related ggml changes relating to the ggml tensor library for machine learning labels Dec 14, 2024
@JohannesGaessler
Copy link
Collaborator Author

I forgot: the current master code also has some (minor) memory leaks related to KV arrays.

A GGUF file with a higher version than the current code is not an error.

Actually, how is GGUF_VERSION supposed to be interpreted? Should a GGUF file from the future result in an error?

@JohannesGaessler
Copy link
Collaborator Author

The sanitizer CI failures are expected since the test code is using some of the GGUF code that has memory leaks (so the tests should be disabled prior to merging and reenabled after they are fixed). I don't understand why the linker is failing on Windows.

@slaren
Copy link
Collaborator

slaren commented Dec 14, 2024

On Windows, you need to specify what symbols are to be exported in a DLL. To do that, you need to use GGML_API in the declaration of the functions that are used outside of ggml.


2024-12-14T15:57:11.6958312Z test-gguf.obj : error LNK2019: unresolved external symbol gguf_type_size referenced in function "bool __cdecl all_kv_in_other(struct gguf_context const *,struct gguf_context const *)" (?all_kv_in_other@@YA_NPEBUgguf_context@@0@Z) [D:\a\llama.cpp\llama.cpp\build\tests\test-gguf.vcxproj]
2024-12-14T15:57:11.6962570Z test-gguf.obj : error LNK2019: unresolved external symbol gguf_init_from_file_impl referenced in function "struct std::pair<int,int> __cdecl test_handcrafted_file(unsigned int)" (?test_handcrafted_file@@YA?AU?$pair@HH@std@@I@Z) [D:\a\llama.cpp\llama.cpp\build\tests\test-gguf.vcxproj]
2024-12-14T15:57:11.6967373Z test-gguf.obj : error LNK2019: unresolved external symbol gguf_buf_init referenced in function "struct std::pair<int,int> __cdecl test_roundtrip(struct ggml_backend_device *,unsigned int,bool)" (?test_roundtrip@@YA?AU?$pair@HH@std@@PEAUggml_backend_device@@I_N@Z) [D:\a\llama.cpp\llama.cpp\build\tests\test-gguf.vcxproj]
2024-12-14T15:57:11.6972974Z test-gguf.obj : error LNK2019: unresolved external symbol gguf_buf_free referenced in function "struct std::pair<int,int> __cdecl test_roundtrip(struct ggml_backend_device *,unsigned int,bool)" (?test_roundtrip@@YA?AU?$pair@HH@std@@PEAUggml_backend_device@@I_N@Z) [D:\a\llama.cpp\llama.cpp\build\tests\test-gguf.vcxproj]
2024-12-14T15:57:11.6976526Z test-gguf.obj : error LNK2019: unresolved external symbol gguf_write_to_buf referenced in function "struct std::pair<int,int> __cdecl test_roundtrip(struct ggml_backend_device *,unsigned int,bool)" (?test_roundtrip@@YA?AU?$pair@HH@std@@PEAUggml_backend_device@@I_N@Z) [D:\a\llama.cpp\llama.cpp\build\tests\test-gguf.vcxproj]

@JohannesGaessler
Copy link
Collaborator Author

Should the symbols be exported? I noticed that while I was working on the GGUF tests some other tests were disabled on Windows because "they use internal functions not exported with LLAMA_API". Wouldn't the same logic apply to test-gguf?

@slaren
Copy link
Collaborator

slaren commented Dec 14, 2024

In these tests, I decided to not make the symbols that they need public, because they have extremely generic names like unicode_cpt_to_utf8. These functions at least have a gguf_ prefix, so I think it would be ok to export them.

@JohannesGaessler
Copy link
Collaborator Author

Note to self: upon failure the current GGUF code can end up giving dangling pointers to user code, that should also be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ggml changes relating to the ggml tensor library for machine learning testing Everything test related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants