From 6533ef4e155abb18d3f753c25b97b8f8c09aae1d Mon Sep 17 00:00:00 2001 From: KaWest Date: Thu, 3 Sep 2026 11:47:06 +0200 Subject: [PATCH] Fix issue with linking Highs for editable py installs and compatibility improvements - Updated `test_device.py` to include conditional skipping for CUDA tests based on availability. - Modified `test_version_ssot.py` to utilize a new helper function for finding the DTWC binary. - Refactored `test_hpc.py` to change how seed arguments are handled in Slurm job scripts. - Revised `cluster_generic.slurm` to consolidate command-line arguments into a single array for better readability and maintainability. - Updated `pyproject.toml` to include Matplotlib in the test dependencies. - Improved CMake configuration to ensure HiGHS is statically linked for Python packaging, addressing unresolved library dependencies on macOS. --- .claude/LESSONS.md | 11 +++++ CHANGELOG.md | 6 +++ cmake/Dependencies.cmake | 58 +++++++++++++++++------- pyproject.toml | 2 +- scripts/slurm/jobs/cluster_generic.slurm | 53 +++++++++++----------- tests/python/test_device.py | 19 +++++++- tests/python/test_hpc.py | 3 +- tests/python/test_version_ssot.py | 15 +++--- 8 files changed, 114 insertions(+), 53 deletions(-) diff --git a/.claude/LESSONS.md b/.claude/LESSONS.md index 8fe678a..666b489 100644 --- a/.claude/LESSONS.md +++ b/.claude/LESSONS.md @@ -110,6 +110,12 @@ Critical knowledge to avoid repeating mistakes. ## Python packaging / bindings +- **A wheel repair tool does not protect local source/editable installs.** + HiGHS defaults to a shared library on Unix, leaving the installed Python + extension with `@rpath/libhighs.1.dylib` but no `LC_RPATH`; cibuildwheel can + repair release wheels, while `uv pip install -e .` cannot. Build HiGHS and + its extras as scoped PIC static libraries for Python packaging, and assert + the resulting CMake target type during configuration. - **Never infer Python import provenance from the checkout layout.** The 2026-07-23 probe found a mixed environment: `dtwcpp` and `_api.py` resolve to `python/dtwcpp/` in the repository, while `_dtwcpp_core` resolves to the venv @@ -327,6 +333,11 @@ Critical knowledge to avoid repeating mistakes. ## ARC SLURM Hardware +- **macOS still ships Bash 3.2, where an empty array expansion fails under + `set -u`.** A Slurm script using `"${optional_args[@]}"` worked on newer + cluster Bash versions but exited with `optional_args[@]: unbound variable` + in local macOS tests. Build one always-nonempty command argument array and + conditionally append optional flags before expanding it. - **htc GPU compute capabilities (corrected from docs).** The ARC docs list CUDA toolkit version, not compute capability. Actual values: P100=6.0, V100=7.0, RTX8000/TitanRTX=7.5, A100=8.0, RTXA6000=8.6, L40S=8.9, H100/GH200=9.0. - **Rome (htc-g019) and Broadwell (htc-g045-049) lack AVX-512.** Use `DTWC_ARCH_LEVEL=v3` for a portable x86 htc build; there is no one x86-64-v4 diff --git a/CHANGELOG.md b/CHANGELOG.md index d0c5b10..fb23d52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ This changelog contains a non-exhaustive list of new features and notable bug-fi

# Unreleased +- Fixed macOS Python source and editable installs by statically bundling HiGHS + into the native extension, eliminating the unresolved + `@rpath/libhighs.1.dylib` import dependency. Made the generic Slurm job's + optional seed arguments compatible with macOS Bash 3.2, restored + platform-independent CLI discovery in Python tests, and included Matplotlib + in the Python test dependencies. - **Fix (build, GCC + LTO):** `run_openmp`'s exception-capture region is an unnamed `#pragma omp critical` again. The named form made GCC emit a COMMON `.gomp_critical_user_dtwc_run_openmp_exception` symbol into every TU that diff --git a/cmake/Dependencies.cmake b/cmake/Dependencies.cmake index c4b3d91..05c56ad 100644 --- a/cmake/Dependencies.cmake +++ b/cmake/Dependencies.cmake @@ -39,22 +39,48 @@ function(dtwc_setup_dependencies) else() set(CUPDLP_GPU OFF CACHE BOOL "Enable HiGHS cuPDLP GPU support" FORCE) endif() - CPMAddPackage( - NAME highs - URL "https://github.com/ERGO-Code/HiGHS/archive/refs/tags/v1.15.1.tar.gz" - # SHA256 pinned (Task 0.12). Computed 2026-07-08 from the GitHub release - # tarball for the immutable tag v1.15.1 (`curl -sL … | sha256sum`). - URL_HASH SHA256=a840d269dff2fafb371dd247df13ad5e026d7ce3b35ad3dc1eedd59bf0c2fb16 - SYSTEM - EXCLUDE_FROM_ALL - OPTIONS - "CI OFF" - "ZLIB OFF" - "BUILD_CXX_EXE OFF" - "BUILD_EXAMPLES OFF" - "BUILD_TESTING OFF" - "FAST_BUILD ON" - ) + # A Python extension cannot rely on HiGHS' Unix default of BUILD_SHARED_LIBS=ON: + # the resulting @rpath/libhighs dependency is outside site-packages in local + # source/editable installs. Keep these normal variables scoped to the HiGHS + # subproject so native DTWC++ builds retain their requested shared-library + # policy. HiGHS sets PIC on its static target, making it safe to fold into the + # extension. GPU HiGHS is exempt because its Windows CUDA build requires DLLs. + block(SCOPE_FOR VARIABLES) + if(DTWC_BUILD_PYTHON AND NOT DTWC_HIGHS_GPU) + set(BUILD_SHARED_LIBS OFF) + set(BUILD_SHARED_EXTRAS_LIB OFF) + endif() + + CPMAddPackage( + NAME highs + URL "https://github.com/ERGO-Code/HiGHS/archive/refs/tags/v1.15.1.tar.gz" + # SHA256 pinned (Task 0.12). Computed 2026-07-08 from the GitHub release + # tarball for the immutable tag v1.15.1 (`curl -sL … | sha256sum`). + URL_HASH SHA256=a840d269dff2fafb371dd247df13ad5e026d7ce3b35ad3dc1eedd59bf0c2fb16 + SYSTEM + EXCLUDE_FROM_ALL + OPTIONS + "CI OFF" + "ZLIB OFF" + "BUILD_CXX_EXE OFF" + "BUILD_EXAMPLES OFF" + "BUILD_TESTING OFF" + "FAST_BUILD ON" + ) + endblock() + + # Fail during configuration, rather than shipping another extension with an + # unresolved libhighs dependency, if HiGHS changes how it honours + # BUILD_SHARED_LIBS. + if(DTWC_BUILD_PYTHON AND NOT DTWC_HIGHS_GPU AND TARGET highs) + get_target_property(_dtwc_highs_library_type highs TYPE) + if(NOT _dtwc_highs_library_type STREQUAL "STATIC_LIBRARY") + message(FATAL_ERROR + "Python packages require statically bundled HiGHS; got " + "${_dtwc_highs_library_type}") + endif() + endif() + # Historically HiGHS <=1.14.0 had a debug assertion (ub_consistent) that # fired on valid warm-start MIP solves (primal-dual integral bookkeeping # tolerance 1e-12 too tight after a presolve reset). Retained defensively: diff --git a/pyproject.toml b/pyproject.toml index fb01d7b..8d24180 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -46,7 +46,7 @@ Issues = "https://github.com/Battery-Intelligence-Lab/dtw-cpp/issues" Changelog = "https://github.com/Battery-Intelligence-Lab/dtw-cpp/blob/main/CHANGELOG.md" [project.optional-dependencies] -test = ["pytest>=6.0"] +test = ["pytest>=6.0", "matplotlib>=3.7"] sklearn = ["scikit-learn>=1.0"] hdf5 = ["h5py>=3.0"] parquet = ["pyarrow>=12.0"] diff --git a/scripts/slurm/jobs/cluster_generic.slurm b/scripts/slurm/jobs/cluster_generic.slurm index 0cae027..9256506 100644 --- a/scripts/slurm/jobs/cluster_generic.slurm +++ b/scripts/slurm/jobs/cluster_generic.slurm @@ -97,34 +97,35 @@ mkdir -p "${RESULTS_DIR}" T_START=$(date +%s.%N) DTWC_EXIT=0 -SEED_ARGS=() +DTWC_ARGS=( + --input "${DTWC_INPUT}" + -k "${DTWC_K}" + --skip-cols "${DTWC_SKIP_COLS}" + --dtype "${DTWC_DTYPE}" + --method "${DTWC_METHOD}" + --band "${DTWC_BAND}" + --device "${DTWC_DEVICE}" + --max-iter "${DTWC_MAX_ITER}" + --variant "${DTWC_VARIANT}" + --wdtw-g "${DTWC_WDTW_G}" + --adtw-penalty "${DTWC_ADTW_PENALTY}" + --msm-c "${DTWC_MSM_C}" + --twe-nu "${DTWC_TWE_NU}" + --twe-lambda "${DTWC_TWE_LAMBDA}" + --mv-mode "${DTWC_MV_MODE}" + --missing-strategy "${DTWC_MISSING_STRATEGY}" + --metric "${DTWC_METRIC}" + --n-init "${DTWC_N_INIT}" +) if [[ -n "${DTWC_SEED}" ]]; then - SEED_ARGS=(--seed "${DTWC_SEED}") + DTWC_ARGS+=(--seed "${DTWC_SEED}") fi -"${DTWC_BIN}" \ - --input "${DTWC_INPUT}" \ - -k "${DTWC_K}" \ - --skip-cols "${DTWC_SKIP_COLS}" \ - --dtype "${DTWC_DTYPE}" \ - --method "${DTWC_METHOD}" \ - --band "${DTWC_BAND}" \ - --device "${DTWC_DEVICE}" \ - --max-iter "${DTWC_MAX_ITER}" \ - --variant "${DTWC_VARIANT}" \ - --wdtw-g "${DTWC_WDTW_G}" \ - --adtw-penalty "${DTWC_ADTW_PENALTY}" \ - --msm-c "${DTWC_MSM_C}" \ - --twe-nu "${DTWC_TWE_NU}" \ - --twe-lambda "${DTWC_TWE_LAMBDA}" \ - --mv-mode "${DTWC_MV_MODE}" \ - --missing-strategy "${DTWC_MISSING_STRATEGY}" \ - --metric "${DTWC_METRIC}" \ - --n-init "${DTWC_N_INIT}" \ - "${SEED_ARGS[@]}" \ - --name "${DTWC_NAME}" \ - --output "${RESULTS_DIR}" \ - --verbose \ - || DTWC_EXIT=$? +DTWC_ARGS+=( + --name "${DTWC_NAME}" + --output "${RESULTS_DIR}" + --verbose +) +"${DTWC_BIN}" "${DTWC_ARGS[@]}" || DTWC_EXIT=$? T_END=$(date +%s.%N) ELAPSED=$(echo "${T_END} - ${T_START}" | bc 2>/dev/null || echo "?") diff --git a/tests/python/test_device.py b/tests/python/test_device.py index 4de9ceb..0c1a5a1 100644 --- a/tests/python/test_device.py +++ b/tests/python/test_device.py @@ -228,7 +228,24 @@ def test_no_python_side_copy_of_the_local_device(self): ) @pytest.mark.parametrize( ("requested", "canonical"), - [("cuda", "gpu"), ("cuda:0", "gpu"), ("gpu:0", "gpu"), ("GPU", "gpu")], + [ + pytest.param( + "cuda", "gpu", + marks=pytest.mark.skipif( + not (dtwcpp.CUDA_AVAILABLE and dtwcpp.cuda_available()), + reason="explicit CUDA unavailable", + ), + ), + pytest.param( + "cuda:0", "gpu", + marks=pytest.mark.skipif( + not (dtwcpp.CUDA_AVAILABLE and dtwcpp.cuda_available()), + reason="explicit CUDA unavailable", + ), + ), + ("gpu:0", "gpu"), + ("GPU", "gpu"), + ], ) def test_gpu_aliases_canonicalise(self, requested, canonical): try: diff --git a/tests/python/test_hpc.py b/tests/python/test_hpc.py index de42f89..4b612c6 100644 --- a/tests/python/test_hpc.py +++ b/tests/python/test_hpc.py @@ -845,7 +845,8 @@ def test_restart_schedule_reaches_dtwc_cl(self): assert "DTWC_SEED=${SEED}" in wrapper assert 'DTWC_N_INIT="${DTWC_N_INIT:-1}"' in job assert '--n-init "${DTWC_N_INIT}"' in job - assert 'SEED_ARGS=(--seed "${DTWC_SEED}")' in job + assert 'DTWC_ARGS+=(--seed "${DTWC_SEED}")' in job + assert '"${DTWC_BIN}" "${DTWC_ARGS[@]}"' in job for name in ( "MAX_ITER", "VARIANT", "WDTW_G", "ADTW_PENALTY", "MSM_C", "TWE_NU", "TWE_LAMBDA", "MV_MODE", "MISSING_STRATEGY", "METRIC", diff --git a/tests/python/test_version_ssot.py b/tests/python/test_version_ssot.py index 8ecaa24..f03d978 100644 --- a/tests/python/test_version_ssot.py +++ b/tests/python/test_version_ssot.py @@ -5,6 +5,7 @@ import subprocess import dtwcpp +from dtwcpp._hpc import find_dtwc_binary ROOT = Path(__file__).resolve().parents[2] @@ -12,14 +13,12 @@ def _cli_path() -> Path: override = os.environ.get("DTWC_CL_PATH") - candidates = [ - Path(override) if override else None, - ROOT / "build" / "cfg-gate-normal" / "dtwc_cl.exe", - ROOT / "build" / "cfg-gate-normal" / "bin" / "dtwc_cl.exe", - ] - for candidate in candidates: - if candidate is not None and candidate.exists(): - return candidate + if override and Path(override).exists(): + return Path(override) + + candidate = find_dtwc_binary(str(ROOT)) + if candidate is not None: + return Path(candidate) raise AssertionError("dtwc_cl executable not found; set DTWC_CL_PATH")