refactor: 🧹 use hwloc instead of custom topology discovery - #208
Conversation
|
Docs preview: https://pr-208.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #208 +/- ##
=======================================
Coverage 97.70% 97.70%
=======================================
Files 14 14
Lines 742 742
Branches 98 98
=======================================
Hits 725 725
Misses 12 12
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Pull request overview
🤖 AI text below 🤖
Refactors the CPU-topology discovery and partition thread pinning in monoprop to use hwloc, aiming for more portable and consistent topology enumeration and affinity binding across supported platforms.
Changes:
- Replace the custom
/sys-parsing topology logic with anhwloc-backed implementation (CpuTopology.*) and expose a deterministic placement-policy helper for unit testing. - Integrate
hwlocinto the C++ build (CMake + pkg-config) and update C++ unit tests accordingly. - Update developer tooling and CI environments to install
hwloc(scripts, workflows, devcontainer) and refresh related documentation.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/install-deps.sh | Adds optional hwloc installation and broadens OS detection for the dependency installer script. |
| pyproject.toml | Updates macOS cibuildwheel pre-build dependencies to include hwloc. |
| docs/content/docs/features/parallelism.mdx | Updates documentation to reflect hwloc-based pinning support beyond Linux. |
| cpp/tests/cpu_topology_tests.cpp | Reworks CPU-topology tests for hwloc and adds deterministic placement-policy unit tests. |
| cpp/tests/CMakeLists.txt | Links unit tests against hwloc via PkgConfig::HWLOC. |
| cpp/monoprop/detail/partition/CpuTopology.h | Updates the public topology/pinning API docs and types for an hwloc implementation. |
| cpp/monoprop/detail/partition/CpuTopology.cpp | Implements topology discovery and thread pinning via hwloc, plus shared placement-policy logic. |
| cpp/monoprop/CMakeLists.txt | Adds pkg-config discovery for hwloc and wires PkgConfig::HWLOC into library linkage. |
| AGENTS.md | Documents hwloc as a required system dependency and where it’s used. |
| .github/workflows/test.yml | Ensures CI runners install hwloc on macOS and Ubuntu. |
| .github/workflows/qa-analysis.yml | Adds libhwloc-dev to QA workflow dependency installs. |
| .github/workflows/docpages.yml | Adds libhwloc-dev to docs build workflow dependency installs. |
| .github/workflows/copilot-setup-steps.yml | Adds libhwloc-dev to Copilot setup steps. |
| .devcontainer/Dockerfile | Installs libhwloc-dev in the devcontainer image. |
Suppressed comments (1)
tools/install-deps.sh:217
- The script advises setting only
CMAKE_PREFIX_PATH, but the build now discovers hwloc viapkg-config(pkg_check_modules(HWLOC …)), which typically requiresPKG_CONFIG_PATHto include the install prefix when using a non-systemINSTALL_PREFIX. Without that, a custom-prefix hwloc installed by this script may still not be found by CMake.
echo "Dependencies installation completed successfully!"
echo "Install location: $INSTALL_PREFIX"
echo "Make sure to set CMAKE_PREFIX_PATH=$INSTALL_PREFIX when building monoprop"
This comment was marked as resolved.
This comment was marked as resolved.
842359c to
f810a4e
Compare
f810a4e to
7a8322a
Compare
7a8322a to
d3c3bee
Compare
aeda5c8 to
8a20dde
Compare
8a20dde to
746e541
Compare
746e541 to
03ce849
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/content/docs/features/parallelism.mdx:32
- 🤖 AI text below 🤖
This incorrectly lists macOS as supporting thread pinning. hwloc 2.13's Darwin backend installs no CPU-binding hooks, so hwloc_set_cpubind fails and this implementation silently leaves workers unpinned. Please document macOS as topology-discovery-only rather than promising binding there.
| `monoprop_PARTITION_PINNING` | `on` | `0`/`false`/`no` disables pinning each partition to a core. Supported on platforms where hwloc can bind threads (Linux, macOS). |
a666c60 to
c672ddf
Compare
ddb10d9 to
797ad0c
Compare
797ad0c to
7a8f169
Compare
Assisted-by: GitHub Copilot: gpt-5.6-sol (plan), claude-haiku-4.5/claude-sonnet-4.6 (execute)
with before-build we re-ran the same installation script before every build.
This reverts commit 3624945.
This reverts commit e5deeb5.
Assisted-by: Copilot:gpt-5.4 Co-authored-by: robertodr <3708689+robertodr@users.noreply.github.com>
7a8f169 to
3fc31ae
Compare
|
Brings in #203 (implementations split into source/inline files), #208 (hwloc replaces the custom topology discovery), #211, #223, #148 and #224. Resolves the conflict that had left this PR with no CI: GitHub cannot recompute refs/pull/166/merge while the merge conflicts, so the pull_request workflows produced no runs at all and only the pull_request_target labeller was firing. Four files conflicted; the rest auto-merged. Two were unions of independent additions (AGENTS.md, parallelism.mdx) -- in the latter, main's wording for the monoprop_PARTITION_PINNING row is now the accurate one, since hwloc replaced the Linux-only /sys path. The substantive resolution is CpuTopology. #208 rewrote discovery onto hwloc, which is the better mechanism and is kept wholesale, but it also reinstated the placement bug this branch exists to fix: enumerate_physical_cores() filters by the calling thread's affinity, and placement_order() then rejects the request when group_count * n > cores.size(), so a rank holding a launcher-assigned slice divides an already-divided machine and silently runs unpinned. That is the mechanism measured at 437 us/sync against 15.5 us/sync placed. So this keeps main's hwloc discovery and re-applies the fix on top of it, rather than keeping either side's file: - NodeMask, classify_node_mask() and this_thread_cpumask() return, with the PerRank collapse moved into partition_cpusets() so placement_order() stays pure and hardware-free the way #208 factored it. - CpuMask replaces cpu_set_t as the exchanged type, since main's CpuSet is now a single PU rather than a mask. It is a fixed-size POD because PartitionGroup ships it through MPI_Allgather as MPI_BYTE, and being hwloc-free makes classify_node_mask() unit-testable without live hardware. 4096 bits rather than glibc's CPU_SETSIZE of 1024: a PU index past the mask is invisible to the disjointness test, which would misread a per-rank split as shared. - pin_this_thread() reports whether the affinity took, which CommProfile's pinned count needs to tell "nothing was pinned" apart from "one domain per rank" when barrier_groups is 0. - cpuset_domains() returns, so the two-level barrier keeps deriving its domains from the placement rather than from the placement logic. Verified on a Deucalion login node, against live hwloc and real launcher bindings, that the collapse is load-bearing and does not disturb the shared case. Same live mask, same call, only the classification differs: 2 ranks, --map-by slot:PE=4 --bind-to core, n=4 => PerRank as Shared (== main's unconditional divide): 0 cores placed as PerRank: 4 cores placed 2 ranks, PE=8, n=8 => PerRank: 0 vs 8 2 ranks, --bind-to none, n=4 => Shared: 4 vs 4 AGENTS.md records the trap under Architecture Overview, including that it has now regressed once, because the guard lives in the placement policy and not in discovery -- so a future rework of that layer has to re-check it. cpu_topology_policy_per_rank_slice_starves_without_collapse pins the mechanism deterministically, without live hardware. Gates on the merged tree (hwloc 2.12.1, already provided transitively by foss/2025b, so hpc/deucalion/env.sh needs no change): - ctest -L serial: 224/224 passed, 0 failed, 412 s - fabric-exclusion scoping still correct: 224/224 serial carry it, 0/1 mpi do - cpu_topology: 14/14 cases, 185 assertions, with per-case assertion counts confirming the two live restricted-mask cases asserted rather than early-returning - rank_count_matches_under_an_asymmetric_emit_gate and zero_cutoff_upper_atol_zero_is_exact_World: pass at 1, 2 and 4 ranks Unchanged by this merge and still open: shm_comm_oversubscribed_repeated_collectives aborts under 2 ranks on main as well as here, so ctest -L mpi remains unusable as a gate; and the layout A build_graph ~1.1x regression is still unexplained. Assisted-by: ClaudeCode:claude-opus-5



Summary
The CPU topology discovery and thread pinning used a custom implementation that was not really portable.
hwlocis the standard tool for this job.Changes
hwlocas dependency for cross-platform CPU topology discovery.Checklist
docs/,CONTRIBUTING.md) if neededCHANGELOG/ release notes updated if applicableAI/LLM disclosure
Important
By opening this PR I confirm that I have read CONTRIBUTING.md and I agree to the terms of the Contributor License Agreement.
Warning
If you're contributing on behalf of your employer, contact cla@algorithmiq.fi to arrange a Corporate CLA.