Merge pull request #332 from woolcoxm/fix/auto-tier-single-core

resource_plan: fix --auto-tier pinning decode to one core (#325)

c/resource_plan.py

@@ -166,14 +166,33 @@ def discover_gpus():
     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():
+    """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":
-        # 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:
             import ctypes
             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)
             k32.GetLogicalProcessorInformationEx(0, None, ctypes.byref(need))
             buf = (ctypes.c_char * need.value)()
@@ -189,18 +208,69 @@ def physical_cpu_count():
                     off += size
                 if 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:
+        # Ask lscpu for exactly core,socket and dedupe on (core, socket).
+        # Counting un-deduplicated rows would return logical threads (SMT),
+        # which was the original over-subscription bug. Empty fields ("-")
+        # mark an offline core/socket and fail int() -> skipped.
+        #
+        # Column layout robustness: `lscpu -p=<list>` emits *exactly* the
+        # requested columns (no CPU prefix), while bare `lscpu -p` prepends
+        # CPU. We requested two columns, but take the LAST TWO fields so the
+        # parser stays correct whether or not a CPU column is present
+        # (JustVugg review: the previous fields[1]/fields[2] indexing assumed
+        #  a 3-column layout and regressed 2-column output to the logical
+        # count -- the opposite of the fix).
         result = subprocess.run(["lscpu", "-p=core,socket"], text=True,
                                 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) < 2:
+                continue
+            try:
+                core, socket = int(fields[-2]), int(fields[-1])
+            except ValueError:
+                continue  # "-" for an offline core/socket
+            cores.add((core, socket))
         if 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():
@@ -366,7 +436,7 @@ def build_plan(model, ram_gb=0, context=4096, gpu_indices=None, vram_gb=0,
         "policy": {"name": policy, **POLICIES[policy],
                    "quality_preserving": policy != "experimental-fast"},
         "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)),
                 "thread_policy": "physical-cores"},
         "tiers": {
@@ -398,11 +468,18 @@ def environment_for_plan(plan, env=None, cuda_enabled=True):
     result = dict(env or {})
     result.setdefault("COLI_POLICY", plan["policy"]["name"])
     result.setdefault("OMP_NUM_THREADS", str(plan["cpu"]["physical_cores"]))
-    if sys.platform != "win32":
+    # NOTE: we intentionally do NOT set OMP_PROC_BIND / OMP_PLACES here.
-        # la libgomp di MinGW non supporta l'affinity su Windows
+    # The engine's own hot-thread tuning (glm.c main(), the COLI_OMP_TUNED
-        # ("Affinity not supported on this configuration"): non impostarle li'.
+    # self-exec) sets OMP_PROC_BIND=close with overwrite=0 -- it prefers
-        result.setdefault("OMP_PROC_BIND", "spread")
+    # packing the team onto adjacent cores for the tiny back-to-back per-expert
-        result.setdefault("OMP_PLACES", "cores")
+    # matmuls. Pre-setting OMP_PROC_BIND=spread here ran first and won (the
+    # engine's overwrite=0 setenv could not override an already-set var), and
+    # spread + OMP_PLACES=cores collapsed the team to one CPU on some libgomp /
+    # multi-socket topologies (#325: --auto-tier pinned decode to 1 core on a
+    # 64-core box even with OMP_NUM_THREADS=64). Leaving affinity to the engine
+    # makes --auto-tier match the plain (working) path. A user who wants a
+    # specific policy can still set OMP_PROC_BIND/OMP_PLACES in the environment
+    # themselves -- setdefault above only covers OMP_NUM_THREADS.
     tune = plan.get("tune", {})
     for key, entry in tune.items():
         if key.startswith("_"):

c/tests/test_resource_plan.py

@@ -5,6 +5,7 @@ import sys
 import tempfile
 import unittest
 from pathlib import Path
+from unittest import mock
 
 from resource_plan import (
     GB,
@@ -14,6 +15,7 @@ from resource_plan import (
     environment_for_plan,
     format_plan,
     memory_available,
+    physical_cpu_count,
 )
 
 
@@ -87,6 +89,79 @@ class ResourcePlanTest(unittest.TestCase):
         self.assertIn("clamped", plan["warnings"][0])
         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.
+
+        # The parser must return 12 physical cores under BOTH lscpu layouts:
+        #  - 2-col: `lscpu -p=core,socket` emits exactly [core,socket] (this is
+        #           what the probe actually requests; the previous fields[1]/[2]
+        #           indexing skipped every line here and fell through to the
+        #           logical count -> the regression JustVugg caught).
+        #  - 3-col: bare `lscpu -p` prepends a CPU column -> [cpu,core,socket].
+        # Taking the last two fields is correct in both cases.
+        layouts = {
+            "2-col (-p=core,socket)": (
+                "# core,socket\n" +
+                "\n".join(f"{core},0" for core in range(12) for _ in range(2))),
+            "3-col (bare -p, CPU prefix)": (
+                "# CPU,Core,Socket\n" +
+                "\n".join(f"{cpu},{core},0" for core in range(12) for cpu in range(2))),
+        }
+        for label, blob in layouts.items():
+            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, label)
+            self.assertEqual(env["OMP_NUM_THREADS"], "12", label)
+
+    def test_plan_does_not_set_omp_affinity_vars(self):
+        # The real #325 regression: --auto-tier set OMP_PROC_BIND=spread +
+        # OMP_PLACES=cores, which ran before the engine's overwrite=0 setenv and
+        # so won, collapsing the OpenMP team to one CPU on the reporter's 64-core
+        # Linux box even though OMP_NUM_THREADS was correct. The plan must leave
+        # affinity to the engine's own hot-thread tuning (which prefers 'close').
+        plan = build_plan(self.model, available_memory=16 * GB,
+                          available_disk=1, gpus=[], physical_cpus=64)
+        env = environment_for_plan(plan)
+        self.assertEqual(env["OMP_NUM_THREADS"], "64")
+        self.assertNotIn("OMP_PROC_BIND", env)
+        self.assertNotIn("OMP_PLACES", env)
+
+    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):
         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,
@@ -117,13 +192,13 @@ class ResourcePlanTest(unittest.TestCase):
         self.assertEqual(env["COLI_CUDA"], "1")
         self.assertEqual(env["COLI_GPUS"], "1")
         self.assertEqual(env["OMP_NUM_THREADS"], str(plan["cpu"]["physical_cores"]))
-        if sys.platform == "win32":
+        # The plan must NOT set OMP_PROC_BIND / OMP_PLACES on any platform:
-            # MinGW libgomp: niente affinity su Windows, le chiavi non vanno emesse
+        # the engine's own hot-thread tuning owns affinity (it prefers
+        # OMP_PROC_BIND=close for the back-to-back per-expert matmuls). Setting
+        # spread + cores here ran before the engine's overwrite=0 setenv and so
+        # won, collapsing the team to one CPU on some libgomp topologies (#325).
         self.assertNotIn("OMP_PROC_BIND", env)
         self.assertNotIn("OMP_PLACES", env)
-        else:
-            self.assertEqual(env["OMP_PROC_BIND"], "spread")
-            self.assertEqual(env["OMP_PLACES"], "cores")
         self.assertEqual(env["PIN_GB"], env["CUDA_EXPERT_GB"])
 
         explicit_threads = environment_for_plan(plan, {"OMP_NUM_THREADS": "7",
@@ -253,5 +328,73 @@ class ResourcePlanTest(unittest.TestCase):
         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__":
     unittest.main()