Repository navigation
Conversation
There was a problem hiding this comment.
I think some design questions need settling before this merges.
The draDrivers mechanism follows design/multi-accelerator.md. Should the CPU inference API land without a design or issue of its own, though? The committed design leaves the CPU-specific parts to #509, which closed without merging. That leaves the optional accelerator, the CPU pool role, the status.schedulablePools rename and the fixed reservedCPUs with no settled design. The PR also adds AMD on Existing, the AMD metrics builtin and two recipes. Could the mechanism merge on its own, with CPU and AMD following once their designs settle? I've held back test and naming nits until that's decided.
CONTRIBUTING's commit messages section asks for capitalized, imperative subjects without a feat(…): prefix and a body on each commit. Please also fold feat(docs): update for vale into the commit it fixes.
| 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? I think if someone edits a class to claim gpu.amd.com on GKE, or mistypes a driver name, the next reconcile drops desired state to the guard and namespaces. Crossplane then garbage collects the GKECluster XR, the ServingStack and the Usages, on every cluster that references the class. Running the GKEReady case with an AMD device added drops gke-cluster from desired resources. Could the gate apply only before the cloud cluster XR exists, or set the condition and keep composing what's already there?
| # 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.
Should a pool's role follow the driver its class claims, rather than whether the class has an accelerator? The design says node taints follow the stack, but devices[].driver never reaches it. A class claiming gpu.nvidia.com without accelerator gets a CPU pool and the modelplane.ai/cpu taint. An AMD class on Vultr would get the nvidia.com/gpu taint. Each new device type needs a role value and a taint branch in six functions, close to the per-device-type field the design rejected. Could the taint come from the claimed driver, or wait for a CPU design? (Here and elsewhere.)
| composition from the claim: DRA devices of the cluster's | ||
| referenced InferenceClasses. A component tagged with a | ||
| driver installs only when that driver is listed; untagged | ||
| components always install. When the field is absent no |
There was a problem hiding this comment.
Does an absent draDrivers need to mean "install everything"? It looks like compose-inference-cluster always sets the list, so the unfiltered path only serves ServingStacks created before the field existed, and Modelplane has none to migrate. What if we required the field, or defaulted it to []? That would remove the None branches in join and compose-serving-stack's fn.py, along with the ExistingStandardUnfiltered case and the comments that explain them.
| GPU pool and a CPU pool on every cloud, plus an AMD GPU pool on | ||
| Existing, the one cloud that installs the AMD stack. The ServingStack | ||
| under test names those stacks' DRA drivers in spec.draDrivers, the way | ||
| compose-inference-cluster stamps them, so the legs exercise the filter: |
There was a problem hiding this comment.
Do the legs exercise the filter? Each leg claims every driver its cloud tags: dra.cpu and gpu.nvidia.com everywhere, plus gpu.amd.com on Existing. I think join never drops a component, so a join that ignored draDrivers would pass all seven legs. Could one leg leave a driver out and assert its stack is absent? Or could this docstring, the README and test_pools_hold_their_own stop claiming the legs test the filter?
| exporter](https://github.com/NVIDIA/dcgm-exporter); on AMD pools it's the AMD | ||
| GPU operator's | ||
| [device-metrics-exporter](https://github.com/ROCm/device-metrics-exporter), | ||
| which the serving stack installs with the AMD stack. The collector discovers |
There was a problem hiding this comment.
Is the "An existing cluster provides its own" bullet below still right for AMD pools? This line says the serving stack installs the AMD exporter, which it does on Existing. But that bullet, the intro at the top of this page and the existing-cluster requirements all say you supply your own exporter. Should they say that only applies to NVIDIA pools?
| template: | ||
| spec: | ||
| containers: | ||
| - name: engine |
There was a problem hiding this comment.
Doesn't the engine need CPU requests that match its claim? dra-driver-cpu's workload requirements say that before KEP-5517 the scheduler and kubelet don't count claimed CPUs, so a pod has to set cpu requests and limits equal to its claim. Without resources, I think the pod runs BestEffort and the scheduler can overcommit the node. Could the recipe set them, or compose-model-replica derive them from a dra.cpu claim?
…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>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
|
/review |
Description of your changes
Design: modelplaneai#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 modelplaneai/modelplane#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 modelplaneai/modelplane#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.html -> modelplaneai/docs-site#5I have:
nix flake check(or./nix.sh flake check) and made sure it passes.git commit -s.