common: refactor model handling - #24980
Conversation
|
|
||
| using hf_files = std::vector<hf_file>; | ||
|
|
||
| struct hf_plan { |
There was a problem hiding this comment.
This is not related to hf-cache.h to me, but to the selection mechanism and model management
There was a problem hiding this comment.
hmm ok I moved it back to download.h in df822d2
| struct hf_plan { | ||
| hf_cache::hf_file primary; | ||
| hf_cache::hf_files model_files; | ||
| hf_cache::hf_file mmproj; | ||
| hf_cache::hf_file mtp; | ||
| hf_cache::hf_file preset; // if set, only this file is downloaded | ||
| }; | ||
| hf_plan get_hf_plan(const common_params_model & model, const common_download_opts & opts); |
There was a problem hiding this comment.
| struct hf_plan { | |
| hf_cache::hf_file primary; | |
| hf_cache::hf_files model_files; | |
| hf_cache::hf_file mmproj; | |
| hf_cache::hf_file mtp; | |
| hf_cache::hf_file preset; // if set, only this file is downloaded | |
| }; | |
| hf_plan get_hf_plan(const common_params_model & model, const common_download_opts & opts); | |
| struct common_download_hf_plan { | |
| hf_cache::hf_file primary; | |
| hf_cache::hf_files model_files; | |
| hf_cache::hf_file mmproj; | |
| hf_cache::hf_file mtp; | |
| hf_cache::hf_file preset; // if set, only this file is downloaded | |
| }; | |
| common_download_hf_plan common_download_get_hf_plan(const common_params_model & model, const common_download_opts & opts); |
|
|
||
| #include "common.h" | ||
| #include "download.h" | ||
| #include "hf-cache.h" |
| struct common_models_handler { | ||
| common_params & params; | ||
| common_download_callback * callback = nullptr; | ||
| bool preset_only = false; // if true, only check & download remote preset (for router mode) | ||
| }; | ||
| hf_plan plan; | ||
| common_download_opts opts; | ||
|
|
||
| common_models_handler(common_params & params) : params(params) {} | ||
|
|
||
| // fetch the metadata if needed (but do not download the model) | ||
| void fetch_meta(llama_example curr_ex); | ||
|
|
||
| // populate model paths (main model, mmproj, etc) from -hf if necessary | ||
| // return true if the model is ready to use | ||
| // throw an exception if there is an error that prevents the model from being used (e.g. network error, model not found, etc) | ||
| // if params.skip_download is true, no downloads will be attempted. return false if the model is invalid or missing (e.g. ETag check failed) | ||
| bool common_params_handle_models( | ||
| common_params & params, | ||
| llama_example curr_ex, | ||
| const common_params_handle_models_params & handle_params); | ||
| // return true if the input -hf is a preset-only repo (i.e. contains a preset.ini file) | ||
| bool is_preset_repo() const; | ||
|
|
||
| // download the model if needed, then apply it to the common_params | ||
| void apply(); | ||
|
|
||
| private: | ||
| std::string get_default_local_path(const std::string & url); | ||
|
|
||
| // build download tasks for a plain (non-hf) url model, honoring a user-supplied -m path | ||
| std::vector<common_download_task> build_url_tasks(const common_params_model & model); | ||
| }; |
There was a problem hiding this comment.
This struct seems a bit unnecessary - likely can remain a function. The private methods can be static functions in the cpp file.
There was a problem hiding this comment.
make sense, I refactored it again:
struct common_models_handler {
common_download_hf_plan plan;
common_download_opts opts;
};
common_models_handler common_models_handler_init(const common_params & params, llama_example curr_ex);
bool common_models_handler_is_preset_repo(const common_models_handler & handler);
void common_models_handler_apply(common_models_handler & handler, common_params & params, common_download_callback * callback = nullptr);Usage:
common_models_handler_initwill fetch metadata and construct the hf_plan / optscommon_models_handler_is_preset_repois to check if the hf-repo is preset.ini-onlycommon_models_handler_applywill download the files and modifyparamsto point to the downloaded location
There was a problem hiding this comment.
Since all the usages are like:
{
auto handler = common_models_handler_init(...);
if (need_apply) {
common_models_handler_apply(handler);
}
}It does not seem necessary to introduce the common_models_handler. A single function such as the following seems enough?
void common_params_update(
common_params & params,
llama_example ex,
bool apply,
common_download_callback * callback);There was a problem hiding this comment.
Your idea is similar to the existing common_params_handle_models before this refactoring, but the main point of the 2-step init/apply is for server code, it looks somewhat like this:
models_handler = common_models_handler_init(params, LLAMA_EXAMPLE_SERVER);
if (common_models_handler_is_preset_repo(models_handler)) {
// apply the preset and start the server in router mode
common_models_handler_apply(models_handler, params);
}
// do other endpoint setup
ctx_http.get ("/models/sse", progress_download_report);
// ... then later on
if (not_preset) {
// non-roter + non-preset: download model, with progress callback via `progress_download_report`
common_models_handler_apply(models_handler, params, callback);
}We could also get rid of common_models_handler but that will require re-fetching the list of files for each call. My idea is somewhat like this:
- The list of files to be downloaded will be stored in
common_models_handler - It can also be useful for future, for example, if we want to show to the user list of files (+ size), then ask for confirmation before downloading
There was a problem hiding this comment.
Also, note that if we only call common_models_handler_init without _apply, that means "validate if the HF model exist", and indeed the logic is being used in POST /models endpoint is now benefit from this
The b9803 model-download refactor (PR ggml-org/llama.cpp#24980) removed common_params::skip_download and common_skip_download_exception. That exposed that the project's ModelFlag.SKIP_DOWNLOAD ("--skip-download") never worked: --skip-download was never a registered upstream argument (at b9789 or b9803), so passing it only ever forced common_params_parse to fail, which SkipDownloadFailureTranslator mapped to ModelUnavailableException. It could not even load a model that WAS present — the unknown arg failed parsing unconditionally. Replace it with the real upstream --offline flag, which the new common_models_handler_apply honors (it skips all download tasks when params.offline is set): a cached/present model loads, a missing one fails instead of being fetched. - ModelFlag: SKIP_DOWNLOAD("--skip-download") -> OFFLINE("--offline") - ModelParameters: setSkipDownload(boolean) -> setOffline(boolean); add @nullable getModel() read-back used by the guard - loader/SkipDownloadFailureTranslator (parse-failure heuristic) -> loader/OfflineModelGuard: a deterministic PRE-check that throws ModelUnavailableException when --offline is set and the configured local --model file does not exist, before the native call. hf-repo / model-url loads (no local path) fall through to the native loader. - LlamaModel: call OfflineModelGuard.check(parameters) before load in both constructors; drop the catch-and-translate - ModelUnavailableException retained (typed air-gapped-miss signal); javadoc updated - Tests: LlamaModelSkipDownloadTest -> LlamaModelOfflineTest (flag round-trip + guard present/missing/hf-repo/not-set cases); ModelFlagTest + ModelUnavailableExceptionTest updated - Docs: breaking-changes row, TODO.md, feature-investigation table Pure-Java change, no JNI rebuild. spotless clean; LlamaModelOfflineTest (8), ModelFlagTest (103), ModelUnavailableExceptionTest (6), ModelParametersTest (64), LlamaArchitectureTest (12) green; javadoc:jar BUILD SUCCESS. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015iMgeCXHE9UNu359GbFyXj
|
After this change the following config no longer loads: https://huggingface.co/ggerganov/preset-128gb/blob/main/preset.ini#L122-L137 This is the error: Before the change it loads the draft model like this: |
* common: refactor models handling * remote preset * cont * rm skip_download option * missing header * fix plan.model_files * fix --offline case * move hf_plan to download * refactor * rm redundant curr_ex, add comments * adapt
|
After this change, this config broke. I start the server generally with the --offline argument. Config: |
* common: refactor models handling * remote preset * cont * rm skip_download option * missing header * fix plan.model_files * fix --offline case * move hf_plan to download * refactor * rm redundant curr_ex, add comments * adapt
Overview
Refactor model handling (
-m,-hf, etc) into a centralizedcommon_models_handlerobjectIt exposes 2 main calls:
fetch_meta(): check if the input option like-hfor-dris valid; in case of HF repo: get the list of filesapply(): download and apply the model local path tocommon_paramsRequirements