Skip to content

refactor: split a deeptools-core crate with a curated public API - #1493

Open
adRn-s wants to merge 9 commits into
masterfrom
deeptools-core-split
Open

adRn-s wants to merge 9 commits into
masterfrom
deeptools-core-split

Conversation

@adRn-s

@adRn-s adRn-s commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator
  • Does the PR contain new feature?
  • Does the PR contain bugfix?
  • Does the PR contain documentation changes?
  • Does the PR contain changes to the galaxy wrapper?
  • Does the PR title follow Conventional Commits?

This is the crate half of the reverted #1485, on its own, with feedback applied. The alignmentSieve changes are in a separate PR.

What changes

  • hp is split into a core rlib (deeptools-core) and an optional Python layer behind the python feature.
  • Public API is only normalization (scale_factor, scale_factor_bamcompare) and deseq_scalefactors. covcalc, filehandler, filtering and the other calc helpers are private.
  • Normalization and ScaleFactorsMethod are enums with FromStr. A bad method string now returns NormalizationError (thiserror) instead of falling through to a default. The println!s and the unused _fraglen argument are gone from the public functions. bamcoverage and bamcompare parse the strings and turn the error into a ValueError.
  • #![warn(missing_docs)], crate level docs, crate metadata. publish = false until we agree on the API.
  • default = [], and [tool.maturin] features = ["python"] builds the extension. cargo tree -e normal shows no pyo3 in the core build, and CI now checks that.
  • Carried over from chore: Split hp into core rlib + optional python layer #1485: crate rename to deeptools-core, the version sync check against release-please, and the Cargo.lock jsonpath fix.

How it was checked

  • cargo test (core only) and cargo test --features python, both without warnings.
  • maturin develop --release, then test_bamCoverage_and_bamCompare.py.
  • Added tests for the new error paths: unknown method names, RPGC with a zero genome size, and a preset scale factor skipping that check.

What requires further feedback

Two things I decided without being sure:

  1. Error scope. Only the public items return proper errors. About 100 panic sites remain in the private modules (55 in filehandler, 29 in covcalc, a few in filtering). They still reach Python as exceptions through pyo3, as before. Converting all of them touches every caller and I'd rather not do it in this PR. Is that acceptable, or do you want it done here?
  2. default = []. I made the python feature opt-in so a plain deeptools-core = ... dependency does not pull pyo3, and let maturin switch it on. The other option is to keep default = ["python"] and make consumers write default-features = false. Do you have a preference?

Also, a fair question about what goes public was raised: I kept it to the two pure modules. filtering and filehandler would be the next candidates, but they are where most of the panics are, so I'd expose them in a follow-up with proper errors.

- crate-type rlib + cdylib; new 'python' feature (default on) gates pyo3
  and the five command modules (bamcoverage, bamcompare, computematrix,
  alignmentsieve, multibamsummary). calc, covcalc, filehandler, filtering
  and normalization are pure Rust and build without python.
- Verified: cargo build with and without the feature; cargo test (51 pass)
…t-in python feature

- pub surface: normalization (typed Normalization/ScaleFactorsMethod enums,
  NormalizationError via thiserror, no printing/panics) and deseq_scalefactors.
  calc helpers, covcalc, filehandler, filtering are private.
- default features empty; maturin enables 'python' so pure-Rust consumers do
  not pull pyo3. CI builds/tests both and asserts pyo3 absent from core tree.
- bamcoverage/bamcompare parse method strings and map errors to ValueError.
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The Rust crate is renamed and can build as a core library without Python bindings. Normalization uses typed inputs and explicit errors. Python bindings use the updated API when enabled, and version checks include Cargo files.

Changes: Rust core extraction and normalization

Layer / File(s) Summary
Typed normalization API and calculations
src/normalization.rs, src/lib.rs, src/calc.rs, src/tests/test_normalization.rs, src/tests/test_calc.rs
Normalization methods and sample statistics use typed values. Calculations return errors for invalid inputs. Tests cover parsing, RPGC validation, presets, zero-read cases, and zero-count bins.
Python feature and build integration
Cargo.toml, src/lib.rs, src/bamcompare.rs, src/bamcoverage.rs, pyproject.toml, src/tests/mod.rs, .github/workflows/test_rust.yml
Python bindings are gated behind the python feature. Python callers parse typed normalization values and map calculation errors to ValueError. CI builds and tests with and without the feature, and checks the default dependency tree for pyo3.
Cargo version synchronization
release-please-config.json, scripts/check_version_sync.py
Release configuration updates the Cargo manifest and lockfile versions. The version-check script compares both with the DeepTools version.

Change: Refactor

Merge Risk: Minimal · up to 024f5

Zero-read normalization and the core dependency check are guarded correctly, leaving no confirmed merge-blocking risk.

Security architecture risk: Low · up to 024f5

The change keeps core calculations separate from Python integration and replaces several silent failures with explicit errors. No expansion of privileged access was identified in the inspected paths. External consumer compatibility and actual build and release outcomes remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated impact is limited to an in-process Rust API, existing Python normalization callers, and repository build and release metadata. The wider Rust API supplies calculation capabilities rather than additional authority over assets or services; external downstream adoption is not established by repository-local evidence.

Trust Boundaries and Controls

  • observed — Caller-supplied method strings cross into typed core calculations through explicit parsing, with errors translated at the Python boundary. Preset scale factors intentionally bypass calculation validation; this behavior is not an authorization bypass and predates the extraction.

Resilience and Maintainability Implications

  • inferred — Centralized zero-read checks and explicit error propagation improve containment of invalid calculations at the existing Python boundary. These checks do not establish validation of every numeric input: presets are returned unchanged, and arbitrary finite-value guarantees are outside the demonstrated contract.
Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed Title names the main change: splitting the hp crate into a deeptools-core crate with a curated public API. Matches the objectives and the Cargo.toml and lib.rs changes. Conventional Commits prefix is …
Description check Passed Description ties to the changeset. It covers the core/python feature split, the public API limits, the NormalizationError and enum changes, the CI pyo3 check, the version sync carry-over, and the test…
Docstring Coverage Passed Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

Title names the main change: splitting the hp crate into a deeptools-core crate with a curated public API. Matches the objectives and the Cargo.toml and lib.rs changes. Conventional Commits prefix is used.

Full details: Description check

Explanation

Description ties to the changeset. It covers the core/python feature split, the public API limits, the NormalizationError and enum changes, the CI pyo3 check, the version sync carry-over, and the test plan.

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Stopped

coderabbitai[bot]

This comment was marked as resolved.

@coderabbitai

coderabbitai Bot commented Oct 26, 2026

Copy link
Copy Markdown

Autofix was enabled. Check current status in the Coding task.

…CI guard

- add NormalizationError::ZeroMappedReads for scale_factor and scale_factor_bamcompare (ReadCount)
- make Python-layer dependencies optional behind the python feature
- parse norm before the RPGC/scalefactor checks in r_bamcoverage and let scale_factor report MissingEffectiveGenomeSize
- write cargo tree output to a file in the pyo3 guard so a failing cargo tree fails the step
@adRn-s
adRn-s requested a review from WardDeb October 26, 2026 10:37
# Conflicts:
#	scripts/check_version_sync.py

This branch has not been deployed

No deployments
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