Design: decompose ModelCache into a per-cluster ModelCacheHydration - #362
Design: decompose ModelCache into a per-cluster ModelCacheHydration#362dennis-upbound wants to merge 11 commits into
Conversation
bf7fe2c to
8830612
Compare
ModelCache runs every matched cluster's hydration lifecycle inline in one fan-out function, and its footprint (clusterSelector) can silently diverge from where replicas are scheduled, so a replica can land on a cluster the cache never staged to and fail to mount at runtime. This design proposes a per-cluster ModelCacheHydration child, mirroring ModelReplica, and makes the cache footprint follow placement. It covers issues modelplaneai#210 and modelplaneai#186, with architecture and lifecycle diagrams. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
8830612 to
4feb19b
Compare
| - **Resolve** the referencing `ModelReplica`s (the placed set) and, for pre-warm, | ||
| the `clusterSelector`. | ||
| - **Stamp** one child per cluster in `placed ∪ preWarm`, named | ||
| `child_name("modelcache", ns, cache-name, cluster)`, copying down `huggingFace`, |
There was a problem hiding this comment.
do we have here a problem with 63 char limit ?
There was a problem hiding this comment.
No 63-char problem here: resource.child_name is DNS-label-safe by construction. It joins the parts, truncates the prefix, and appends a 5-char hash, so the result is always a valid label at or under 63 chars, including the child's own name with cluster folded in. The PVC/Job/Secret keep the cluster out of their names (child_name("modelcache", ns, cache-name[, "hydrate"|"auth"])) so they match the serving-side mount, and those stay ≤63 too.
Added a line to the naming section spelling this out. Pushed.
|
|
||
| ### Hydrating before ready | ||
|
|
||
| A replica can be scheduled onto a cluster new to the model before its PVC exists. |
There was a problem hiding this comment.
If hydration on that cluster fails permanently, expired HF token, storage exhausted, bad revision, the replica appears to be gated indefinitely.
This needs a backoff plus a Failed condition that propagates up to the ModelDeployment, otherwise a credential problem on one cluster surfaces as a deployment that simply never becomes ready?!
There was a problem hiding this comment.
Good catch, this is the real gap in the gate. It had no failure exit: a bad token or bad revision would hold the replica in Hydrating forever.
The child already has a Failed phase, so the fix is to use it: a failed hydration surfaces as ArtifactReady=False with reason HydrationFailed on the ModelCache, and the gated replica fails with that reason instead of gating indefinitely. The hydration Job's backoffLimit bounds retries, so a permanent failure stops and is reported rather than retried forever.
Added a failure path to Hydrating before ready. Pushed.
The hydration gate had no failure exit: a bad token or bad revision would hold the replica in Hydrating forever. Add a Failed path that surfaces ArtifactReady=False and fails the replica, bounded by the Job's backoffLimit. Note that child_name keeps every composed name a valid 63-char DNS label. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
negz
left a comment
negz
left a comment
There was a problem hiding this comment.
I like this direction overall. The two things that need the most thought before we proceed:
- Does this need to wait for crossplane/crossplane#7572?
- Is hydrate-on-demand definitely desirable?
Adopt Nic's direction: the cache's clusterSelector is the authoritative footprint the platform team pre-warms, rather than deriving the footprint from replica placement. A deployment loads from the cache where staged and from the source elsewhere, decided at compose time via the footprint resolution that already exists (resolve_cache_footprint). This removes the double-download tax and the crossplane#7572 dependency, and relaxes the modelplaneai#189 placement constraint. Keep the ModelCacheHydration decomposition. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
|
Went with pre-warm on both counts.
Doc reworked around this. Inline replies below. |
Add an explicit approval line so a review means something clear: it covers the ModelCacheHydration decomposition and the pre-warm-authoritative footprint, including load-from-source off the footprint. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
A cache-referencing deployment now runs only where the cache is pre-warmed, so the engine command is uniform and Modelplane injects no engine flags. Drop load-from-source off the footprint: it needed per-replica model-arg injection and didn't generalize to loader-plugin caches like ModelExpress. State the two modes with both actors named, the env-values-not-flags rule, and the two engine contracts (path vs loader plugin), and add reference-aware reclaim so a cache is never pulled out from under a live replica. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
Fold the two open questions into decisions in the body: the child is named ModelCacheHydration for the lifecycle it owns, and a ModelCache with no clusterSelector is rejected at apply time. Remove the Open questions section. Cut the Alternatives roughly in half so each rejected path states its reason once, so a reader can see what the doc proposes without wading through the paths not taken. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
The two engine contracts (path vs loader plugin) and the three costs of the on-demand alternative were dense paragraphs describing parallel items. Set each as a short list so a reader scans the choices instead of parsing them out of prose. No wording change beyond the split. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
…vLLM The contract text cited only vLLM flags and read as if --load-format were universal. Stress-testing against SGLang and TensorRT-LLM shows the model holds but the flag names are engine-specific: SGLang reads --model-path, not --model, and ModelExpress provides loaders across vLLM, SGLang, and TensorRT-LLM. Name the concrete flags and state that the model-naming flag belongs to the engine in both contracts, which is the reason Modelplane injects env values and not flags. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
The doc read like a generated draft next to design.md: decorative bold-bullet
lists where prose belonged, third-person framing, and trailing comma-appositives
where a colon or parenthetical reads cleaner. Move the summary to first person
("I propose two changes"), set the two modes and the two contracts as bold
lead-in paragraphs rather than bullets, echo the established persona split
(platform owns what's cached and where, ML owns what to run), and use colons and
parentheses for the inline expansions. No change to the proposal itself.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Dennis Ramdass <dennis@upbound.io>
| Either way the flag that names the model belongs to the engine, so Modelplane stays out | ||
| of it and injects only env values. Placing only where the cache is pre-warmed | ||
| makes whatever the ML team wrote valid wherever the replica runs. How the bytes reach | ||
| the cluster, whether a shared filesystem, peer-to-peer distribution, or GPU-to-GPU | ||
| streaming, is the platform team's concern and orthogonal to the command. The catalog of | ||
| backends is a separate design, and each backend has to present one of these two | ||
| contracts. |
There was a problem hiding this comment.
What env value will it inject? Can you show how a ModelDeployment will use it?
There was a problem hiding this comment.
In the common case, nothing. The cache mounts at the same path on every footprint cluster, so the ML team just writes --model=/mnt/models. Added a snippet showing it.
Injection only covers a value that varies per cluster: MODELPLANE_LEADER_ADDRESS today, or a loader-plugin's server address. Always an env var via $(VAR), never a flag we write.
There was a problem hiding this comment.
Got it, and my earlier "nothing" over-rotated. Locked the name.
With a cache Modelplane owns the mount, so it injects the mount path as MODELPLANE_MODEL and the ML team writes --model=$(MODELPLANE_MODEL) (or --model-path=$(MODELPLANE_MODEL) on SGLang). They never hardcode our mount path, so we stay free to move it. Same $(VAR) mechanism as MODELPLANE_LEADER_ADDRESS. Without a cache they write --model=<source> directly, since the source is theirs.
Updated the section and the snippet. MODELPLANE_MODEL is my pick for the name, easy to change if you'd rather something else.
negz
left a comment
negz
left a comment
There was a problem hiding this comment.
I'd like to clarify what env var(s) you'll inject and how they'll be used but other than that LGTM.
The env section asserted that Modelplane injects env values without showing what value or how a deployment uses it. State the stronger truth: in the common path-cache case it injects nothing, because placement constrains the mount to a uniform path the ML team writes directly. Add a ModelDeployment snippet, and keep env injection as the mechanism for a value that varies per cluster (the leader address today, a loader-plugin's server address), referenced with Kubernetes $(VAR) expansion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
Nic asked for the concrete env var name and the caller-facing shape. With a cache Modelplane owns the mount path, so it injects it as MODELPLANE_MODEL and the ML team writes --model=$(MODELPLANE_MODEL), never hardcoding the mount. Same $(VAR) mechanism as MODELPLANE_LEADER_ADDRESS. Without a cache they write --model=<source> directly. Replaces the earlier injects-nothing framing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
3 participants
Description of your changes
A design doc, not code yet — opening as a draft for direction before implementation.
ModelCacheruns every matched cluster's hydration lifecycle inline in one fan-out function (PVC + Job + token Secret + a four-phase machine + drop-after-Ready + token resolution, threadingcluster_namethrough every method), and it's the one fan-out that doesn't followModelDeployment → ModelReplica. Separately (#186), a cache'sclusterSelectorand a deployment's placement are decided independently, so a replica can land on a cluster the cache never staged to and fail to mount at runtime, with nothing visible at apply time.This proposes handling both together: a per-cluster
ModelCacheHydrationchild (the cache analogue ofModelReplica) that owns one cluster's hydration, and making theModelCachefootprint follow where its referencing replicas are placed rather than a hand-maintained selector. Covers #210 and #186. Includes an architecture diagram and a lifecycle flow diagram.Where I'd most like judgment (all in the doc): the unified vs phased delivery, keeping
modelCacheRefvs deriving the model from the deployment, the fleet-reconciler-vs-reuse-ModelCacheownership question, and the child kind name (ModelCacheHydrationvsModelCacheReplica).design/modelcache-hydration.mdonly; no code.I have:
Run(design doc only, no code paths)nix flake check(or./nix.sh flake check) and made sure it passes.Added or updated tests covering any composition function changes.(design doc only)git commit -s.