From 7423f5fe5974577147fde1863c588b8359673605 Mon Sep 17 00:00:00 2001 From: Changhyun Kwon Date: Tue, 19 May 2026 19:48:57 +0900 Subject: [PATCH] Fix ctypes memory safety, enrich tests, bump to 0.1.0 - Wrap RoutingSolution extraction in try/finally so delete_solution always runs even if extraction raises. - Replace arr.astype(c_double).ctypes + cast(...) with named np.ascontiguousarray locals + .ctypes.data_as(c_double_p) so the numpy buffer is pinned through the C call. Also removes up to two redundant n^2 copies of the distance matrix. - Add tests covering the new exception path, dtype/contiguity variants, input validation, solver reuse, and CVRP solution invariants. - Bump CI Python matrix to 3.10 and 3.13. - Bump version to 0.1.0. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ci.yml | 2 +- pyproject.toml | 2 +- src/hygese/hygese.py | 57 +++++++----- src/hygese/tests/conftest.py | 47 ++++++++++ src/hygese/tests/test_cvrp.py | 31 +++++++ src/hygese/tests/test_inputs.py | 118 +++++++++++++++++++++++++ src/hygese/tests/test_memory_safety.py | 51 +++++++++++ 7 files changed, 282 insertions(+), 26 deletions(-) create mode 100644 src/hygese/tests/conftest.py create mode 100644 src/hygese/tests/test_inputs.py create mode 100644 src/hygese/tests/test_memory_safety.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 703c783..cdec28a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,7 +17,7 @@ jobs: fail-fast: false matrix: os: [ "ubuntu-latest", "windows-2019", "macos-latest" ] - python-version: [ "3.11", "3.10" ] + python-version: [ "3.10", "3.13" ] steps: - uses: actions/checkout@v2 diff --git a/pyproject.toml b/pyproject.toml index 2d02cee..7ce8ede 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "hygese" -version = "0.0.0.11" +version = "0.1.0" authors = [{name = "Changhyun Kwon"}] description = "Wrapper for the Hybrid Genetic Search algorithm for Capacitated Vehicle Routing Problems (HGS-CVRP)" readme = {file = "README.md", content-type = "text/markdown"} diff --git a/src/hygese/hygese.py b/src/hygese/hygese.py index 70de3c5..2ed08e0 100644 --- a/src/hygese/hygese.py +++ b/src/hygese/hygese.py @@ -8,7 +8,6 @@ c_double, c_char, sizeof, - cast, byref, ) from dataclasses import dataclass @@ -287,10 +286,13 @@ def _solve_cvrp( verbose: bool, ): n_nodes = x_coords.size - x_ct = x_coords.astype(c_double).ctypes - y_ct = y_coords.astype(c_double).ctypes - s_ct = service_times.astype(c_double).ctypes - d_ct = demand.astype(c_double).ctypes + # Bind each contiguous float64 view to a local so the buffer is pinned + # for the full duration of the C call. ascontiguousarray copies only + # when dtype/layout already mismatches the C expectation. + x = np.ascontiguousarray(x_coords, dtype=np.float64) + y = np.ascontiguousarray(y_coords, dtype=np.float64) + s = np.ascontiguousarray(service_times, dtype=np.float64) + d = np.ascontiguousarray(demand, dtype=np.float64) ap_ct = algorithm_parameters.ctypes # struct Solution * solve_cvrp( @@ -299,10 +301,10 @@ def _solve_cvrp( # int max_nbVeh, const struct AlgorithmParameters* ap, char verbose); sol_p = self._c_api_solve_cvrp( n_nodes, - cast(x_ct, c_double_p), - cast(y_ct, c_double_p), - cast(s_ct, c_double_p), - cast(d_ct, c_double_p), + x.ctypes.data_as(c_double_p), + y.ctypes.data_as(c_double_p), + s.ctypes.data_as(c_double_p), + d.ctypes.data_as(c_double_p), vehicle_capacity, duration_limit, is_rounding_integer, @@ -312,8 +314,11 @@ def _solve_cvrp( verbose, ) - result = RoutingSolution(sol_p) - self._c_api_delete_sol(sol_p) + try: + result = RoutingSolution(sol_p) + finally: + if sol_p: + self._c_api_delete_sol(sol_p) return result def _solve_cvrp_dist_mtx( @@ -332,12 +337,13 @@ def _solve_cvrp_dist_mtx( ): n_nodes = x_coords.size - x_ct = x_coords.astype(c_double).ctypes - y_ct = y_coords.astype(c_double).ctypes - s_ct = service_times.astype(c_double).ctypes - d_ct = demand.astype(c_double).ctypes - - m_ct = dist_mtx.reshape(n_nodes * n_nodes).astype(c_double).ctypes + x = np.ascontiguousarray(x_coords, dtype=np.float64) + y = np.ascontiguousarray(y_coords, dtype=np.float64) + s = np.ascontiguousarray(service_times, dtype=np.float64) + d = np.ascontiguousarray(demand, dtype=np.float64) + # A 2-D contiguous float64 array's data buffer is already a flat n*n + # block of doubles, so no extra reshape/ravel is needed. + m = np.ascontiguousarray(dist_mtx, dtype=np.float64) ap_ct = algorithm_parameters.ctypes # struct Solution *solve_cvrp_dist_mtx( @@ -346,11 +352,11 @@ def _solve_cvrp_dist_mtx( # int max_nbVeh, const struct AlgorithmParameters *ap, char verbose); sol_p = self._c_api_solve_cvrp_dist_mtx( n_nodes, - cast(x_ct, c_double_p), - cast(y_ct, c_double_p), - cast(m_ct, c_double_p), - cast(s_ct, c_double_p), - cast(d_ct, c_double_p), + x.ctypes.data_as(c_double_p), + y.ctypes.data_as(c_double_p), + m.ctypes.data_as(c_double_p), + s.ctypes.data_as(c_double_p), + d.ctypes.data_as(c_double_p), vehicle_capacity, duration_limit, is_duration_constraint, @@ -359,6 +365,9 @@ def _solve_cvrp_dist_mtx( verbose, ) - result = RoutingSolution(sol_p) - self._c_api_delete_sol(sol_p) + try: + result = RoutingSolution(sol_p) + finally: + if sol_p: + self._c_api_delete_sol(sol_p) return result diff --git a/src/hygese/tests/conftest.py b/src/hygese/tests/conftest.py new file mode 100644 index 0000000..2ea5ba1 --- /dev/null +++ b/src/hygese/tests/conftest.py @@ -0,0 +1,47 @@ +import pytest + +from hygese import AlgorithmParameters + + +_OR_TOOLS_DISTANCE_MATRIX = [ + [0, 548, 776, 696, 582, 274, 502, 194, 308, 194, 536, 502, 388, 354, 468, 776, 662], + [548, 0, 684, 308, 194, 502, 730, 354, 696, 742, 1084, 594, 480, 674, 1016, 868, 1210], + [776, 684, 0, 992, 878, 502, 274, 810, 468, 742, 400, 1278, 1164, 1130, 788, 1552, 754], + [696, 308, 992, 0, 114, 650, 878, 502, 844, 890, 1232, 514, 628, 822, 1164, 560, 1358], + [582, 194, 878, 114, 0, 536, 764, 388, 730, 776, 1118, 400, 514, 708, 1050, 674, 1244], + [274, 502, 502, 650, 536, 0, 228, 308, 194, 240, 582, 776, 662, 628, 514, 1050, 708], + [502, 730, 274, 878, 764, 228, 0, 536, 194, 468, 354, 1004, 890, 856, 514, 1278, 480], + [194, 354, 810, 502, 388, 308, 536, 0, 342, 388, 730, 468, 354, 320, 662, 742, 856], + [308, 696, 468, 844, 730, 194, 194, 342, 0, 274, 388, 810, 696, 662, 320, 1084, 514], + [194, 742, 742, 890, 776, 240, 468, 388, 274, 0, 342, 536, 422, 388, 274, 810, 468], + [536, 1084, 400, 1232, 1118, 582, 354, 730, 388, 342, 0, 878, 764, 730, 388, 1152, 354], + [502, 594, 1278, 514, 400, 776, 1004, 468, 810, 536, 878, 0, 114, 308, 650, 274, 844], + [388, 480, 1164, 628, 514, 662, 890, 354, 696, 422, 764, 114, 0, 194, 536, 388, 730], + [354, 674, 1130, 822, 708, 628, 856, 320, 662, 388, 730, 308, 194, 0, 342, 422, 536], + [468, 1016, 788, 1164, 1050, 514, 514, 662, 320, 274, 388, 650, 536, 342, 0, 764, 194], + [776, 868, 1552, 560, 674, 1050, 1278, 742, 1084, 810, 1152, 274, 388, 422, 764, 0, 798], + [662, 1210, 754, 1358, 1244, 708, 480, 856, 514, 468, 354, 844, 730, 536, 194, 798, 0], +] +_OR_TOOLS_DEMANDS = [0, 1, 1, 2, 4, 2, 4, 8, 8, 1, 2, 1, 2, 4, 4, 8, 8] + + +@pytest.fixture +def or_tools_data(): + """The 17-node CVRP instance from the OR-Tools VRP tutorial. + + Returns a fresh dict each call so tests can mutate it without bleeding into + each other (note: Solver.solve_tsp also mutates the dict it receives). + """ + return { + "distance_matrix": [row[:] for row in _OR_TOOLS_DISTANCE_MATRIX], + "num_vehicles": 4, + "depot": 0, + "demands": list(_OR_TOOLS_DEMANDS), + "vehicle_capacity": 15, + } + + +@pytest.fixture +def quick_ap(): + """AlgorithmParameters tuned for fast, deterministic runs (no optimum-cost asserts).""" + return AlgorithmParameters(timeLimit=0.5, seed=0) diff --git a/src/hygese/tests/test_cvrp.py b/src/hygese/tests/test_cvrp.py index 105ea1d..716576c 100644 --- a/src/hygese/tests/test_cvrp.py +++ b/src/hygese/tests/test_cvrp.py @@ -107,3 +107,34 @@ def test_cvrp_duration(): result = hgs_solver.solve_cvrp(data, rounding=True) assert result.cost == 42 + + +def test_routing_solution_invariants(): + """A feasible CVRP solution must visit every customer exactly once, + keep the depot out of route bodies, respect vehicle count, and not + exceed per-vehicle capacity. + """ + data = get_data() + n_nodes = len(data['demands']) + + ap = AlgorithmParameters(timeLimit=1.1) + result = Solver(ap, verbose=False).solve_cvrp(data) + + assert result.cost > 0 + assert result.n_routes >= 1 + assert result.n_routes <= data['num_vehicles'] + assert len(result.routes) == result.n_routes + + visited = [node for route in result.routes for node in route] + assert sorted(visited) == list(range(1, n_nodes)), ( + f"Each customer 1..{n_nodes - 1} must appear exactly once across " + f"all routes (and depot 0 must not appear); got {sorted(visited)}" + ) + + capacity = data['vehicle_capacity'] + demand = data['demands'] + for route in result.routes: + load = sum(demand[i] for i in route) + assert load <= capacity, ( + f"Route {route} has total demand {load} > capacity {capacity}" + ) diff --git a/src/hygese/tests/test_inputs.py b/src/hygese/tests/test_inputs.py new file mode 100644 index 0000000..982a55a --- /dev/null +++ b/src/hygese/tests/test_inputs.py @@ -0,0 +1,118 @@ +"""Input-variant and validation tests for hygese.Solver.solve_cvrp. + +Variant tests pin down the dtype / contiguity branches of np.ascontiguousarray +in _solve_cvrp and _solve_cvrp_dist_mtx. Validation tests pin down the +explicit ValueError / assertion guards in solve_cvrp. +""" + +import numpy as np +import pytest + +from hygese import AlgorithmParameters, Solver + + +# --- helpers --------------------------------------------------------------- + + +def _tiny_coord_data(): + """A tiny coord-based CVRP instance used by variant tests that need coords.""" + return { + "x_coordinates": [10.0, 20.0, 30.0, 40.0, 50.0], + "y_coordinates": [50.0, 40.0, 30.0, 20.0, 10.0], + "demands": [0, 2, 3, 1, 2], + "vehicle_capacity": 5, + "depot": 0, + } + + +# --- dtype / contiguity correctness ---------------------------------------- + + +def test_float32_inputs_match_float64(quick_ap): + solver = Solver(quick_ap, verbose=False) + + base = _tiny_coord_data() + f64 = { + **base, + "x_coordinates": np.asarray(base["x_coordinates"], dtype=np.float64), + "y_coordinates": np.asarray(base["y_coordinates"], dtype=np.float64), + "demands": np.asarray(base["demands"], dtype=np.float64), + } + f32 = { + **base, + "x_coordinates": np.asarray(base["x_coordinates"], dtype=np.float32), + "y_coordinates": np.asarray(base["y_coordinates"], dtype=np.float32), + "demands": np.asarray(base["demands"], dtype=np.float32), + } + + cost_f64 = solver.solve_cvrp(f64).cost + cost_f32 = solver.solve_cvrp(f32).cost + assert cost_f64 == cost_f32 + + +def test_non_contiguous_distance_matrix(or_tools_data, quick_ap): + """A non-C-contiguous (F-contiguous transpose of a symmetric matrix) view + must produce the same cost as the contiguous original — ascontiguousarray + is responsible for the layout copy. + """ + solver = Solver(quick_ap, verbose=False) + + base = or_tools_data + cost_contig = solver.solve_cvrp(base).cost + + m = np.asarray(base["distance_matrix"], dtype=np.float64) + assert np.array_equal(m, m.T), "OR-Tools matrix should be symmetric" + non_contig = m.T # F-contiguous view, same numerical content + assert not non_contig.flags["C_CONTIGUOUS"] + + variant = {**base, "distance_matrix": non_contig} + cost_view = solver.solve_cvrp(variant).cost + + assert cost_contig == cost_view + + +def test_int_demands_accepted(quick_ap): + """np.int64 demands must round-trip through ascontiguousarray(float64).""" + solver = Solver(quick_ap, verbose=False) + + data = _tiny_coord_data() + data["demands"] = np.array(data["demands"], dtype=np.int64) + + result = solver.solve_cvrp(data) + assert result.cost > 0 + assert result.n_routes >= 1 + + +def test_list_coordinates_accepted(quick_ap): + """Plain Python list coordinates must work through asarray -> ascontiguousarray.""" + solver = Solver(quick_ap, verbose=False) + + result = solver.solve_cvrp(_tiny_coord_data()) + assert result.cost > 0 + assert result.n_routes >= 1 + + +# --- input validation ------------------------------------------------------ + + +def test_invalid_depot_raises_value_error(or_tools_data, quick_ap): + solver = Solver(quick_ap, verbose=False) + or_tools_data["depot"] = 1 + with pytest.raises(ValueError, match="depot location must be 0"): + solver.solve_cvrp(or_tools_data) + + +def test_mismatched_lengths_raises_assertion(or_tools_data, quick_ap): + """Coords explicitly shorter than demands must trip the length assertion.""" + solver = Solver(quick_ap, verbose=False) + or_tools_data["x_coordinates"] = [0.0, 1.0] + or_tools_data["y_coordinates"] = [0.0, 1.0] + with pytest.raises(AssertionError): + solver.solve_cvrp(or_tools_data) + + +def test_negative_demand_raises_assertion(or_tools_data, quick_ap): + solver = Solver(quick_ap, verbose=False) + or_tools_data["demands"][1] = -1 + with pytest.raises(AssertionError): + solver.solve_cvrp(or_tools_data) diff --git a/src/hygese/tests/test_memory_safety.py b/src/hygese/tests/test_memory_safety.py new file mode 100644 index 0000000..cbd8767 --- /dev/null +++ b/src/hygese/tests/test_memory_safety.py @@ -0,0 +1,51 @@ +"""Regression tests for the ctypes memory-safety fixes in hygese.Solver. + +- delete_solution must run even when RoutingSolution extraction raises + (the `try/finally` block in _solve_cvrp / _solve_cvrp_dist_mtx). +- A single Solver must be reusable: each call gets its own ctypes buffers + and produces a clean result. +""" + +import pytest + +import hygese.hygese as _hg +from hygese import AlgorithmParameters, Solver + + +def test_delete_called_on_exception(or_tools_data, quick_ap, monkeypatch): + solver = Solver(quick_ap, verbose=False) + + delete_calls = [] + original_delete = solver._c_api_delete_sol + + def counting_delete(ptr): + delete_calls.append(bool(ptr)) + return original_delete(ptr) + + solver._c_api_delete_sol = counting_delete + + def boom(self, sol_ptr): + raise RuntimeError("simulated extraction failure") + + monkeypatch.setattr(_hg.RoutingSolution, "__init__", boom) + + with pytest.raises(RuntimeError, match="simulated extraction failure"): + solver.solve_cvrp(or_tools_data) + + assert delete_calls == [True], ( + "delete_solution must be called exactly once with a non-null pointer " + "when RoutingSolution.__init__ raises" + ) + + +def test_solver_reuse(or_tools_data): + ap = AlgorithmParameters(timeLimit=0.5, seed=42) + solver = Solver(ap, verbose=False) + + costs = [solver.solve_cvrp(or_tools_data).cost for _ in range(3)] + + assert all(c == costs[0] for c in costs), ( + f"Reusing a Solver with seed=42 should give identical costs across " + f"calls; got {costs}" + ) + assert costs[0] > 0