Support pulling CNCF ModelPack models with --source_model oci:// - #4489
Open
ericcurtin wants to merge 1 commit into
Open
Support pulling CNCF ModelPack models with --source_model oci://#4489ericcurtin wants to merge 1 commit into
ericcurtin wants to merge 1 commit into
Conversation
OVMS can pull generative models from Hugging Face today, but models are increasingly published as OCI artifacts following the CNCF ModelPack specification (https://github.com/modelpack/model-spec). Those live in the same registries, and behind the same auth and mirroring, as the container images a deployment already uses. Add an "oci://" source scheme to the existing pull flow: ovms --pull --source_model oci://ghcr.io/org/model:tag \ --model_repository_path /models --task text_generation The registry side is delegated to the llmman CLI, which already implements ModelPack media types, registry auth, resumable blob download and a content-addressed local store. A new OciDownloader runs `llmman resolve <ref>` and turns its single line of JSON into the two things the rest of the pull flow needs - the models_path for graph.pbtxt and, for GGUF payloads, the file name to append to it. llmman is located via PATH or LLMMAN_BIN; it is only required when an oci:// reference is actually used. Three payloads are handled: - OpenVINO IR, served straight from llmman's store, so the model repository only holds graph.pbtxt and the weights are not copied - GGUF, likewise served in place, via the existing GGUF support - HuggingFace safetensors, converted to IR by reusing OptimumDownloader with the resolved checkout as its --model argument, honoring --weight-format and --extra_quantization_params An explicit scheme is required rather than sniffing a bare "registry/name:tag", which is indistinguishable from a HuggingFace repo id and would hijack existing --source_model org/model deployments. References are rewritten for local use in one place, localModelDirectoryName(), which is the identity for every non-OCI source, so the ':' tag separator does not end up in a directory name on Windows. --task is required for oci:// references: inferring it means reading config.json from the source, which for a registry reference would mean pulling the whole image before the command line has finished parsing. --gguf_filename is rejected, since the layer media types already identify the payload. Tested with a mock llmman binary covering command construction, resolve output parsing (including diagnostics interleaved by exec_cmd), payload classification, and each failure mode, plus CLI parsing coverage in ovmsconfig_test.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds first-class support for pulling CNCF ModelPack models published as OCI artifacts by introducing an oci:// --source_model scheme, resolved via the external llmman CLI, while keeping the existing Hugging Face pull/conversion and downstream serving flow unchanged.
Changes:
- Add
OciDownloader(anIModelDownloaderimplementation) that resolvesoci://references viallmman resolveand maps outputs into OVMS’ existing graph export inputs. - Update CLI parsing, default-task inference, and pull flow routing to recognize OCI references, sanitize on-disk directory names, and reject incompatible flags (
--gguf_filename) for OCI. - Add dedicated unit tests + mock
llmmanbinary and user documentation for OCI pull mode.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/pull_oci_model_test.cpp | New unit tests for scheme handling, command construction, resolve parsing, and payload classification. |
| src/test/ovmsconfig_test.cpp | CLI parsing coverage for OCI download selection and validation. |
| src/test/llmman_mock.cpp | Mock llmman binary used by tests to avoid registry access. |
| src/status.hpp | Adds new StatusCode values for OCI pull failures. |
| src/status.cpp | Adds human-readable messages for new OCI StatusCodes. |
| src/pull_module/oci_downloader.hpp | Declares OciDownloader and its responsibilities/contract. |
| src/pull_module/oci_downloader.cpp | Implements OCI resolution via llmman, output parsing, and format-based handling (IR/GGUF/safetensors). |
| src/pull_module/model_downloader.cpp | Routes graph directory creation through localModelDirectoryName() to sanitize OCI references. |
| src/pull_module/hf_pull_model_module.cpp | Integrates OciDownloader into pull flow and propagates resolved model path into graph export settings. |
| src/pull_module/BUILD | Adds Bazel targets/deps for the new OCI downloader. |
| src/default_task.cpp | Prevents task inference attempts for OCI references (forces explicit --task). |
| src/cli_parser.cpp | Updates --source_model help, rejects --gguf_filename for OCI, and adjusts naming/path derivation. |
| src/capi_frontend/server_settings.hpp | Adds OCI detection/sanitization helpers and new download type enum value. |
| src/capi_frontend/server_settings.cpp | Implements isOciDownload, stripOciScheme, and localModelDirectoryName. |
| src/BUILD | Wires new tests and the mock llmman binary into Bazel build/test targets. |
| docs/pull_oci_models.md | New user documentation for pulling ModelPack OCI artifacts. |
| docs/pull_hf_models.md | Cross-links OCI pull mode from existing pull documentation. |
| docs/prepare_generative_use_cases.md | Adds OCI pull mode to the generative use-case preparation doc. |
| docs/parameters.md | Documents OCI-specific behaviors and LLMMAN_BIN. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+140
to
+147
| Status OciDownloader::convertToOpenVinoIr(const std::string& resolvedPath) { | ||
| SPDLOG_INFO("OCI model {} contains a HuggingFace-format checkout. Converting it to OpenVINO IR with optimum-cli.", this->sourceModel); | ||
| // optimum-cli accepts a local directory for --model, so the checkout that | ||
| // llmman produced is passed straight through as the export source. The | ||
| // conversion output lands in the graph directory, which keeps models_path | ||
| // at its default of "./". | ||
| OptimumDownloader optimumDownloader(this->exportSettings, this->task, resolvedPath, this->downloadPath, this->overwriteModels); | ||
| auto status = optimumDownloader.downloadModel(); |
Comment on lines
+265
to
+275
| auto* ociDownloader = dynamic_cast<OciDownloader*>(downloader.get()); | ||
| if (ociDownloader != nullptr) { | ||
| // llmman keeps the weights in its own content-addressed store, so the | ||
| // resolved location has to be propagated into graph.pbtxt rather than | ||
| // relying on the default "models live next to graph.pbtxt" layout. | ||
| this->hfSettings.exportSettings.modelPath = ociDownloader->getModelPath(); | ||
| this->hfSettings.ggufFilename = ociDownloader->getGgufFilename(); | ||
| std::cout << "Model: " << this->hfSettings.sourceModel << " resolved to: " << this->hfSettings.exportSettings.modelPath << std::endl; | ||
| } else { | ||
| std::cout << "Model: " << this->hfSettings.sourceModel << " downloaded to: " << graphDirectory << std::endl; | ||
| } |
Comment on lines
+194
to
+214
| if (format == "gguf") { | ||
| // models_path must point at the GGUF file itself, which the graph | ||
| // exporter builds by joining the directory with ggufFilename. | ||
| const std::filesystem::path ggufPath(resolvedPath); | ||
| this->modelPath = ggufPath.parent_path().string(); | ||
| this->ggufFilename = ggufPath.filename().string(); | ||
| } else if (format == "safetensors") { | ||
| if (containsOpenVinoIr(resolvedPath)) { | ||
| // Already an OpenVINO IR ModelPack - serve it straight from | ||
| // llmman's store, no conversion and no second copy on disk. | ||
| this->modelPath = resolvedPath; | ||
| } else { | ||
| status = this->convertToOpenVinoIr(resolvedPath); | ||
| if (!status.ok()) { | ||
| return status; | ||
| } | ||
| } | ||
| } else { | ||
| SPDLOG_ERROR("llmman reported unsupported format \"{}\" for {}. Supported formats: gguf, safetensors.", format, this->sourceModel); | ||
| return StatusCode::OCI_UNSUPPORTED_MODEL_FORMAT; | ||
| } |
Comment on lines
+55
to
+68
| std::string OciDownloader::getVersionCmd() const { | ||
| std::ostringstream oss; | ||
| oss << this->llmmanBinary << " --version"; | ||
| return oss.str(); | ||
| } | ||
|
|
||
| std::string OciDownloader::getResolveCmd() const { | ||
| std::ostringstream oss; | ||
| // Quoting keeps a reference containing shell-significant characters in a | ||
| // single argv entry. exec_cmd() never spawns a shell, so this is only | ||
| // about argument splitting, not injection. | ||
| oss << this->llmmanBinary << " resolve \"" << stripOciScheme(this->sourceModel) << "\""; | ||
| return oss.str(); | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Suppressed comments (2)
src/pull_module/oci_downloader.cpp:57
LLMMAN_BINis documented as a full path, but the executable token is unquoted in both generated commands. A valid path such asC:\Program Files\llmman\llmman.exeis split at the space, so the presence check and every OCI pull fail. Quote the executable in both commands.
std::string OciDownloader::getVersionCmd() const {
std::ostringstream oss;
oss << this->llmmanBinary << " --version";
src/pull_module/oci_downloader.cpp:146
- The resolved local path is passed as
OptimumDownloader::sourceModel, but that class concatenatessourceModelanddownloadPathinto command strings without quoting (for example,optimum_export.cpp:40-45). llmman's cache/store path can legally contain spaces viaLLMMAN_MODELSor a Windows profile, causing safetensors conversion to split both paths into multiple arguments and fail. Escape/quote path arguments consistently, or use an argv-based execution API.
OptimumDownloader optimumDownloader(this->exportSettings, this->task, resolvedPath, this->downloadPath, this->overwriteModels);
Comment on lines
+200
to
+204
| } else if (format == "safetensors") { | ||
| if (containsOpenVinoIr(resolvedPath)) { | ||
| // Already an OpenVINO IR ModelPack - serve it straight from | ||
| // llmman's store, no conversion and no second copy on disk. | ||
| this->modelPath = resolvedPath; |
Comment on lines
+64
to
+68
| std::string name = stripOciScheme(sourceModel); | ||
| // ':' separates the tag (and, for a non-default registry port, the port). | ||
| // It is not a legal filename character on Windows. | ||
| std::replace(name.begin(), name.end(), ':', '_'); | ||
| return name; |
Comment on lines
82
to
+84
| // Try local model repository path before downloading from HuggingFace | ||
| if (modelRepositoryPath.has_value() && !modelRepositoryPath->empty()) { | ||
| const auto localModelDir = std::filesystem::path(*modelRepositoryPath) / *sourceModel; | ||
| const auto localModelDir = std::filesystem::path(*modelRepositoryPath) / localModelDirectoryName(*sourceModel); |
Comment on lines
+205
to
+206
| } else { | ||
| status = this->convertToOpenVinoIr(resolvedPath); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an
oci://source scheme to OVMS pull mode so models published as CNCF ModelPack OCI artifacts can be deployed the same way Hugging Face models are today:ovms --pull --source_model oci://ghcr.io/org/model:tag \ --model_repository_path /models --task text_generationModel distribution is increasingly moving to OCI registries — the same registries, credentials, mirroring and air-gap tooling a deployment already uses for container images. ModelPack is the CNCF spec for that, and this makes OVMS a first-class consumer of it.
How it works
The registry side is delegated to the
llmmanCLI, which already implements the ModelPack media types, registry auth, resumable blob download and a content-addressed local store. A newOciDownloader : IModelDownloaderrunsllmman resolve <reference>and parses its documented single-line JSON contract:{"reference":"ghcr.io/org/model:tag","path":"/abs/path","format":"safetensors"}That yields the two things the rest of the pull flow needs: the
models_pathforgraph.pbtxt, and for GGUF payloads the file name to append to it. Everything downstream —GraphExport,config.json, serving — is unchanged.llmmanis located viaPATHorLLMMAN_BIN, and is only required when anoci://reference is actually used. No new build-time dependency, no new third-party target inWORKSPACE.Payloads
graph.pbtxt, weights are not copiedOptimumDownloaderwith the resolved checkout as its--model, honoring--weight-format/--extra_quantization_paramsDesign notes
registry/name:tagis indistinguishable from a Hugging Face repo id (org/model); guessing would silently hijack existing--source_model org/modeldeployments.localModelDirectoryName()is the identity for every non-OCI source, so theoci://scheme and the:tag separator never reach a directory name (:is illegal on Windows).IModelDownloader::getGraphDirectory()andprepareGraphStart()both route through it, so behaviour for Hugging Face sources is bit-for-bit unchanged.ghcr.io/org/model:tag), overridable with--model_name.Limitations (documented)
--taskis required. Inferring it means readingconfig.jsonfrom the source, which for a registry reference would mean pulling the whole image before the CLI has finished parsing — sodetermineDefaultTaskParameter()returns "unknown" foroci://and the existing "specify --task explicitly" error fires instead of issuing a bogushuggingface.co/oci://...request.--gguf_filenameis rejected; the layer media types already identify the payload.--draft_source_modeland--source_lorasstill resolve from Hugging Face.Testing
New
src/test/pull_oci_model_test.cpp(28 assertions across 18 cases) driven by a mockllmmanbinary (src/test/llmman_mock.cpp, same pattern as the existingoptimum-climock), covering:stripOciScheme,localModelDirectoryName, graph directory derivationisOptimumCliDownload()no longer claimsoci://referencesllmmancommand construction andLLMMAN_BIN/PATHresolutionexec_cmd()interleaves from the child's stderr, and every malformed-output casePlus CLI-parsing coverage in
ovmsconfig_test.cpp: download-type selection,--weight-formatnot rerouting to optimum-cli, pull-and-start naming, and death tests for--gguf_filenameand missing--task.cpplint,codespelland the license-header check pass on the diff.