feat: accept oci:// model sources in the model builder - #2503
Open
Eric Curtin (ericcurtin) wants to merge 1 commit into
Open
Eric Curtin (ericcurtin) wants to merge 1 commit into
Eric Curtin (ericcurtin) wants to merge 1 commit into
Conversation
Lets -i/--input (or -m/--model_name) point at a model published as a
CNCF ModelPack OCI artifact:
python builder.py -i oci://ghcr.io/org/model:tag -o out -p int4 -e cpu
Model distribution is increasingly moving to OCI registries, which lets
a build reuse the registry, credentials, mirroring and air-gap tooling it
already has for container images.
Acquisition is delegated to a running `llmman serve`, which already
implements the ModelPack media types, registry auth, resumable blob
download and a content-addressed store. The daemon does the pull (POST
/api/pull, streamed as NDJSON so a multi-gigabyte fetch is not silent,
and an error arriving in-band at HTTP 200 is caught) but deliberately
exposes no local path, so `llmman resolve --no-pull` reports where the
bytes landed. The client is stdlib-only, so no new dependency.
Resolution happens once in get_args(), rewriting args.input to the
extracted directory. From there os.path.isdir(input_path) is true, so
get_hf_details and everything downstream treat it as an ordinary local
Hugging Face directory with no further changes.
An explicit oci:// scheme is required rather than sniffing a bare
registry/name:tag: that shape is indistinguishable from a Hugging Face
repo id, so guessing would silently hijack existing -m values.
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds support for using oci://... model sources in the Python model builder by delegating OCI artifact acquisition to a running llmman serve daemon, then rewriting the resolved path to a local Hugging Face-style directory so the rest of the builder pipeline remains unchanged.
Changes:
- Add a new stdlib-only
llmmandaemon client for probing, pulling NDJSON progress, and resolving the pulled model path. - Extend
builder.pyargument parsing to detectoci://...in--inputor--model_nameand resolve it to a local directory before continuing. - Add pytest coverage for the daemon client behaviors using a real loopback HTTP server (NDJSON streaming, in-band errors, endpoint parsing, scheme detection).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/python/test_llmman.py | Adds integration-style tests for the llmman client using a real local HTTP server. |
| src/python/py/models/llmman.py | Implements the llmman serve client (probe, pull NDJSON, resolve path, oci:// helpers). |
| src/python/py/models/builder.py | Adds oci:// detection and resolution during argument parsing and documents the new input form. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+58
to
+79
| if "://" in raw: | ||
| raw = raw.split("://", 1)[1] | ||
| raw = raw.split("/", 1)[0] | ||
|
|
||
| host, port = raw, DEFAULT_PORT | ||
| if raw.startswith("["): # bracketed IPv6, optionally with :port | ||
| close = raw.find("]") | ||
| if close != -1: | ||
| host = raw[: close + 1] | ||
| rest = raw[close + 1 :] | ||
| if rest.startswith(":") and rest[1:].isdigit(): | ||
| port = int(rest[1:]) | ||
| elif raw.count(":") == 1: | ||
| maybe_host, maybe_port = raw.rsplit(":", 1) | ||
| if maybe_port.isdigit(): | ||
| host, port = maybe_host, int(maybe_port) | ||
|
|
||
| host = host or DEFAULT_HOST | ||
| resolved = _connectable_host(host) | ||
| if ":" in resolved and not resolved.startswith("["): | ||
| resolved = f"[{resolved}]" | ||
| return f"http://{resolved}:{port}" |
Comment on lines
+142
to
+144
| if status == "success": | ||
| succeeded = True | ||
| continue |
Comment on lines
+41
to
+42
| def _connectable_host(host: str) -> str: | ||
| """Rewrite a wildcard bind host to its loopback equivalent.""" |
Comment on lines
622
to
+626
| help=textwrap.dedent("""\ | ||
| Input model source. Currently supported options are: | ||
| hf_path: Path to folder on disk containing the Hugging Face config, model, tokenizer, etc. | ||
| gguf_path: Path to float16/float32 GGUF file on disk containing the GGUF model | ||
| oci://<registry>/<repo>:<tag>: CNCF ModelPack artifact, pulled via an llmman daemon |
Comment on lines
+245
to
+251
| def _progress(status, completed, total): | ||
| if total: | ||
| print(f"llmman: {status} ({completed}/{total} bytes)") | ||
| else: | ||
| print(f"llmman: {status}") | ||
|
|
||
| return pull_and_resolve(reference, progress=_progress) |
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.
Description
Adds an
oci://source so a model published as a CNCF ModelPack artifact can be built without first fetching it from Hugging Face:Model distribution is increasingly moving to OCI registries -- the same registries, credentials, mirroring and air-gap tooling a build pipeline already uses for container images. Usually easier to run in a locked-down build environment than reaching the Hub.
Implementation
Resolution happens once in
get_args(), rewritingargs.inputto the extracted directory. From thereos.path.isdir(input_path)is true, soget_hf_detailsand everything downstream treat it as an ordinary local Hugging Face directory --AutoConfig.from_pretrained,AutoTokenizer.from_pretrained, quantization and export all unchanged.Accepted on either flag, since a registry reference is equally a "model name" and an "input source";
-m oci://...setsargs.inputand clearsargs.model_name, matching what a local-ipath already does.Acquisition is delegated to a running
llmman serverather than hand-rolled: llmman already implements the ModelPack media types, registry auth, resumable blob download and a content-addressed store.New
src/python/py/models/llmman.pyis the daemon client, stdlib-only (urllib), no new dependency:GET /api/versionprobes reachability and identity -- a server answering without aversionfield is reported as "not an llmman daemon", worth distinguishing from nothing listening.POST /api/pullstreams NDJSON so a multi-gigabyte fetch is not silent. An error arrives in-band at HTTP 200, and a stream that ends withoutsuccessis also a failure -- both are errors, not a completed pull.llmman resolve --no-pullreports where the bytes landed;--no-pullkeeps the daemon the only thing that touches the network.LLMMAN_HOSTis honoured with llmman's own parsing, including rewriting a wildcard bind (0.0.0.0,[::]) to loopback.A build needs both the daemon reachable and the binary on
PATH(orORTGENAI_LLMMAN_BIN); each missing piece has its own actionable error, and neither is required unless anoci://source is used.Design notes
registry/name:tagis indistinguishable from a Hugging Face repo id (microsoft/Phi-3-mini-4k-instruct); guessing would silently hijack existing-m org/modelinvocations. Every other value reaches exactly the branch it did before.resolvestdout is used; unknown JSON fields are ignored so the contract can grow.-i/--inputhelp text documents the new form alongsidehf_pathandgguf_path.Testing
New
test/python/test_llmman.py, running against a real HTTP server on a loopback port rather than mocks, so the NDJSON contract is genuinely exercised.All 22 executed here. Coverage:
/api/versionaccepted, a non-llmman server rejected, nothing-listening reported actionably; pull success with forwarded byte progress and the exact request body asserted; in-band error at HTTP 200; a stream ending withoutsuccess; non-OK status; a non-JSON diagnostic tolerated; scheme detection incl. case-insensitivity; that a HF repo id, a bare registry ref and a local path are not claimed;strip_schemeround-trips; empty reference rejected; everyLLMMAN_HOSTform incl. wildcard-to-loopback.ruff formatandruff checkclean on both new filesNot verified here, flagged rather than implied: no end-to-end
builder.pyrun against a livellmman servebacked by a real registry -- that needs torch, transformers and onnx, which are not installed in this environment. Theget_args()branch itself is therefore covered by inspection rather than execution.