Skip to content

Gate take_ownership imports on what GraphBLAS can free - #641

Open
eriknw wants to merge 6 commits into
47-ss-export-full-messagefrom
47a-take-ownership-allocator
Open

eriknw wants to merge 6 commits into
47-ss-export-full-messagefrom
47a-take-ownership-allocator

Conversation

@eriknw

@eriknw eriknw commented Oct 19, 2026

Copy link
Copy Markdown
Member

A take_ownership=True import handed GraphBLAS any input array with
owndata and writeable set. owndata says an array owns its data, not
which allocator made it: an array from a custom NumPy memory handler (NEP 49)
owns its data too, and GraphBLAS frees what it owns with the allocator it was
initialized with. python-suitesparse-graphblas's letter of 2026-10-04 found
the process aborting (exit 134) when GraphBLAS freed one. Reproduced here
with psg 10.5.1.1, NumPy 2.5.4 and Python 3.15 under PYTHONMALLOC=debug:
an array from a GraphBLAS arena given libc's allocator (so it carries its own
memory handler) raised ValueError from unclaim_buffer after GraphBLAS had
taken it, and the process aborted in Vector.__del__.

Four commits:

Hand GraphBLAS only arrays its allocator can free. Ask psg's
can_unclaim_buffer (new in 10.5.1.1) when it exists, and keep the
owndata check for older psg. An array that fails is copied into memory
from psg's empty, which GraphBLAS allocated, not with NumPy: with
memory_manager="c" (psg initialized before python-graphblas) GraphBLAS
frees with libc's free, so a NumPy copy is no better. Before this, with
psg 10.5.1.1 and memory_manager="c", every ss import raised that
ValueError, take_ownership=False included. The take_ownership
docstrings say what the array needs.

Copy an array given twice to a take_ownership import. GraphBLAS frees
every buffer it takes. The imports copied the values when they were the
same array as one index array, but not other pairs: the same uint64 array
as indptr and col_indices of import_csr was freed twice (exit 134).

Fail a test that frees an owned array with no memory handler. psg's own
conftest guard: NumPy's private _set_numpy_warn_if_no_mem_policy(True)
for the session, so filterwarnings = error fails such a test even where
the allocators happen to agree. It is off on NumPy 1.24 (no setter) and
with psg before 10.5.1.1, whose claim_buffer makes exported arrays with
no handler: forced on with an older psg, 296 tests fail and 13 error,
every one on an array that claim_buffer made. This commit is test
infrastructure only; it can be dropped on its own (its revert applies
cleanly and nothing else depends on it).

Run the one-array-twice import test in a subprocess. A regression there
is a double free, which would abort the whole pytest run (exit 134)
instead of failing one test; the child process imports this tree.

Tests. test_take_ownership_of_foreign_memory imports arrays from a libc
arena (a real foreign memory handler, built without compiling anything) and
checks they were copied; test_take_ownership_asks_suitesparse_graphblas
monkeypatches can_unclaim_buffer to refuse and checks the copy;
test_import_with_libc_allocator (slow) runs imports under
memory_manager="c" with PYTHONMALLOC=debug in a subprocess;
test_take_ownership_of_one_array_twice imports csr, hypercsr and hypercsc
with an index array given twice (in a subprocess). All four fail on the
parent commit with psg 10.5.1.1. The first and third need psg
10.5.1.1 and are skipped with older psg.

Not done: psg's give_buffer context manager. It copies when needed,
commits only if GraphBLAS took the pointer, and unclaims, so it would replace
this and all eleven unclaim_buffer sites. Every import already unclaims
only after check_status, so it adds no safety yet; it restructures all
eleven import paths and needs psg 10.5.1.1 as a floor. Worth it once psg
10.5.1.1 is the minimum, or for the GxB_IS_READONLY load path.

@eriknw
eriknw added this pull request to stack #650 October 19, 2026 07:50
@codecov

codecov Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.66%. Comparing base (aca2f1f) to head (4d473ee).

Additional details and impacted files
@@                      Coverage Diff                      @@
##           47-ss-export-full-message     #641      +/-   ##
=============================================================
+ Coverage                      98.65%   98.66%   +0.01%     
=============================================================
  Files                            105      105              
  Lines                          29280    29389     +109     
  Branches                        3447     3446       -1     
=============================================================
+ Hits                           28885    28997     +112     
- Misses                           252      257       +5     
+ Partials                         143      135       -8     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@eriknw
eriknw force-pushed the 47a-take-ownership-allocator branch 2 times, most recently from fea0693 to 59c75dc Compare October 20, 2026 22:28
@eriknw
eriknw force-pushed the 47a-take-ownership-allocator branch 2 times, most recently from 78db402 to 3c9e06a Compare October 21, 2026 08:38
@eriknw
eriknw removed this pull request from stack #650 October 28, 2026 04:29
@eriknw
eriknw force-pushed the 47a-take-ownership-allocator branch from 3c9e06a to 41926fe Compare October 28, 2026 04:29
@eriknw
eriknw added this pull request to stack #653 October 28, 2026 04:30
@eriknw
eriknw removed this pull request from stack #653 October 28, 2026 04:31
@eriknw
eriknw added this pull request to stack #655 October 28, 2026 04:32
@eriknw
eriknw removed this pull request from stack #655 October 28, 2026 04:33
@eriknw
eriknw added this pull request to stack #657 October 28, 2026 04:33
@eriknw
eriknw force-pushed the 47a-take-ownership-allocator branch from 41926fe to 61513fb Compare October 28, 2026 13:21
A take_ownership import gave GraphBLAS any input array with owndata and
writeable set. owndata says the array owns its data, not which allocator
made it: an array from a custom NumPy memory handler (NEP 49) owns its
data too, and GraphBLAS frees what it owns with the allocator it was
initialized with. With suitesparse-graphblas 10.5.1.1, NumPy 2.5 and
PYTHONMALLOC=debug, importing such an array (one from libc's malloc,
carrying its own handler) raised ValueError from unclaim_buffer after
GraphBLAS had taken it, and the process aborted when the object was
freed.

suitesparse-graphblas 10.5.1.1 adds can_unclaim_buffer, which also
compares the array's memory handler with GraphBLAS's. Ask it when it
exists, and keep the owndata check for older versions. When an array
fails the check, copy it into memory from psg's empty, which GraphBLAS
allocated, rather than with NumPy: with memory_manager="c" (if psg was
initialized before python-graphblas) GraphBLAS frees with libc's free,
so a NumPy copy is no better. Before this, with psg 10.5.1.1 and
memory_manager="c", every ss import raised that ValueError, including
take_ownership=False, whose copy NumPy made.

The tests import arrays from a GraphBLAS arena given libc's allocator
(a real foreign memory handler, built without compiling anything) and
check that they were copied, check that the import asks
can_unclaim_buffer, and run imports under memory_manager="c" with
PYTHONMALLOC=debug in a subprocess. All three fail on the parent commit
with psg 10.5.1.1, and pytest itself then aborts at exit.

psg also offers give_buffer, a context manager that copies when needed,
commits only if GraphBLAS took the pointer, and unclaims. It would
replace this and every unclaim_buffer call, but each import already
unclaims only after check_status, so it adds no safety here yet; it
restructures all eleven import paths and needs psg 10.5.1.1.
GraphBLAS frees every buffer an import hands it. The imports copied the
values when they were the same array as one index array (col_indices,
row_indices, indices or bitmap), but not other pairs: the same uint64
array as indptr and col_indices of import_csr, or as rows and
col_indices of import_hypercsr, went to GraphBLAS twice, and freeing
the Matrix freed it twice (exit 134 on mlx; with suitesparse-graphblas
10.5.1.1 the second unclaim_buffer raises ValueError first).

Check every pair, and make the copy as the parent commit does, in
memory GraphBLAS can free (np.copy gave a NumPy copy, which GraphBLAS
can't own with memory_manager="c"). The test imports csr, hypercsr and
hypercsc with an index array given twice; on the parent commit it fails
and the process aborts.
NumPy frees the data of an array that owns it but has no NEP 49 memory
handler with libc free, which is right only when libc's malloc made it.
GraphBLAS allocates with NumPy's allocator, which on NumPy 2.5 is
PyMem_RawMalloc, so such a free is wrong under PYTHONMALLOC=debug and on
free-threaded Python. NumPy can warn whenever it does one; turn that on
for the test session (the same guard suitesparse-graphblas's own tests
use), so filterwarnings = error fails the test even where the two
allocators happen to agree.

The setter is private: numpy._core._multiarray_umath from NumPy 1.26,
numpy.core._multiarray_umath on 1.25, and absent on 1.24, where the
guard does nothing. It also does nothing with suitesparse-graphblas
before 10.5.1.1 (no can_unclaim_buffer), whose claim_buffer makes
exported arrays without a handler, so every export test would fail.

With suitesparse-graphblas 10.5.1.1, NumPy 2.5.4 and Python 3.15 the
whole suite passes with the guard on (2165 passed, 11 skipped with
--runslow).
A regression there frees an array twice, and the abort took down the
whole pytest run (exit 134) instead of failing the test. Run the imports
in a child process that imports this tree, as test_import_with_libc_allocator
does, through a helper both tests now share. The child's output is
unbuffered, so the report after a crash still shows which graphblas it ran.
…version

Without can_unclaim_buffer and empty (suitesparse-graphblas < 10.5.1.1),
a take_ownership import gives GraphBLAS an array that owns its data and
is writeable, and a NumPy copy of any other. CI ran that rule only on
draws of an older suitesparse-graphblas. The new test sets both names to
None, as test_take_ownership_asks_suitesparse_graphblas sets the other,
and checks that a read-only array and a view are copied and an exported
array is taken. It fails if the writeable check or the copy is dropped.
The import fallbacks themselves still run only on older versions.
The fallbacks for suitesparse-graphblas < 10.5.1.1, which has neither
can_unclaim_buffer nor empty, in utils.py and test_ss_utils.py run only
in CI jobs that draw an older version, so Codecov counted them hit or
missed by draw. Mark them "import", as the other import fallbacks are;
the MAINT note moves to its own line.

_libc_arena initializes GraphBLAS's last arena with libc's allocator
unless it already is, since GxB_arena_init refuses a second time. Its
one caller runs once per process, so the other arm runs only when the
test is repeated in the same process.
@eriknw
eriknw force-pushed the 47a-take-ownership-allocator branch from 61513fb to 4d473ee Compare October 29, 2026 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant