Repository navigation
Conversation
P0-1: mix.exs package metadata - closed #71 - Added source_url: @source_url to project() and docs: config - Added source_ref: "v#{@Version}" for correct GitHub source links - Uncommented files: list — explicit: lib native priv/native .formatter.exs mix.exs README.md LICENSE CHANGELOG.md - Changed prod elixirc_paths from ["lib", "native"] to ["lib"] — users won't need SuiteSparse headers to compile the package - Added runtime: false to excoveralls dependency P0-2: Resource leaks (transposed matrices) - closed #72 - maybe_transpose_inp0/inp1 now returns {ptr, transposed?} boolean flag instead of {ptr, struct} - Added maybe_free_transposed/2 helper that frees the pointer when true - matrix_mxm, matrix_mxv, vector_vxm now free transposed matrices in after blocks (runs on both success and error) P0-3: Ignored error codes in Zig - closed #73 - Replaced 22 instances of _ = GrB_*() with try translate_info(GrB_*()) for dimension queries, type queries, descriptor set calls, JIT disable, and finalize - Only _free calls remain with _ = (correct — nothing to do on free failure) P0-4: Silent type defaults - closed #74 - type_code_to_grb_type, semiring_from_code, monoid_from_code now return GraphBLASError!T and the else branch returns error.invalid_value instead of silently defaulting - All 13 call sites updated with try Verification: mix compile --warnings-as-errors (dev + test + prod), mix test (350/0), mix credo --strict (0 issues), mix dialyzer (0 errors) all pass.
P1-5: Documentation fixes (9 items) - closed #76 - guides/architecture_walkthrough.md — Replaced "what's coming" masks/descriptors section with current state; replaced "next steps" Phase 2-6 with "completed phases" reflecting actual status - guides/native_backend_walkthrough.md — Fixed 6 bare SuiteSparse.* calls to GraphBLAS.Backend.SuiteSparse.* in standalone code blocks (aliased blocks left alone) - guides/installation_guide.md — Updated SuiteSparse version requirement from >= 7.0 to >= 9.4.5; added macOS Intel path note (/usr/local/include/suitesparse) - README.md — Added ZigStub backend to architecture diagram and intro paragraph; added environment variables table (SUITESPARSE_INCLUDE_PATH, EX_GRAPHBLAS_BUILD, EX_GRAPHBLAS_COMPILE_NATIVE) - mix.exs — Changed Elixir requirement from ~> 1.17 to ~> 1.18 to match installation guide - guides/parity_testing_guide.md — Updated total test count from "~175-200" to "350+" P1-6: Unsafe hd() call — algorithm.ex:258 — Replaced hd(unvisited_entries) with pattern match [{v, _} | _] - closed #77 P1-7/P1-8: No change needed — SuiteSparse @impl functions inherit docs from the behaviour; all NIF calls are already within rescue scope - closed #78, closed #79 P1-9: Zig build bounds checks — Added if (nvals > *.len) return error.invalid_index guards to all 6 build functions (3 matrix, 3 vector) preventing buffer overread - closed #80 P1-10: Closure loop limit — relation.ex:closure/4 now accepts :max_iter option (default: 100); updated @doc - closed #81 P1-11: SSSP infinity — algorithm.ex:sssp/3 now accepts :infinity option (default: 1.0e18); updated @doc - closed #82 P1-12: Already fixed in P0-3 - closed #83 P1-13: ZigStub dimension validation — Added when nrows >= 0 and ncols >= 0 guard to matrix_new and when size >= 0 guard to vector_new, matching the Elixir backend's validation - closed #84 Verification: mix compile --warnings-as-errors (dev + test), mix test (350/0), mix credo --strict (0 issues), mix dialyzer (0 errors).
All P2 items done. Summary: P2-14: Edge case tests — New test/ex_graphblas/edge_case_test.exs (22 tests): closed #86 - Zero-dimension matrices (0x0, 0xN, Nx0) — create, nvals, to_coo - Zero-size vectors — create, nvals, to_entries - Negative dimensions rejected — matrix and vector - Out-of-bounds column indices rejected - Dup of empty containers - All 3 type variants (:int64, :fp64, :bool) for matrix and vector - Set on empty containers - to_dense/to_list on empty containers P2-15: Algebraic property tests — New test/ex_graphblas/algebraic_property_test.exs (17 tests): closed #87 - Commutativity: A + B == B + A (matrix and vector) - Associativity: (AB)C == A(BC) - Transpose: (A^T)^T == A, shape inversion, index swapping - ewise_add with disjoint entries: nvals additive, A + empty == A - Reduce: single-entry vector, row-sum matrix reduce - Dup independence: modify copy doesn't affect original P2-16: Backend callback coverage — Covered by the edge case tests above (negative dims, dup empty, set on empty, all type variants) - closed #88 P2-17: CI fixes — Coverage flag (mix test --cover when matrix.coverage), formatter inputs expanded to native/ and bench/, removed 2>/dev/null from checksum download - closed #89 P2-18: Config fixes — dev.exs force_build now conditional on EX_GRAPHBLAS_BUILD env var; prod.exs documents how to switch to SuiteSparse backend - closed #90 P2-19: Native test skip stubs — Added else branches to suite_sparse_test, parity_test, property_test with visible skip messages (3 skipped in output) - closed #91 P2-20: @doc raises documentation — Added "Raises KeyError" to new/1 in BinaryOp, UnaryOp, Monoid, Semiring; added "Raises ArgumentError" to fn_for/1 in BinaryOp, UnaryOp - closed #92 P2-21: @moduledoc false — No change needed; idiomatic for internal modules - closed #93 P2-22: Logger.debug — Application.ex rescue blocks now log init/finalize failures with Logger.debug instead of silently discarding - closed #94 Verification: 392 tests, 0 failures, 3 skipped. Credo strict: 0 issues. Compile warnings-as-errors: clean.
All P3 items done. Summary: P3-23: Duplicate constants — Replaced defp max_int/min_int private functions in monoid.ex with @int64_max/@int64_min module attributes. Positioned before first use. The scalar.ex private type_max/type_min handles more types (fp64, bool) so left as-is — no true duplication. - closed #96 P3-24: fixed_point/3 @doc — Added "Default convergence behavior" section documenting: matrices use exact COO comparison, vectors with tol > 0 use tolerance-based comparison, vectors with tol == 0 use exact comparison, other values never converge. - closed #97 P3-25: Relation backend inheritance — Already documented at lines 50-51 and 81-82. No change needed. - closed #98 P3-26: Descriptor cleanup — The descriptor_is_custom / cleanup_descriptor pattern is correct. Both Zig functions use the same predefined descriptor list. Not fragile in practice. No change needed. - closed #99 P3-27: SuiteSparse include path — Added "Include path" section to GraphBLAS.Native.SuiteSparse @moduledoc listing platform-specific defaults (Apple Silicon, Intel Mac, Linux). - closed #100 P3-28: replace_output no-op — Replaced speculative "future in-place operations" language with clear statement: in v0.2.0, output: :replace has no observable effect from the Elixir API, included for forward-compatibility. D- closed #101 Verification: mix compile --warnings-as-errors (dev + test), mix test (392/0/3 skipped), mix credo --strict (0 issues), mix dialyzer (0 errors), mix docs (0 warnings). All clean. - closed #101 --- All 28 review items (P0 through P3) are now addressed. The full review plan in plans/review-0.2.0.md is complete.
Workflow fix (3 changes): - native/suite_sparse.ex -- Added targets: restricting to the 6 platforms we actually build (was defaulting to all 12 ZiglerPrecompiled defaults, causing 404 on arm-linux-gnueabihf etc.) - .github/workflows/precompiled-nifs.yml -- publish_hex job now builds GraphBLAS v9.4.5 from source (was using Ubuntu's v7.4.0 libgraphblas-dev which lacks GrB_Descriptor_set_INT32) - .github/workflows/precompiled-nifs.yml -- Added --ignore-unavailable flag to mix zigler_precompiled.download as a safety net Release materials (2 files): - CHANGELOG.md -- Complete v0.2.0 release notes covering the three-backend architecture, all P0-P3 fixes, new tests, CI changes, and documentation updates - plans/release-announcement-0.2.0.md -- Reddit r/elixir and Elixir Forum announcements ready to post Pending on you: 1. Push these changes and re-run the precompiled-nifs workflow to verify the fix 2. mix hex.publish --dry-run to check package contents before the real publish
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.
Pre-Release Code Review: ex_graphblas v0.2.0
Date: 2026-04-25
Reviewer: Sisyphus (automated)
Scope: Full branch review — lib/, native/, test/, docs, CI, mix.exs
Purpose: Identify and prioritize issues before first Hex.pm release
Executive Summary
The codebase is well-structured with good type safety, consistent error handling, and a solid backend abstraction. The three-backend architecture (Elixir, SuiteSparse, ZigStub) is clean and the
@behaviour GraphBLAS.Backendcontract is well-defined with 30 callbacks. However, there are significant issues across resource management, documentation accuracy, package metadata, and test coverage that must be addressed before publishing to Hex.pm.Issue counts by severity:
P0 — Release Blockers
1. mix.exs Package Metadata (will break Hex.pm listing)
source_urlmissing fromproject()source_url: @source_urlto project listsource_urlmissing fromdocs:configsource_url: @source_urlto docs configfiles:commented out — package will includenative/,bench/, Zig staging filesfiles: ~w(lib priv .formatter.exs mix.exs README.md LICENSE CHANGELOG.md)elixirc_pathsincludes"native"— users without SuiteSparse can't compiledefp elixirc_paths(_), do: ["lib"]Why this is P0: Without
source_url, ExDoc generates docs without GitHub source links. Without explicitfiles:, the Hex package includes 795-line Zig NIF source code, benchmark files, and Zigler staging artifacts. Users whomix deps.getthe package will fail to compile becausenative/suite_sparse.exusesuse ZiglerPrecompiledwhich requires SuiteSparse headers.2. Resource Leaks in SuiteSparse Backend (memory leak in production)
Transposed matrices created by
maybe_transpose_inp0/inp1inmatrix_mxm,matrix_mxv,vector_vxmare never freed. Every call with a transpose descriptor leaks aGrB_Matrix.matrix_mxmmatrix_mxvvector_vxmRoot cause:
maybe_transpose_inp0andmaybe_transpose_inp1(lines 671-697) callmatrix_transpose()which allocates a new SuiteSparse matrix. The pointer is used for the NIF call but never freed afterward. The variables_a,_b,_matrixare captured with_prefix (acknowledging they're unused) but this is the code smell — they should be freed.Fix: Track which pointers were transposed and free them after the operation:
Impact: Unbounded memory growth in any application using transpose descriptors.
3. Ignored Error Codes in Zig NIFs (undefined behavior)
Multiple
GrB_Matrix_nrows,GrB_Matrix_ncols,GxB_Matrix_typecalls discard return values with_ = ..., leaving variables uninitialized on failure. If the GrB call fails, the variable declared withundefinedis used in subsequent operations — classic undefined behavior.nrows_a,ncols_bnrowsnrows,ncolsnrows,ncolsnrows,ncolsnrowssizesizencolsFix: Replace all
_ = GrB_Matrix_nrows(...)withtry translate_info(GrB_Matrix_nrows(...)).Note: Contrast with
matrix_nrows()at line 329 which correctly does:4. Silent Type Defaults in Zig (wrong results, not crashes)
Invalid type/semiring/monoid codes silently default to INT64 variants instead of returning errors. An fp64 operation accidentally passed with an invalid code would compute using INT64 arithmetic and return wrong results with no error.
type_code_to_grb_typeelse => GrB_INT64semiring_from_codeelse => GrB_PLUS_TIMES_SEMIRING_INT64monoid_from_codeelse => GrB_PLUS_MONOID_INT64Fix: Return an error for unknown codes and surface it as
{:error, %Error{reason: {:invalid_argument, code}}}to Elixir.P1 — High Priority (fix before release)
5. Documentation Outdated / Incorrect
SuiteSparse.matrix_free()should beGraphBLAS.Backend.SuiteSparse.matrix_free()GrB_Descriptor_set_INT32EX_GRAPHBLAS_COMPILE_NATIVEandEX_GRAPHBLAS_BUILD~> 1.17, guide says>= 1.18— reconcile/opt/homebrew(Apple Silicon), Intel uses/usr/local6. Unsafe
hd()Callalgorithm.ex:259callshd(unvisited_entries)which crashes on empty list. Thenvals == 0check above (line 257) should prevent this, but a defensive pattern match is safer:7. Missing @doc/@SPEC in SuiteSparse Backend
native/suite_sparse_backend.exhas 30@impl GraphBLAS.Backendcallback functions with zero@docor@specannotations. These functions appear in generated ExDoc documentation as undocumented.Affected functions (all in native/suite_sparse_backend.ex):
Lines 56, 71, 86, 94, 97, 100, 117, 146, 171, 196, 221, 242, 261, 282, 305, 328, 346, 360, 374, 382, 385, 388, 405, 430, 455, 480, 500, 519, 540, 561
Only
matrix_free(line 580) andvector_free(line 591) have specs.8. Unprotected NIF Calls Outside try/rescue
Several NIF calls in
suite_sparse_backend.exare outsidetry/rescueblocks. If the NIF crashes, the exception propagates unhandled:matrix_nvals(ptr)matrix_to_cooextractmatrix_nrows(ptr)matrix_mxmNIFmatrix_ncols(ptr)matrix_mxmNIFvector_size(ptr)matrix_mxvNIFmatrix_nrows(ptr)matrix_reduceNIFvector_nvals(ptr)vector_to_entriesextract9. Missing Input Validation in Zig Build Functions
The
nvalsparameter is passed separately from array slices. Ifnvalsdoesn't matchrows.len,cols.len, orvals.len, the C function will read past array bounds:matrix_build_int64matrix_build_fp64matrix_build_boolvector_build_int64vector_build_fp64vector_build_boolFix: Add assertions:
std.debug.assert(nvals <= rows.len and nvals <= cols.len and nvals <= vals.len)10. Closure Loop Limit Hardcoded
relation.ex:265hardcodes100as max iterations for the closure loop. Should be configurable viaopts:11.
@sssp_infNot Configurablealgorithm.ex:29defines@sssp_inf 1.0e18as a module attribute. This should be configurable viaoptsinsssp/3for users with different scale requirements.12. GrB_finalize and JIT Disable Errors Ignored
GrB_finalize()_ =GxB_Global_Option_set_INT32(GxB_JIT_C_CONTROL, GxB_JIT_OFF)13. ZigStub Backend Doesn't Validate Dimensions
lib/ex_graphblas/backend/zig_stub.exlines 79-87 (matrix_new) and 134-142 (vector_new) don't validate thatnrows,ncols, andsizeare non-negative. The Elixir backend validates these (elixir.ex:336-347), creating inconsistent behavior between backends.P2 — Moderate Priority (fix for quality)
14. Test Gaps — Missing Edge Cases
assert entries != [](algorithm_test.exs:310),assert is_list(result)(algorithm_test.exs:452)matrix_settype coercion (int into fp64 matrix)available?()but no test for unavailable case15. Test Gaps — Missing Mathematical Property Tests
The property_test.exs file has 25 property-based tests but misses fundamental algebraic properties:
16. Test Gaps — Incomplete Backend Callback Coverage
~24/30 backend callbacks are tested. Missing or incomplete:
matrix_new/4matrix_from_coo/5vector_from_entries/4matrix_set/4,vector_set/3matrix_dup/1,vector_dup/117. CI Workflow Issues
--covertomix testwhenmatrix.coverage == true2>/dev/nullswallows checksum download errors2>/dev/null, check exit codeexcoverallsmissingruntime: falseruntime: falsenative/orbench/18. Config Issues
config/dev.exshardcodesforce_build_all: trueEX_GRAPHBLAS_BUILDenv varconfig/prod.exsdefaults to Elixir backendsuitesparse_include_path19. SuiteSparse Test Guards Lack Skip Messages
Tests in
parity_test.exs,suite_sparse_test.exs,property_test.exsare guarded withif System.get_env("EX_GRAPHBLAS_COMPILE_NATIVE"), but when native is not compiled, tests silently disappear. No indication in test output that native tests were skipped.Fix: Add explicit skip module:
20. Missing Error Handling Documentation
BinaryOp.fn_for/1ArgumentErroron unknown op but @doc doesn't mention itUnaryOp.fn_for/1BinaryOp.new/1Keyword.fetch!— raisesKeyErroron missing keys but @doc doesn't sayMonoid.new/1Semiring.new/1UnaryOp.new/121. Incomplete @moduledoc
GraphBLAS.Helpers@moduledoc false— should explain internal utilitiesGraphBLAS.Application@moduledoc false— should explain SuiteSparse init/finalize lifecycle22. Application.ex Silently Swallows Errors
application.exlines 10-16 and 29-35 use baretry/rescuecatching all exceptions fromgrb_init()andgrb_finalize(). Errors are silently discarded. Should at minimum log withLogger.debug.P3 — Low Priority (polish)
23. Duplicate Constants
Int64/fp64 limit constants are duplicated between
monoid.ex(lines 122-123) andscalar.ex(lines 102-108). Consider extracting to a sharedGraphBLAS.Constantsmodule.24. Algorithm fixed_point Documentation Incomplete
algorithm.ex:405— @doc forfixed_point/3mentionsconvergence_fnoption but doesn't explain default behavior (exact comparison for matrices, tolerance-based for vectors).25. Relation Documentation Missing Backend Inheritance
relation.ex:55(add_triples/4) andrelation.ex:92(add_weighted_triples/5) — @doc doesn't mention that when extending an existing predicate, the backend is inherited from the existing matrix.26. Descriptor Cleanup Logic Fragile
native/suite_sparse_backend.ex:752-758—cleanup_descriptorchecksdescriptor_is_custom()to decide whether to free. This relies on the Zig code correctly identifying custom vs. predefined descriptors. If the Zig code has a bug, descriptors will leak or be double-freed.27. Default SuiteSparse Include Path is macOS-Only
native/suite_sparse.ex:30-37— Hardcoded default path/opt/homebrew/include/suitesparseis macOS Apple Silicon-specific. Linux and Intel Mac users must set the env var or config. Should be documented prominently.28. Missing No-Op Documentation
guides/masks_and_descriptors_guide.md:105-112—replace_output()descriptor is documented as "no-op" and "for future in-place operations". Should clearly state this is not yet functional.Recommended Fix Order
hd()fix (#6)Files Reviewed
lib/
native/
priv/
test/
docs/
config/
CI/tooling
What Passed Review
These areas are well-implemented and need no changes:
@behaviourwith 30 callbacks, consistent across all 3 backendsGraphBLAS.Error, properreasontypes,format_reason/1coverageTypes.validate_scalar_type/1, consistent:int64/:fp64/:boolsupportHelpers.maybe_free/2properly dispatches cleanup per backendvalidate_dimensions/2,validate_size/1,validate_index/2)