resource_plan: fix --auto-tier pinning decode to one core (#325)
physical_cpu_count() silently returned 1 in two situations, and that value
flowed through build_plan -> OMP_NUM_THREADS to pin every matmul region to a
single thread under --auto-tier (reported on Rocky Linux 9, 512 GB RAM).
Two root causes:
1. The lscpu parse counted the wrong thing. `lscpu -p=core,socket` prepends the
CPU column, so the output is actually CPU,Core,Socket; the old set
comprehension collected (CPU,core,socket) tuples that were unique per logical
CPU. Now parse CPU,Core,Socket and dedupe on (core, socket) to get true
physical cores (the SMT-doubling the surrounding comments warn against was
the actual behavior).
2. Any probe failure fell through to `os.cpu_count() or 1`. On a cgroup'd or
otherwise constrained box os.cpu_count() can be 1 (or None), silently
capping the run. Skip offline core/socket fields ("-" instead of raising
ValueError) so a single offline row no longer discards the whole parse, and
replace the silent `or 1` fallback with os.cpu_count() (logical) plus an
explicit warning. Only return 1 when nothing at all is detected, and warn.
Also harden the win32 branch: declare argtypes/restype on
GetLogicalProcessorInformationEx (an undeclared 64-bit WinAPI returns c_int and
takes c_int pointers, so the probe could silently fail), and warn on its
fallbacks. Replace the silent max(1, int()) clamp in build_plan with
_resolve_physical_cores() that clamps to 1 with a warning instead of masking.
Tests: the existing OMP_NUM_THREADS test passed physical_cpus=24 explicitly, so
it never exercised the real probe -- that is why this regressed. Add regression
coverage for the lscpu physical-core dedup, offline fields, lscpu-missing
fallback, zero-cores degenerate case, and an end-to-end build_plan +
environment_for_plan check that OMP_NUM_THREADS reflects physical (not logical)
cores.
This commit is contained in:
+75
-12
@@ -166,14 +166,33 @@ def discover_gpus():
|
|||||||
return devices
|
return devices
|
||||||
|
|
||||||
|
|
||||||
|
def _physical_cores_warn(message):
|
||||||
|
"""Visibility for a mis-detected core count: a silent "1" here becomes
|
||||||
|
OMP_NUM_THREADS=1 and pins the whole run to a single core (#325). Emit on
|
||||||
|
stderr so it surfaces in the [PLAN]/[OMP] stream without being swallowed."""
|
||||||
|
print(f"[plan] warning: {message}", file=sys.stderr)
|
||||||
|
|
||||||
|
|
||||||
def physical_cpu_count():
|
def physical_cpu_count():
|
||||||
|
"""Number of physical CPU cores (not SMT siblings).
|
||||||
|
|
||||||
|
Per-expert matmul regions are tiny and back-to-back; two SMT siblings share
|
||||||
|
one AVX-512 unit and contend, so logical (SMT) counts over-subscribe and
|
||||||
|
hurt throughput. We want true physical cores. A silent 1 here propagates to
|
||||||
|
OMP_NUM_THREADS=1 and pins the run to one core (#325), so every fallback
|
||||||
|
must be visible, never just ``or 1``.
|
||||||
|
"""
|
||||||
if sys.platform == "win32":
|
if sys.platform == "win32":
|
||||||
# os.cpu_count() conta i processori logici (SMT): 2 thread/core saturano
|
# Contiamo i core fisici veri con GetLogicalProcessorInformationEx
|
||||||
# le unita' AVX-512 e peggiorano il matmul. Contiamo i core fisici veri
|
# (RelationProcessorCore). Le firme vanno dichiarate: su Python a 64 bit
|
||||||
# con GetLogicalProcessorInformationEx(RelationProcessorCore).
|
# una WinAPI non dichiarata ritorna c_int (32 bit) e riceve i puntatori
|
||||||
|
# come c_int di default, quindi il probe puo' fallire silenziosamente.
|
||||||
try:
|
try:
|
||||||
import ctypes
|
import ctypes
|
||||||
k32 = ctypes.windll.kernel32
|
k32 = ctypes.windll.kernel32
|
||||||
|
k32.GetLogicalProcessorInformationEx.argtypes = [
|
||||||
|
ctypes.c_uint, ctypes.c_void_p, ctypes.POINTER(ctypes.c_ulong)]
|
||||||
|
k32.GetLogicalProcessorInformationEx.restype = ctypes.c_int
|
||||||
need = ctypes.c_ulong(0)
|
need = ctypes.c_ulong(0)
|
||||||
k32.GetLogicalProcessorInformationEx(0, None, ctypes.byref(need))
|
k32.GetLogicalProcessorInformationEx(0, None, ctypes.byref(need))
|
||||||
buf = (ctypes.c_char * need.value)()
|
buf = (ctypes.c_char * need.value)()
|
||||||
@@ -189,18 +208,62 @@ def physical_cpu_count():
|
|||||||
off += size
|
off += size
|
||||||
if cores:
|
if cores:
|
||||||
return cores
|
return cores
|
||||||
except (OSError, ValueError, AttributeError):
|
_physical_cores_warn("GetLogicalProcessorInformationEx returned no cores")
|
||||||
pass
|
except (OSError, ValueError, AttributeError) as error:
|
||||||
|
_physical_cores_warn(f"Windows core probe failed: {error}")
|
||||||
try:
|
try:
|
||||||
result = subprocess.run(["lscpu", "-p=core,socket"], text=True,
|
# lscpu -p prepende SEMPRE la colonna CPU, quindi `-p=core,socket` emette
|
||||||
|
# in realta' "CPU,Core,Socket" (3 colonne). Dobbiamo leggere esplicitamente
|
||||||
|
# CPU/Core/Socket e deduplicare su (core, socket): contare le righe non
|
||||||
|
# deduplicate restituirebbe i thread logici (SMT) - l'errore originale.
|
||||||
|
# I campi vuoti ("-") marcano core/socket offline e non sono interi.
|
||||||
|
result = subprocess.run(["lscpu", "-p=CPU,Core,Socket"], text=True,
|
||||||
capture_output=True, check=True, timeout=5)
|
capture_output=True, check=True, timeout=5)
|
||||||
cores = {tuple(map(int, line.split(","))) for line in result.stdout.splitlines()
|
cores = set()
|
||||||
if line and not line.startswith("#")}
|
for line in result.stdout.splitlines():
|
||||||
|
if not line or line.startswith("#"):
|
||||||
|
continue
|
||||||
|
fields = line.split(",")
|
||||||
|
if len(fields) < 3:
|
||||||
|
continue
|
||||||
|
try:
|
||||||
|
core, socket = int(fields[1]), int(fields[2])
|
||||||
|
except ValueError:
|
||||||
|
continue # "-" per un core/socket offline
|
||||||
|
cores.add((core, socket))
|
||||||
if cores:
|
if cores:
|
||||||
return len(cores)
|
return len(cores)
|
||||||
except (OSError, ValueError, subprocess.SubprocessError):
|
except (OSError, ValueError, subprocess.SubprocessError) as error:
|
||||||
pass
|
_physical_cores_warn(f"lscpu core probe failed: {error}")
|
||||||
return os.cpu_count() or 1
|
logical = os.cpu_count()
|
||||||
|
if not logical:
|
||||||
|
_physical_cores_warn(
|
||||||
|
"could not detect any CPU cores; falling back to 1. "
|
||||||
|
"Set OMP_NUM_THREADS manually to fix single-core decode (#325).")
|
||||||
|
return 1
|
||||||
|
_physical_cores_warn(
|
||||||
|
f"physical-core probes unavailable; using {logical} logical CPUs "
|
||||||
|
f"(SMT may over-subscribe). Set OMP_NUM_THREADS to physical cores if slow.")
|
||||||
|
return logical
|
||||||
|
|
||||||
|
|
||||||
|
def _resolve_physical_cores(physical_cpus):
|
||||||
|
"""Coerce the build_plan() physical-core argument to a sane positive int.
|
||||||
|
|
||||||
|
A None/0/None-ish value reaching here means physical_cpu_count() already
|
||||||
|
warned; clamp to 1 (so the engine always gets a positive team size) but keep
|
||||||
|
that clamp visible rather than silently masking it as the old ``max(1, int())``
|
||||||
|
did (#325)."""
|
||||||
|
try:
|
||||||
|
count = int(physical_cpus or 0)
|
||||||
|
except (TypeError, ValueError):
|
||||||
|
count = 0
|
||||||
|
if count < 1:
|
||||||
|
_physical_cores_warn(
|
||||||
|
"physical core count resolved to 0; defaulting to 1. "
|
||||||
|
"Set OMP_NUM_THREADS to fix single-core decode (#325).")
|
||||||
|
return 1
|
||||||
|
return count
|
||||||
|
|
||||||
|
|
||||||
def cpu_socket_count():
|
def cpu_socket_count():
|
||||||
@@ -366,7 +429,7 @@ def build_plan(model, ram_gb=0, context=4096, gpu_indices=None, vram_gb=0,
|
|||||||
"policy": {"name": policy, **POLICIES[policy],
|
"policy": {"name": policy, **POLICIES[policy],
|
||||||
"quality_preserving": policy != "experimental-fast"},
|
"quality_preserving": policy != "experimental-fast"},
|
||||||
"model": {key: value for key, value in info.items() if key != "config"},
|
"model": {key: value for key, value in info.items() if key != "config"},
|
||||||
"cpu": {"physical_cores": max(1, int(physical_cpus)),
|
"cpu": {"physical_cores": _resolve_physical_cores(physical_cpus),
|
||||||
"sockets": max(1, int(cpu_sockets)),
|
"sockets": max(1, int(cpu_sockets)),
|
||||||
"thread_policy": "physical-cores"},
|
"thread_policy": "physical-cores"},
|
||||||
"tiers": {
|
"tiers": {
|
||||||
|
|||||||
@@ -5,6 +5,7 @@ import sys
|
|||||||
import tempfile
|
import tempfile
|
||||||
import unittest
|
import unittest
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
from unittest import mock
|
||||||
|
|
||||||
from resource_plan import (
|
from resource_plan import (
|
||||||
GB,
|
GB,
|
||||||
@@ -14,6 +15,7 @@ from resource_plan import (
|
|||||||
environment_for_plan,
|
environment_for_plan,
|
||||||
format_plan,
|
format_plan,
|
||||||
memory_available,
|
memory_available,
|
||||||
|
physical_cpu_count,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -87,6 +89,51 @@ class ResourcePlanTest(unittest.TestCase):
|
|||||||
self.assertIn("clamped", plan["warnings"][0])
|
self.assertIn("clamped", plan["warnings"][0])
|
||||||
self.assertIn("0:test-gpu", format_plan(plan))
|
self.assertIn("0:test-gpu", format_plan(plan))
|
||||||
|
|
||||||
|
def test_auto_tier_thread_count_uses_physical_cores(self):
|
||||||
|
# End-to-end for #325: build_plan + environment_for_plan must export the
|
||||||
|
# physical (not logical SMT) core count as OMP_NUM_THREADS. The original
|
||||||
|
# suite passed physical_cpus=24 explicitly, so it never exercised the
|
||||||
|
# real physical_cpu_count() probe whose single-core failure pinned decode.
|
||||||
|
def lscpu(stdout):
|
||||||
|
return subprocess.CompletedProcess(args=[], returncode=0,
|
||||||
|
stdout=stdout, stderr="")
|
||||||
|
# 1 socket, 12 cores, 2 SMT siblings -> 24 threads, 12 physical cores.
|
||||||
|
# lscpu prepends the CPU column, so output is CPU,Core,Socket.
|
||||||
|
rows = [f"{cpu},{core},0" for core in range(12) for cpu in range(2)]
|
||||||
|
blob = "# CPU,Core,Socket\n" + "\n".join(rows)
|
||||||
|
with mock.patch("resource_plan.subprocess.run", return_value=lscpu(blob)), \
|
||||||
|
mock.patch.object(sys, "platform", "linux"):
|
||||||
|
plan = build_plan(self.model, available_memory=16 * GB,
|
||||||
|
available_disk=1, gpus=[])
|
||||||
|
env = environment_for_plan(plan)
|
||||||
|
self.assertEqual(plan["cpu"]["physical_cores"], 12)
|
||||||
|
self.assertEqual(env["OMP_NUM_THREADS"], "12")
|
||||||
|
|
||||||
|
def test_plan_conserves_budget_and_experts_above_256gb(self):
|
||||||
|
# Regression for #325's reporter: a 512 GB machine loading the whole
|
||||||
|
# model into RAM. Verify the budget math stays exact at large RAM sizes
|
||||||
|
# (no integer truncation, no over-allocation, no experts lost between
|
||||||
|
# tiers). Checked at 256/512/800 GB to bracket the reporter's box.
|
||||||
|
for ram_gb in (256, 512, 800):
|
||||||
|
plan = build_plan(self.model, ram_gb=ram_gb, available_disk=1,
|
||||||
|
gpus=[], physical_cpus=64)
|
||||||
|
ram = plan["tiers"]["ram"]
|
||||||
|
# RAM budget never over-allocated: dense + runtime + cache <= budget.
|
||||||
|
allocated = (ram["dense_bytes"] + ram["runtime_bytes"]
|
||||||
|
+ ram["expert_cache_bytes"])
|
||||||
|
self.assertLessEqual(allocated, ram["budget_bytes"],
|
||||||
|
f"over-allocated RAM at {ram_gb} GB")
|
||||||
|
# Every expert byte is accounted for exactly once across the tiers.
|
||||||
|
tiers = plan["tiers"]
|
||||||
|
tiered = (tiers["vram"]["hot_expert_bytes"]
|
||||||
|
+ ram["warm_expert_bytes"]
|
||||||
|
+ tiers["disk"]["cold_expert_bytes"])
|
||||||
|
self.assertEqual(tiered, plan["model"]["expert_bytes"],
|
||||||
|
f"expert bytes lost/duplicated at {ram_gb} GB")
|
||||||
|
# A positive RAM budget yields a non-negative cache and a sensible cap.
|
||||||
|
self.assertGreaterEqual(ram["expert_cache_bytes"], 0)
|
||||||
|
self.assertGreaterEqual(ram["cache_slots_per_layer"], 0)
|
||||||
|
|
||||||
def test_filters_requested_devices(self):
|
def test_filters_requested_devices(self):
|
||||||
gpus = [{"index": 0, "name": "a", "total_bytes": 8 * GB, "free_bytes": 8 * GB}]
|
gpus = [{"index": 0, "name": "a", "total_bytes": 8 * GB, "free_bytes": 8 * GB}]
|
||||||
plan = build_plan(self.model, available_memory=16 * GB, available_disk=1,
|
plan = build_plan(self.model, available_memory=16 * GB, available_disk=1,
|
||||||
@@ -253,5 +300,73 @@ class ResourcePlanTest(unittest.TestCase):
|
|||||||
self.assertIn("expected_bottleneck", plan)
|
self.assertIn("expected_bottleneck", plan)
|
||||||
|
|
||||||
|
|
||||||
|
class PhysicalCpuCountTest(unittest.TestCase):
|
||||||
|
"""Regression for #325: --auto-tier pinned decode to one core because
|
||||||
|
physical_cpu_count() silently returned 1.
|
||||||
|
|
||||||
|
Two root causes this locks down:
|
||||||
|
1. lscpu -p prepends a CPU column, so `-p=core,socket` emits
|
||||||
|
CPU,Core,Socket; counting rows counted logical SMT siblings.
|
||||||
|
2. any probe failure fell through to ``os.cpu_count() or 1`` and the
|
||||||
|
``or 1`` could pin a constrained/cgroup'd box to a single core.
|
||||||
|
"""
|
||||||
|
|
||||||
|
def _lscpu(self, stdout):
|
||||||
|
return subprocess.CompletedProcess(args=[], returncode=0,
|
||||||
|
stdout=stdout, stderr="")
|
||||||
|
|
||||||
|
def _lscpu_topology(self, sockets, cores_per_socket, threads_per_core):
|
||||||
|
# Real lscpu shape: socket-local core IDs repeat across sockets; the
|
||||||
|
# CPU column (always prepended) is a unique logical-CPU index.
|
||||||
|
rows, cpu = [], 0
|
||||||
|
for sock in range(sockets):
|
||||||
|
for core in range(cores_per_socket):
|
||||||
|
for _ in range(threads_per_core):
|
||||||
|
rows.append(f"{cpu},{core},{sock}")
|
||||||
|
cpu += 1
|
||||||
|
return "# CPU,Core,Socket\n" + "\n".join(rows)
|
||||||
|
|
||||||
|
def test_counts_physical_cores_not_smt_threads(self):
|
||||||
|
blob = self._lscpu_topology(sockets=2, cores_per_socket=16, threads_per_core=2)
|
||||||
|
with mock.patch("resource_plan.subprocess.run", return_value=self._lscpu(blob)), \
|
||||||
|
mock.patch.object(sys, "platform", "linux"):
|
||||||
|
self.assertEqual(physical_cpu_count(), 32)
|
||||||
|
|
||||||
|
def test_single_socket_no_smt(self):
|
||||||
|
blob = self._lscpu_topology(sockets=1, cores_per_socket=8, threads_per_core=1)
|
||||||
|
with mock.patch("resource_plan.subprocess.run", return_value=self._lscpu(blob)), \
|
||||||
|
mock.patch.object(sys, "platform", "linux"):
|
||||||
|
self.assertEqual(physical_cpu_count(), 8)
|
||||||
|
|
||||||
|
def test_skips_offline_core_socket_fields(self):
|
||||||
|
# VMs / large NUMA boxes emit "-" for offline core or socket IDs; that
|
||||||
|
# used to raise ValueError, discard the whole parse, and fall through
|
||||||
|
# to the single-core fallback.
|
||||||
|
blob = "# CPU,Core,Socket\n0,0,0\n1,-,0\n2,1,0\n3,1,0\n"
|
||||||
|
with mock.patch("resource_plan.subprocess.run", return_value=self._lscpu(blob)), \
|
||||||
|
mock.patch.object(sys, "platform", "linux"):
|
||||||
|
self.assertEqual(physical_cpu_count(), 2)
|
||||||
|
|
||||||
|
def test_lscpu_missing_falls_back_to_logical_not_silent_one(self):
|
||||||
|
# The bug: lscpu absent -> os.cpu_count() or 1. On a constrained box
|
||||||
|
# os.cpu_count() can be 1. We still must never silently pick 1 without
|
||||||
|
# a warning, and when logical cores exist they must be used.
|
||||||
|
import os
|
||||||
|
with mock.patch("resource_plan.subprocess.run", side_effect=FileNotFoundError), \
|
||||||
|
mock.patch.object(sys, "platform", "linux"), \
|
||||||
|
mock.patch("resource_plan.os.cpu_count", return_value=16), \
|
||||||
|
mock.patch("sys.stderr"):
|
||||||
|
self.assertEqual(physical_cpu_count(), 16)
|
||||||
|
|
||||||
|
def test_zero_logical_cores_warns_and_returns_one(self):
|
||||||
|
# The genuine degenerate case: no probe works and os.cpu_count() is
|
||||||
|
# None/1. Must return 1 (engine needs a positive team size) but warn.
|
||||||
|
with mock.patch("resource_plan.subprocess.run", side_effect=FileNotFoundError), \
|
||||||
|
mock.patch.object(sys, "platform", "linux"), \
|
||||||
|
mock.patch("resource_plan.os.cpu_count", return_value=None), \
|
||||||
|
mock.patch("sys.stderr"):
|
||||||
|
self.assertEqual(physical_cpu_count(), 1)
|
||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|||||||
Reference in New Issue
Block a user