llama : add fd-based model loading via llama_model_load_from_fd ( REWORK )#20402
llama : add fd-based model loading via llama_model_load_from_fd ( REWORK )#20402Siddhesh2377 wants to merge 4 commits intoggml-org:masterfrom
Conversation
Siddhesh2377
requested review from
JohannesGaessler and
ggerganov
as code owners
Siddhesh2377
changed the title
github-actions
Bot
added
testing
|
|
||
| GGML_API struct gguf_context * gguf_init_empty(void); | ||
| GGML_API struct gguf_context * gguf_init_from_file(const char * fname, struct gguf_init_params params); | ||
| GGML_API struct gguf_context * gguf_init_from_fd(int fd, struct gguf_init_params params); |
There was a problem hiding this comment.
For your purposes, would it work to expose the current gguf_init_from_file_impl as gguf_init_from_file_ptr and to use that as the basis for the implementation instead? That way we would be able to also use this code on Windows in conjunction with ggml_fopen.
There was a problem hiding this comment.
Done, replaced gguf_init_from_fd with gguf_init_from_file_ptr(FILE *) and moved the dup+fdopen logic up to llama-model-loader
There was a problem hiding this comment.
The llama C API should also use a file pointer if at all possible, the conversion from file descriptor to file pointer should be in your user code.
There was a problem hiding this comment.
Done, switched the llama C API to FILE pointer as well. The test shows the fd to FILE* conversion on the caller side.
JohannesGaessler
left a comment
JohannesGaessler
left a comment
There was a problem hiding this comment.
Maybe I should have made my intent more clear: The GGUF and llama APIs should be using a file pointer rather than a file descriptor because that has Windows compatibility. But then that is also what should be used internally because conversions between the two add unnecessary complexity. Your PR should not be using file descriptors anywhere, please consistently use file pointers.
| struct gguf_context * gguf_init_from_file_ptr(FILE * file, struct gguf_init_params params) { | ||
| if (!file) { | ||
| return nullptr; | ||
| } | ||
| return gguf_init_from_file_impl(file, params); | ||
| } | ||
|
|
There was a problem hiding this comment.
Just rename gguf_init_from_file_impl to gguf_init_from_file_ptr and add the check there. Keep the check in gguf_init_from_file since it is associated with a warning.
| struct llama_model_params params); | ||
|
|
||
| // Load a model from an open FILE pointer | ||
| LLAMA_API struct llama_model * llama_model_load_from_file_ptr(FILE * file, struct llama_model_params params); |
There was a problem hiding this comment.
Please keep the formatting consistent with the surrounding code.
There was a problem hiding this comment.
Done, and sorry for the misunderstanding :)
| int file_id() const; // fileno overload | ||
| int file_id() const; |
There was a problem hiding this comment.
What did "fileno" refer to here and why did you remove this comment?
| GGML_ASSERT((metadata == nullptr) != path_model.empty() && "exactly one out of metadata and path_model needs to be defined"); | ||
| if (metadata == nullptr && path_model.empty() && !file) { | ||
| LLAMA_LOG_ERROR("%s: no model source provided\n", __func__); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
The logic should remain that exactly one out of the three things needs to be defined. Something like this should work:
GGML_ASSERT(int(metadata != nullptr) + int(path_model.empty()) + int(file != nullptr) == 1 && "exactly one out of metadata and path_model needs to be defined");
There was a problem hiding this comment.
I am currently adding file saving/loading to the recently added end-to-end tests in test-llama-archs.cpp via #20503 . This will provide test coverage for your newly added code so I don't think we need this additional test.
2 participants
Adds
llama_model_load_from_fd()to load GGUF models from a POSIX file descriptor instead of a file path.On Android, apps accessing user files through SAF only get a file descriptor, not a path. The alternative is copying the model into app storage or requesting MANAGE_EXTERNAL_STORAGE, which gets rejected by Google Play. This happened with my app (ToolNeuron).
Reworked version of a previous PR that was rejected for code quality.
Not supported on Windows. The fd is dup'd internally so the caller retains ownership.
Tested locally with CI and a real model (vocab_only + mmap).