Repository navigation
Conversation
…pdate inferenceClusters, servingStack Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
|
Docs preview: https://modelplane-docs-pr-556.vercel.app (ready once the site's Content workflow finishes) |
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
There was a problem hiding this comment.
Thanks, the mechanism reads well and I checked a lot of it: the tags on every cloud (neutral components untagged, every NVIDIA-only one tagged), the dependency check, the mirror against supported_drivers() for all six clouds, stacks-current regenerating the aicr files byte for byte, the dra-driver-cpu 0.3.0 chart and its dra.cpu capacity key, and the AMD exporter's names, units and discovery labels against device-metrics-exporter v1.5.1. A pre-#556 ServingStack also keeps its full stack until the cluster composition stamps draDrivers, which is the right upgrade behavior.
One problem deletes running things, so I'd fix it before merge: the UnsupportedDevices gate deletes a live cluster (inline on compose-inference-cluster). I've withdrawn the gpuPools rename comment: it only matters on upgrade, which is fine before v1.
The rest are inline: the CPU role coming from a missing accelerator, the AMD operator values, the energy metric's type, the docs-site ordering, and what the two recipes and e2e legs actually validated. Also, the four commits use typed prefixes and have no bodies, and update for vale is a fixup; CONTRIBUTING asks for problem-first messages without prefixes.
| return | ||
|
|
||
| source = cluster.source | ||
| if not self.check_devices_supported(source): |
There was a problem hiding this comment.
Blocking: when this gate fires on a running cluster, Crossplane deletes the cluster. The early return comes before compose_<cloud>(), and check_devices_supported sets a condition rather than a fatal result. So the desired state holds only the guard and namespaces, and Crossplane garbage-collects the cloud cluster XR, the ServingStack and their Usages. I ran the GKEReady case with the class's driver changed to gpu.amd.com: desired went from [activation, cluster-provider-config-kubernetes, gke-cluster, serving-stack, usage-gke-by-backend] to [].
WaitingForClasses already works this way, but this gate is easy to hit on a live cluster. Someone edits a shared class, or adds a pool whose class claims gpu.amd.com on Vultr (the design's own example), and the whole cluster goes, NVIDIA pools and their models included.
A fatal result would stop it: Crossplane returns before garbage collection on a fatal result. Or keep composing what exists and leave out only the unsupported pool. The gate's tests call check_devices_supported directly; a RunFunction case on a cluster with observed resources would have caught this.
| the cache PVC. | ||
| maxLength: 253 | ||
| gpuPools: | ||
| schedulablePools: |
There was a problem hiding this comment.
Withdrawn: this only bites during an upgrade, and breaking changes are fine before v1. Ignore it.
| gke: | ||
| type: object | ||
| required: [machineType, accelerator] | ||
| required: [machineType] |
There was a problem hiding this comment.
The pool's role now comes from whether accelerator is set (role="GPU" if prov.accelerator else "CPU" in each cloud), and nothing ties that to devices. A GPU class claiming gpu.nvidia.com × 8 that leaves out accelerator by mistake was rejected on main. Now it composes a CPU pool: no GPU, a standard image and the modelplane.ai/cpu taint, while draDrivers installs the NVIDIA stack and schedulablePools advertises 8 GPUs. Deployments then schedule onto it and their claims never allocate. A CEL rule on the class would close it, for example requiring accelerator when a DRA device's driver isn't dra.cpu, and rejecting accelerator on a class that claims only dra.cpu.
| }, | ||
| ), | ||
| # The AMD GPU operator, in DRA mode: the chart's default DeviceConfig | ||
| # (crds.defaultCR) enables the DRA driver and disables the device |
There was a problem hiding this comment.
I rendered gpu-operator-charts v1.5.1 with these values. The DeviceConfig they produce is right (enableDevicePlugin: false, draDriver.enable: true, driver.enable: false), but:
- Later value changes never reach it.
templates/default-deviceconfig.yamlrenders only when(IsInstall && crds.defaultCR.install) || (IsUpgrade && crds.defaultCR.upgrade),defaultCR.upgradedefaults tofalse, and the object hashelm.sh/resource-policy: keep. Changing tolerations or an image here later upgrades the release and leaves the live DeviceConfig at its install-time spec. Settingcrds.defaultCR.upgrade: truefixes it. - The DRA driver floats. The render has
docker.io/rocm/k8s-gpu-dra-driver:latest,rocm/k8s-device-plugin:labeller-latestanddocker.io/rocm/gpu-operator-utils:latest. The chart pin doesn't pin the DRA driver, the component the claims go through. The metrics exporter does resolve tov1.5.1. - KMM is installed. "Driver management (KMM) stays off" holds for the DeviceConfig, but
kmm.enableddefaults totrue, so the release runs the KMM controller, its webhook server and a ValidatingWebhookConfiguration. Either setkmm.enabled: falseor reword the comment.
| "gpu_umc_activity": "modelplane_gpu_memory_bandwidth_ratio", | ||
| "gpu_junction_temperature": "modelplane_gpu_temperature_celsius", | ||
| "gpu_power_usage": "modelplane_gpu_power_watts", | ||
| "gpu_energy_consumed": "modelplane_energy_joules_total", |
There was a problem hiding this comment.
gpu_energy_consumed is a Gauge in device-metrics-exporter v1.5.1 (NewGaugeVec, "Accumulated energy consumed by the GPU in uJ"), and DCGM_FI_DEV_TOTAL_ENERGY_CONSUMPTION is a counter. Both now become modelplane_energy_joules_total, which the telemetry design documents as a counter, so one name carries two types: a monotonic Sum from NVIDIA and a Gauge from AMD, on a mixed Existing cluster or across a fleet. rate() on a Prometheus sink still works, but a sink that respects OTLP types will see the conflict. A convert_gauge_to_sum("cumulative", true) for this metric before the rename would make them agree.
| icon: brand-civo | ||
| - name: Bring your own | ||
| term: Existing | ||
| accelerators: |
There was a problem hiding this comment.
On merge ordering: docs-site builds main from this repo's HEAD with one theme for every version, and the theme reads gpus (shortcodes/validated-deployments.html:39, partials/recipe-row.html:17). Until it changes, the GPU chips in the validated-deployments panel render empty on /main/, and nothing fails the build because ranging over a missing key is empty in Go templates. release-0.4 and release-0.5 still have gpus, so the theme has to read both, something like $d.accelerators | default $d.gpus, rather than switch. #brand-civo also doesn't exist yet, so the Civo tile renders an empty <svg> rather than the monogram fallback.
There was a problem hiding this comment.
it's in the PR description already
The recipe site needs two follow-ups in its repo: the gpus data key the ayouts read is now accelerators, and partials/icons.html needs brand-amd, brand-civo and cpu symbols. https://github.com/modelplaneai/docs-site/blob/main/themes/geekboot/layouts/partials/icons.html
| when a class claims that driver, against the amdgpu kernel driver the node | ||
| image already carries. The engine is AMD's ROCm vLLM. | ||
|
|
||
| The manifests are the ones validated end to end on an 8x MI325X bare metal |
There was a problem hiding this comment.
#434 ran this with source: VultrBaremetal and a class with a vultrBaremetal provisioning block. Here the class has no provisioning and the cluster is source: Existing, with the AMD operator coming from the Existing list instead. The ModelDeployment and ModelService are the ones that ran, and the operator values match byte for byte, but the cluster and class shown didn't. I'd say "the deployment manifests" were validated, or run it again on Existing.
Same question for the CPU recipe: is there a run on Civo behind it? I didn't find one in #556 or #509.
There was a problem hiding this comment.
I running e2e the Recipes thats why the PR is a draft atm
| spec: | ||
| containers: | ||
| - name: engine | ||
| image: vllm/vllm-openai-cpu:v0.32.0-x86_64 |
There was a problem hiding this comment.
dra-driver-cpu's docs/user/workload-requirements.md says that before KEP-5517 a pod must also request and limit cpu equal to the CPUs its claim takes, or the scheduler doesn't count them and the pod is BestEffort. The claim here takes the whole 6-CPU device and the container sets no resources, so the engine is first to be evicted under memory pressure. requests and limits of cpu: "6" would follow the driver's contract.
| operands) never sit anywhere else — a toleration too *broad* (the | ||
| "tolerates every taint" class of bug) fails here even when the scheduler | ||
| happened to keep the pod on the system node, and so does a device stack | ||
| installing where its driver was never claimed. |
There was a problem hiding this comment.
Every leg sets dra_drivers to every stack its cloud's list carries (dra.cpu and gpu.nvidia.com, plus gpu.amd.com on Existing). So each leg installs the same components an unfiltered stack would, and a join that ignored draDrivers would pass all seven. The unit tests cover exclusion. Either reword this to match, or add a leg that leaves a driver out, such as Existing without gpu.amd.com, and assert the AMD DaemonSets are absent.
| components = stacks.join( | ||
| self.xr.spec.cloud, | ||
| self.xr.spec.stack or "Standard", | ||
| dra_drivers=None if self.xr.spec.draDrivers is None else [d.root for d in self.xr.spec.draDrivers], |
There was a problem hiding this comment.
Given we don't need to worry about backward compatibility yet, could we avoid this shim? compose-inference-cluster always sets spec.draDrivers, so I think an unset field could mean the same as []: install only the untagged components.
| dra_drivers=None if self.xr.spec.draDrivers is None else [d.root for d in self.xr.spec.draDrivers], | |
| dra_drivers=[d.root for d in self.xr.spec.draDrivers or []], |
We'd then drop the "draDrivers absent" sentences from the comment above, "When the field is absent no filtering happens..." from the XRD's field description, and the matching sentences in join()'s docstring and in compose_serving_stack's docstring in compose-inference-cluster.
| return | ||
|
|
||
| source = cluster.source | ||
| if not self.check_devices_supported(source): |
There was a problem hiding this comment.
Won't returning here delete a running cluster? We compose only the guard and namespaces when we gate, so on a cluster that's already up I think the cloud cluster XR, ServingStack and Usages all drop out of desired state, and Crossplane garbage collects them. That's fine for a new cluster, but adding a pool that references an AMD class to a running Vultr cluster would tear it down. So would editing a shared class to claim an unsupported driver, for every cluster that references it.
Could we set the condition without returning, and just leave the offending pool out of what we compose? It'd be good to have a test case with observed resources covering this too.
| # label and taint composed by compose-gke-cluster. | ||
| node_pool = gkev1alpha1.NodePool( | ||
| name=pool.name, | ||
| role="GPU" if prov.accelerator else "CPU", |
There was a problem hiding this comment.
What happens if a GPU class leaves out accelerator? It used to fail validation, because the field was required, but now I think the pool is composed as CPU: the modelplane.ai/cpu taint, and none of the GPU labels or taint. On Civo, for example, the NFD worker and GPU operator DaemonSets only tolerate nvidia.com/gpu, so I don't think they'd reach the node. Does something catch this case, or would the class's claims fail to allocate?
If it's a problem, one option would be to derive the role from the class's claimed driver instead: CPU when it claims dra.cpu, GPU when it claims a GPU driver, and reject a mismatch alongside the existing InvalidNodePool check. (Here and in the other five builders.)
| icon: brand-civo | ||
| - name: Bring your own | ||
| term: Existing | ||
| accelerators: |
There was a problem hiding this comment.
Do you have a plan for the release-* docs builds here? I think docs-site builds every content branch with one shared theme, so once the theme reads accelerators, the release branches (still on gpus) would lose their GPU memory and chips.
There was a problem hiding this comment.
Yes will Open a PR for this
| catches a generator allowlist that dropped something another | ||
| component needs. | ||
| component needs - and on a dependency that is neither untagged nor | ||
| tagged with every driver its dependent carries, which filtering |
There was a problem hiding this comment.
Nit: This rule, that a dependency must be untagged or carry every driver its dependent does, is also spelled out in components.py's docstring, the comment further down in join(), and twice in generate.py. The Existing rule (a claimed driver the list doesn't tag installs nothing and is never rejected) is similar: it's in five places across this function and compose-inference-cluster. Could we state each once, here where join() enforces them, and point to it from the others? Rationale that's restated in several places tends to drift as the code changes.
| the GCP machine's GPU count and need not be restated in | ||
| devices. | ||
| devices. Omit for a pool with no GPU, which is | ||
| composed with role CPU: a standard node image and the |
There was a problem hiding this comment.
Nit: InferenceClass has no role field, so "composed with role CPU" leans on an internal detail a kubectl explain reader can't see. Maybe "Omit for a pool with no GPU. Modelplane provisions it with a standard node image and the modelplane.ai/cpu label and taint."? (Here and in the other five providers.)
| # that claims a different unit, so each says so. Junction temperature is the | ||
| # one the Instinct MI3xx series reports (MI2xx reports edge instead); no AMD | ||
| # series measures what DCGM's tensor-pipe activity does, so | ||
| # modelplane_gpu_tensor_active_ratio stays NVIDIA-only. |
There was a problem hiding this comment.
The device-metrics-exporter does list GPU_PROF_TENSOR_ACTIVE_PERCENT ("MFMA Utilization Unit percent"), though as a profiler metric it's off unless ProfilerMetrics is enabled. Is it close enough to DCGM's tensor-pipe activity to map? If not, could the comment say why, rather than that no AMD series measures it?
Description of your changes
Design: #432
The serving stack installed NVIDIA's device stack on every cluster and no other vendor's anywhere: every component list pinned the NVIDIA operator or DRA driver unconditionally, nothing checked that the devices a class declares match the stack its cluster gets, and a pool without a GPU couldn't be provisioned at all.
This implements the multi-accelerator design. The cluster composition collects the DRA drivers claimed across a cluster's referenced classes (
claim: DRAonly; Synthetic devices describe placement, not a stack) ontoServingStack spec.draDrivers; components carry the drivers they serve asdra_driverstags - hand-written in place, fail-closed throughALLOWfor the generated clouds and the join installs a tagged component only when one of its drivers is claimed. The supported-drivers table is derived from the tags (plus EKS's declared-externaldra.net), mirrored by hand incompose-inference-cluster, which gates
ClusterReady=False / UnsupportedDevicesbefore anything is provisioned.On top of the mechanism:
CPU inference (Docs: Add design for CPU inference class support #509, rebased onto the mechanism): dra-driver-cpu is one chart tagged
dra.cpu, pinned once in common.py for every cloud including Existing, with scoped tolerations,acceleratorbecomes optional in every provisioning block; a class without one composes aCPU-role pool with themodelplane.ai/cpulabel and taint on all six cloudsstatus.gpuPoolsbecomesstatus.schedulablePools: nothing in it was GPU-specific.AMD on BYO clusters: the AMD GPU operator (DRA mode, driver management off) is pinned on the Existing list tagged
gpu.amd.com- the pin validated on 8x MI325X bare metal in Provision Vultr bare metal servers into k3s inference clusters, composing multi-vendor GPU serving stacks #434, which rebases onto this mechanism. NFD carries both GPU drivers' tags there; engine pods tolerate the per-driver taint list (the pool nodeSelector is what places them).The cloud e2e legs shape one KWOK pool per claimed device stack, an NVIDIA and a CPU pool on every leg, an AMD pool on Existing and stamp
spec.draDrivers, so the placement and toleration audits now check the filter itself: each stack must reach its own pool and stay off the othersAMD GPU metrics: the collector's GPU job also discovers the AMD operator's device-metrics-exporter (label
metrics-exporter, portexporter-port), and amodelplane-gpu-amdbuiltin maps its series onto the samemodelplane_gpu_*names - VRAM MiB to bytes, activity percent to ratio, and energy through a newMicrojoulesfromUnit.modelplane_gpu_tensor_active_ratiostays NVIDIA-only.Two recipes: Qwen2.5-0.5B on a single MI325X on a bring-your-own cluster and Qwen2.5-0.5B on CPUs alone on a Civo
g4p.kube.mediumpool with vLLM's CPU image.The recipe site needs two follow-ups in its repo: the
gpusdata key the ayouts read is nowaccelerators, andpartials/icons.htmlneedsbrand-amd,brand-civoandcpusymbols. https://github.com/modelplaneai/docs-site/blob/main/themes/geekboot/layouts/partials/icons.htmlI have:
nix flake check(or./nix.sh flake check) and made sure it passes.git commit -s.