Conversation
|
I found one more instance on the cmake level, but that should be it. |
|
The |
|
Understood. Would you be open to conditionalizing the tests in cmake on |
Yes, that would be acceptable. I think the ideal version is to move the ggml tests from |
When using system ggml, skip tests against embedded ggml. The sources might have been stripped of the embedded version, downstream.
e44bd1d to
bd42c81
Compare
Thanks, I force-pushed a change just for that.
Agreed. I considered proposing this, but then guessed keeping |
| llama_build(test-export-graph-ops.cpp) | ||
| target_include_directories(test-export-graph-ops PRIVATE ${PROJECT_SOURCE_DIR}/ggml/src) | ||
| if (NOT LLAMA_USE_SYSTEM_GGML) | ||
| target_include_directories(test-export-graph-ops PRIVATE ${PROJECT_SOURCE_DIR}/ggml/src) | ||
| endif() |
There was a problem hiding this comment.
Should the llama_build(test-export-graph-ops.cpp) line be inside the if?
There was a problem hiding this comment.
I thought it's useful to test-backend-ops which is always built, so I conditionalized only the use of the embedded-ggml headers.
But that is because I initially misunderstood test-backend-ops as being llama-specific, whereas you clarified above it's not.
That made me wonder why test-backend-ops didn't need that target_include_directories that test-export-graph-ops needs. So I rechecked, and it's because the former includes <ggml.h> not "ggml.h".
So maybe the CMake change is the wrong change, and the backend approach should be mirrored in test-export-graph-ops with the following change - what do you think?
-#include "ggml.h"
+#include <ggml.h>
#include "gguf-model-data.h"
-#include "gguf.h"
-#include "ggml-backend.h"
+#include <gguf.h>
+#include <ggml-backend.h>test-gguf and test-alloc refer to non-public ggml headers, so they continue to need the guard.
No, I think the reason was that it was simpler to have all tests in one place at the beginning. Now it makes sense to differentiate between ggml tests and llama.cpp tests. |
|
Closing, superseded by #25616 |
Overview
The tests don't use anything from llama.cpp, only from ggml, so they can be moved there. This enables a source-level separation of llama.cpp and ggml.
Additional information
Tests are taken over by ggml PR1551. This needs a separate PR there because the
ggml/testsdirectory where they go isn't part of llama.cpp.Requirements