feat(concurrency)!: finalize parallel triangulation support#129
Merged
Conversation
- expose validated --threads limits in cdt and initialize - add the opt-in CGAL/oneTBB build, CI contract, and scaling records - persist thread limits and report source and toolchain provenance - finalize typed seeds, scoped enums, move APIs, and physical parameters - require CMake 4.4+ and enforce schema-12 configure diagnostics BREAKING CHANGE: CMake 4.4.0 or newer is required. The final 1.0 C++ API replaces legacy snake_case types and get_* accessors, separates RandomSeed from RandomStream, removes the MoveTracker template parameter, and requires validated PhysicalParameters for generalized S3 actions. Closes #88
acgetchell
enabled auto-merge
July 24, 2026 23:11
WalkthroughThe change adds opt-in CGAL/oneTBB parallel builds and bounded thread configuration, refreshes public C++ type and accessor contracts, persists thread metadata, strengthens validation and benchmark coverage, and updates CI, documentation, and build tooling. ChangesParallel execution and API contract refresh
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI
participant RuntimeConfig
participant TBB
participant Persistence
CLI->>RuntimeConfig: make_triangulation(..., threads)
RuntimeConfig->>RuntimeConfig: validate thread limit
RuntimeConfig-->>CLI: Triangulation with threads()
CLI->>TBB: set max_allowed_parallelism
CLI->>Persistence: write parallel.max_threads
Persistence-->>CLI: validate persisted metadata
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/CONTRIBUTING.md:
- Around line 57-58: Update the GitHub Actions CI description in CONTRIBUTING.md
to state that Ubuntu GCC, Ubuntu Clang, macOS AppleClang, and Windows MSVC jobs
run both `just ci` and `just build-parallel`, preserving the existing compiler
and LLVM tooling details.
In @.github/workflows/ci.yml:
- Around line 149-155: Update the CI matrix around “Run the opt-in CGAL/oneTBB
contract” so the parallel build runs only on platform/toolchain combinations
where CGAL and oneTBB are reliably available through vcpkg, rather than
unconditionally on all jobs. Preserve the standard reference build/test for
every matrix entry and avoid making unsupported or unavailable parallel
dependencies fail the entire matrix.
In `@CMakeLists.txt`:
- Around line 33-60: Ensure the CDT_SOURCE_REVISION computation is rerun when
Git metadata changes during incremental builds by registering .git/HEAD and
.git/index as CMake configure dependencies. Update the existing Git revision
logic around CDT_SOURCE_REVISION without changing its commit and dirty-state
detection behavior.
In `@include/Foliated_triangulation.hpp`:
- Around line 1049-1062: Update the predicate in find_invalid_timevalue_cells to
compute expected_cell_type<dimension>(cell) once per cell, store the
classification locally, and compare that cached value against ACAUSAL and
UNCLASSIFIED. Preserve the existing filtering behavior and return type.
In `@include/Move_tracker.hpp`:
- Around line 60-79: In generate_random_move_3 and move_from_index, enforce at
compile time that the move lookup array contains exactly NUMBER_OF_3D_MOVES
entries before allowing the optional dereference. Keep the existing distribution
and invalid-index handling unchanged, but make any future mismatch fail
compilation rather than relying on unchecked synchronization.
In `@include/Utilities.hpp`:
- Around line 638-655: Extend Parsed_persistence_metadata to store the validated
parallel.max_threads value, then update read_persistence_metadata to copy the
parsed value into that field when constructing its return object. Preserve the
optional-field behavior by representing an absent thread limit consistently with
the existing metadata parsing conventions, so downstream consumers such as
validate_payload_integrity can retrieve it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54882790-dd48-431b-b0ea-4296f4e1bd0d
📒 Files selected for processing (72)
.clang-format.github/CONTRIBUTING.md.github/workflows/ci.yml.github/workflows/codecov-upload.ymlCMakeLists.txtCMakePresets.jsonJustfileREADME.mdcmake/RunCdtNoOutputTest.cmakecmake/RunPersistenceOutputTest.cmakecmake/Version.hpp.indocs/api-boundary.mddocs/cgal-integration.mddocs/multithreading.mddocs/reproducibility.mdinclude/Apply_move.hppinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Formatters.hppinclude/Manifold.hppinclude/Metropolis.hppinclude/Move_always.hppinclude/Move_command.hppinclude/Move_outcome.hppinclude/Move_run.hppinclude/Move_strategy.hppinclude/Move_tracker.hppinclude/Mpfr_value.hppinclude/Random.hppinclude/Runtime_config.hppinclude/S3Action.hppinclude/Settings.hppinclude/Torus_d.hppinclude/Utilities.hppscripts/build.batscripts/build.shscripts/pkgx-build.shscripts/slurm.shsemgrep.yamlsrc/CMakeLists.txtsrc/cdt-viewer.cppsrc/cdt.cppsrc/initialize.cpptests/Apply_move_test.cpptests/Bistellar_flip_test.cpptests/CGAL_benchmark.cpptests/CGAL_integration_test.cpptests/CMakeLists.txttests/Ergodic_moves_3_audit_test.cpptests/Ergodic_moves_3_test.cpptests/Foliated_triangulation_test.cpptests/Function_ref_test.cpptests/Geometry_test.cpptests/Manifold_test.cpptests/Metropolis_test.cpptests/Move_always_test.cpptests/Move_command_test.cpptests/Move_outcome_test.cpptests/Move_run_test.cpptests/Move_tracker_test.cpptests/Parallel_triangulation_test.cpptests/Public_api_consumer.cpptests/Random_benchmark.cpptests/Random_test.cpptests/Runtime_config_test.cpptests/S3Action_test.cpptests/Settings_test.cpptests/Tetrahedron_test.cpptests/Torus_test.cpptests/Utilities_test.cpptests/Vertex_test.cpptests/semgrep/functional_boundaries.cpp
- Correct pkgx tool-list handling across CI and coverage workflows. - Refresh source revision metadata when the Git state changes. - Preserve optional thread limits when parsing persistence metadata. - Enforce move-table consistency and avoid duplicate cell classification.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
CMakeLists.txt (1)
36-41: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTrack the resolved branch ref and worktree Git directory.
HEADis normally symbolic, so a branch commit updatesrefs/heads/<branch>, not.git/HEAD; linked worktrees also use a.gitfile and skip this block. Resolve paths viagit rev-parse --git-pathand register the resolved branch ref pluspacked-refs, HEAD, and index.#!/usr/bin/env bash set -euo pipefail git rev-parse --is-inside-work-tree git rev-parse --git-path HEAD git rev-parse --git-path index git rev-parse --git-path packed-refs ref="$(git symbolic-ref -q HEAD || true)" if [[ -n "${ref}" ]]; then git rev-parse --git-path "${ref}" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 36 - 41, Update the Git dependency configuration around the CMAKE_CONFIGURE_DEPENDS property to support linked worktrees and symbolic branches: use git rev-parse --git-path to resolve HEAD, index, packed-refs, and the current symbolic HEAD ref, then register the resolved paths. Replace the IS_DIRECTORY .git guard so repositories whose .git entry is a file are handled as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@CMakeLists.txt`:
- Around line 36-41: Update the Git dependency configuration around the
CMAKE_CONFIGURE_DEPENDS property to support linked worktrees and symbolic
branches: use git rev-parse --git-path to resolve HEAD, index, packed-refs, and
the current symbolic HEAD ref, then register the resolved paths. Replace the
IS_DIRECTORY .git guard so repositories whose .git entry is a file are handled
as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: af5b010a-014e-4504-a57a-341ef649c450
📒 Files selected for processing (9)
.github/CONTRIBUTING.md.github/workflows/ci.yml.github/workflows/codecov-upload.ymlCMakeLists.txtCMakePresets.jsoninclude/Foliated_triangulation.hppinclude/Move_tracker.hppinclude/Utilities.hpptests/Utilities_test.cpp
💤 Files with no reviewable changes (1)
- CMakePresets.json
Keep Unix CI on pkgx and restrict the CMake and Ninja wheel fallback to Windows, where the pkgx packages are unavailable. - Install locked Python tooling exclusively from wheels - Use pinned upstream actionlint and zizmor distributions - Enforce the toolchain policy with a repository Semgrep rule - Refresh development-tool and GitHub Action pins
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@semgrep.yaml`:
- Around line 23-27: Update the semgrep rule’s pattern-either matcher to
recognize uv tool install commands, including flags such as --no-build, when
installing cmake or ninja. Preserve the existing Windows fallback fixture and
add a non-Windows negative fixture covering the uv installation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e6f79ae-9977-4c36-8eac-597324b56f3f
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/CONTRIBUTING.md.github/actions/setup-just/action.yml.github/workflows/ci.ymlJustfileREADME.mdpyproject.tomlscripts/mnist_experiment.pyscripts/optimize_initialize.pysemgrep.yamltests/semgrep/tooling_policy.yml
Resolve pkgx’s concrete CA path before vcpkg bootstrap so Unix CI and coverage jobs do not pass an unexpanded fallback expression to curl. Extend the pkgx-first Semgrep policy to reject non-Windows uv toolchain installations while preserving the approved Windows wheel fallback.
- Disable unused C++ module scanning so CodeQL can observe compiler calls. - Run the CGAL/oneTBB contract only on pinned Ubuntu toolchains while retaining reference coverage across all platforms.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@semgrep.yaml`:
- Around line 28-39: The Windows exclusion in the Semgrep rule must only allow
the documented pinned cmake/ninja fallback, not every Windows uv tool install.
Update the rule near the existing Windows pattern exclusion to require the
permitted fallback conditions, and add negative Windows fixtures in
tests/semgrep/tooling_policy.yml at lines 30-34 for installs lacking --no-build
or a pinned cmake/ninja package; preserve the existing documented pinned
fallback fixture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b1b98ba-5616-4505-9f50-7ead280bf63e
📒 Files selected for processing (8)
.github/CONTRIBUTING.md.github/workflows/ci.yml.github/workflows/codecov-upload.ymlCMakeLists.txtREADME.mddocs/multithreading.mdsemgrep.yamltests/semgrep/tooling_policy.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: CMake 4.4.0 or newer is required. The final 1.0 C++ API replaces legacy snake_case types and get_* accessors, separates RandomSeed from RandomStream, removes the MoveTracker template parameter, and requires validated PhysicalParameters for generalized S3 actions.
Closes #88
Summary by CodeRabbit
cdt --threadswith validation and “Maximum Delaunay threads” reporting, plusparallel.max_threadsin reproducibility metadata.just build-parallel, parallel CMake preset/build/test, and parallel CGAL benchmarking with warmups.--threads 0.threads/parallel runs.