Add inmem ergonomics: libssu_inmem.a static target, _seeded overloads by wasade · Pull Request #86 · biocore/unifrac-binaries

wasade · 2026-05-06T19:51:37Z

Summary

Three small ergonomic improvements requested by a downstream embedder (duckdb-miint, embedding libssu inside a DuckDB extension). None are blocking; each tightens a rough edge that surfaced while planning that integration.

Four commits, ~580 LOC across 9 files, organized so each change is independently reviewable:

059851d Add libssu_inmem.a native in-memory static target. Mirrors libssu_wasm.a (same in-memory subset of TUs, no HDF5/lz4/GPU/dlopen) but compiled with the host toolchain so OpenMP is enabled. New src/inmem_build.mk defines INMEM_CXXFLAGS, per-TU .inmem.o rules, libssu_inmem.a, and install_inmem / inmem_clean targets.
c9b6f7a Guard install_inmem against unset PREFIX. Code-review follow-up: error out instead of silently installing into /lib.
1c6d25a Add _seeded overloads for inmem PERMANOVA and subsample. compute_permanova_inmem_* and subsample_table_inmem previously read from the global skbb RNG (re-seedable only via ssu_set_random_seed), forcing concurrent multi-subsample workflows to serialize on an external mutex. New compute_permanova_inmem_fp{64,32}_seeded and subsample_table_inmem_seeded accept an explicit int seed. Convention matches skbb_permanova_*: seed >= 0 is deterministic, seed < 0 falls back to the global RNG. The non-seeded entry points become thin convenience wrappers calling _seeded(..., -1, ...).

Dependency

Commit #2 (libssu_inmem.a) only delivers the full no-LAPACKE story when paired with the companion scikit-bio/scikit-bio-binaries#13 (libskbb_inmem.a). Without it, libssu_inmem.a still builds and runs but must link against libskbb.so, which drags LAPACKE/cblas. The other commits in this PR are independent of skbb.

Recommended merge order: skbb PR #13 first, then this PR.

Test plan

All seven WASM tests pass (smoke / faith_pd / subsample / pcoa / permanova / unifrac_e2e / capi_inmem)
New C-API test cases exercise the dlopen-wrapper path natively AND the static-archive path under WASM, since the same test/capi_inmem_test.c source flows through both:
- test_subsample_inmem_seeded_capi: same-seed reproducibility under adversarial global-RNG perturbation, seed = -1 equivalence with the legacy non-seeded API, seed influence over a small set of alt seeds
- test_permanova_inmem_seeded_capi: same contracts as subsample plus an fp32 smoke check
make inmem_static builds cleanly from a fresh checkout (688K archive); nm shows zero HDF5/lz4 undefined symbols, only skbb_* externals
End-to-end full-static link: libssu_inmem.a + libskbb_inmem.a + capi_inmem_test.c produces a binary whose ldd shows no project runtime dependencies (only stdc++/m/gomp/gcc_s/c). All unit tests pass
make install_inmem with PREFIX/CONDA_PREFIX both unset fails with a clear error message

🤖 Generated with Claude Code

sfiligoi · 2026-05-07T00:16:41Z

-void pcoa(const double * mat, const uint32_t n_samples, const uint32_t n_dims, double **eigenvalues, double **samples, double **proportion_explained);
-void pcoa_fp32(const float * mat, const uint32_t n_samples, const uint32_t n_dims, float * *eigenvalues, float * *samples, float * *proportion_explained);
-void pcoa_mixed(const double * mat, const uint32_t n_samples, const uint32_t n_dims, float * *eigenvalues, float * *samples, float * *proportion_explained);
+EXTERN void pcoa(const double * mat, const uint32_t n_samples, const uint32_t n_dims, double **eigenvalues, double **samples, double **proportion_explained);


The pcoa interface was not exposed with a C interface by design.
This functionality is not supposed to be public.
(it is there just for internal uses only)

Tools that require pcoa should use directly the pcoa interface in scikit-bio-binaries.

sfiligoi · 2026-05-07T00:20:18Z

Hi @wasade .
I don't want to add pcoa to the unifrac-binaries API.

We have scikit-bio-binaries for that.

sfiligoi · 2026-05-07T00:27:57Z

We probably should not have permanova functions either.
But that was added at one point in the past, and it is probably not a good idea to deprecate it now.

But I would not add any other API calls that can be served by scikit-bio-binaries instead,
since it would be just a pass-through anyway.

wasade · 2026-05-07T14:45:50Z

Totally makes sense, I'll back those changes out

Mirror libssu_wasm.a (same in-memory subset of TUs, no HDF5/lz4/GPU/ dlopen) but compile with the host toolchain so OpenMP and host-aware optimization can be enabled. Intended for downstream native projects that want to embed the in-memory UniFrac surface as a single static archive — same shape libssu_wasm.a provides for emscripten consumers. Companion to scikit-bio-binaries' libskbb_inmem.a, which provides the Eigen-backed skbb dependency without dragging in LAPACKE/cblas. The two archives together are link-complete with no project runtime deps. Implementation: - Add src/inmem_build.mk defining INMEM_CXX/INMEM_CXXFLAGS, per-TU .inmem.o rules for the same source set as libssu_wasm.a, and the libssu_inmem.a / install_inmem / inmem_clean targets. Stages skbb public headers under .inmem-skbb-include/ in the same shape as the WASM build's .wasm-skbb-include/. - Include the new fragment from src/Makefile alongside the existing wasm/emscripten_build.mk include. - Gitignore the staged-headers cache. The archive carries only skbb_* externals; HDF5/lz4 are entirely absent. Skbb is the embedder's responsibility (link with libskbb_inmem.a for full-static, or libskbb.so for the LAPACKE path). Verified: - libssu_inmem.a (688K) builds with zero HDF5/lz4 undefined symbols. - End-to-end link of test/capi_inmem_test.c against libssu_inmem.a plus libskbb_inmem.a yields a binary with no project runtime dependencies; ldd shows only stdc++/m/gomp/gcc_s/c. - All inmem unit tests pass (UniFrac compute, faith_pd, subsample, PERMANOVA, PCoA — including the C-API symmetry case added in the prior pcoa-EXTERN commit). - Existing make wasm rebuild still works; the WASM test suite passes end-to-end against the rebuilt skbb. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Without this guard, invoking `make install_inmem` outside a conda env with PREFIX unset would silently expand to `mkdir -p /lib` and `cp libssu_inmem.a /lib/` — either failing on permissions or, in the worst case, polluting the root filesystem. Fail fast with a clear message instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

wasade · 2026-05-07T15:27:59Z

How's this?

sfiligoi · 2026-05-07T23:30:32Z

    delete sub;
 }

+compute_status compute_permanova_inmem_fp64_seeded(const double *mat, unsigned int n_dims,


Is there a reason why we need this in unifrac-binaries?
Don't we already have this in scikit-bio-binaries?

I understand the need for internal use...
My question is about making this a public API.

wasade · 2026-05-07T23:45:02Z

Your prior comments seemed to suggest it was fine adding permanova to the public API and the concern was PCoA. I’m happy to undo permanova as well. Are there any other items that should be addressed?On May 7, 2026, at 17:33, Igor Sfiligoi ***@***.***> wrote: @sfiligoi commented on this pull request. In src/api.cpp:

*t = NULL;

delete sub; } +compute_status compute_permanova_inmem_fp64_seeded(const double *mat, unsigned int n_dims, I understand the need for internal use... My question is about making this a public API. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

sfiligoi · 2026-05-07T23:48:26Z

Sorry if I was ambiguous.
I would prefer not to add ANY new public API that is already served by scikit-bio-binaries.
If you don't mind, please remove the new permanova public API, too.

(And I had not have time to do a proper, complete review yet... sorry, I have a few 100% meeting days)

wasade · 2026-05-07T23:49:56Z

No worries, got it and will do!

The existing subsample_table_inmem reads from the global skbb-side RNG, re-seedable only via ssu_set_random_seed. That forces concurrent multi-subsample workflows to serialize on an external mutex around the seeder. This adds a per-call _seeded variant that accepts an explicit `int seed` and constructs su::biom_subsampled directly, sidestepping the global RNG. Convention: seed >= 0 produces a deterministic draw; seed < 0 falls back to the global RNG, making the non-seeded API equivalent to passing seed = -1. Subsample is unifrac-specific (not served by scikit-bio-binaries). The original branch also added compute_permanova_inmem_*_seeded but those have been dropped per reviewer feedback (PR #86 review): new public APIs that scikit-bio-binaries already serves should not be duplicated here. Plumbing: - api.cpp: subsample_table_inmem_seeded constructs su::biom_subsampled directly when seed >= 0 (skipping the skbio_biom_subsampled subclass whose only purpose is to fetch a seed from the global RNG). The accessors and destroy_subsampled_inmem cast the opaque handle to su::biom_inmem* (the actual interface contract) instead of the most-derived type — biom_inmem has a virtual destructor, so delete chains correctly through either subclass. - subsample_table_inmem becomes a thin convenience wrapper calling subsample_table_inmem_seeded(..., -1, ...). - combined/libssu.c: new dlopen wrapper for the seeded entry point. Tests (test/capi_inmem_test.c): - test_subsample_inmem_seeded_capi: same-seed reproducibility under adversarial global-RNG perturbation, seed=-1 equivalence with the legacy non-seeded API, and seed influence over a small set of alt seeds (avoid pinning to a single pair — the 6-sample fixture has limited entropy and per-pair collisions are not unusual). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

sfiligoi · 2026-05-08T14:44:02Z

 const char* subsampled_get_sample_id(const opaque_biom_inmem_t *t, unsigned int idx) {
    if (t == NULL) return NULL;
-    const su::skbio_biom_subsampled *sub = (const su::skbio_biom_subsampled*) t;
+    const su::biom_inmem *sub = (const su::biom_inmem*) t;


Why did you change the type here?

Claude is stating “ Casting to
skbio_biom_subsampled* when the actual object is the parent class is
technically UB, even though it works in practice.”

It suggests: “ there's a cleaner fix that avoids the cast change entirely: add a second
constructor to skbio_biom_subsampled that takes an explicit seed and just
forwards to biom_subsampled(parent, w_replacement, n, seed). Then both paths
construct skbio_biom_subsampled, the original casts stay valid, and the
accessor diff disappears.”

I’m happy to revert, take the suggested approach or do something else. Preference?

I see where it is coming from;
opaque_biom_inmem_t can actually be either skbio_biom_subsampled or biom_subsampled.

unifrac-binaries/src/api.cpp

Line 948 in 0feebf5

if (seed < 0) {

So using the parent class makes sense.

sfiligoi

Looks good.

wasade · 2026-05-11T18:03:07Z

Thanks!On May 11, 2026, at 12:01, Igor Sfiligoi ***@***.***> wrote: Merged #86 into main. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

sfiligoi reviewed May 7, 2026

View reviewed changes

wasade and others added 2 commits May 7, 2026 08:57

wasade force-pushed the inmem_static_260506 branch from c0c52b8 to 24b21fc Compare May 7, 2026 15:25

wasade changed the title ~~Add inmem ergonomics: pcoa C-ABI, libssu_inmem.a static target, _seeded overloads~~ Add inmem ergonomics: libssu_inmem.a static target, _seeded overloads May 7, 2026

sfiligoi reviewed May 7, 2026

View reviewed changes

wasade force-pushed the inmem_static_260506 branch from 24b21fc to 0feebf5 Compare May 8, 2026 02:40

sfiligoi reviewed May 8, 2026

View reviewed changes

sfiligoi approved these changes May 11, 2026

View reviewed changes

sfiligoi merged commit 91c2600 into main May 11, 2026
12 checks passed

Uh oh!

Conversation

wasade commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Dependency

Test plan

Uh oh!

sfiligoi May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfiligoi commented May 7, 2026

Uh oh!

sfiligoi commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wasade commented May 7, 2026

Uh oh!

wasade commented May 7, 2026

Uh oh!

sfiligoi May 7, 2026

Choose a reason for hiding this comment

Uh oh!

sfiligoi May 7, 2026

Choose a reason for hiding this comment

Uh oh!

wasade commented May 7, 2026 via email

Uh oh!

sfiligoi commented May 7, 2026

Uh oh!

wasade commented May 7, 2026

Uh oh!

sfiligoi May 8, 2026

Choose a reason for hiding this comment

Uh oh!

wasade May 8, 2026

Choose a reason for hiding this comment

Uh oh!

sfiligoi May 11, 2026

Choose a reason for hiding this comment

Uh oh!

sfiligoi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wasade commented May 11, 2026 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wasade commented May 6, 2026 •

edited

Loading

sfiligoi May 7, 2026 •

edited

Loading

sfiligoi commented May 7, 2026 •

edited

Loading