Modernize by tanghaibao · Pull Request #2 · tanghaibao/Splithunter

tanghaibao · 2026-04-17T01:32:11Z

No description provided.

- Replace Py2-only constructs (print statements, fp.next, basestring, df.ix, df.reindex_axis) with Py3 equivalents - Swap os.system shell invocation for subprocess.run with a list of args, avoiding shell injection via bam/samplekey paths - Point datadir/exec_path at src/data and src/Splithunter so the package finds the compiled binary and bundled indices - Simplify get_abs_path via os.path.realpath (fixes pathological recursion) and drop unused code (InputParams, Popen/popen, sh, s3 helpers) - Rewrite tests/test_splithunter.py with real assertions, tmp_path isolation, and a skip guard when the C++ binary is absent - Drop legacy top-level wrappers now exposed as console scripts Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- Replace setup.py + setup_helper.py with a pyproject.toml declaring Python 3.8+, BSD-3-Clause license, and console_scripts entry points for splithunter_run / splithunter_report - Swap retired Travis config for a GitHub Actions workflow testing Python 3.9 and 3.11 with submodule checkout - Add a root .gitignore covering Python build artifacts, compiled C++ outputs, and splithunter work directories - Upgrade the Dockerfile to Ubuntu 22.04, Python 3, pinned apt dependencies with cleanup, and the new console-script entry point - Refresh README with CI badge, updated email/license, and Py3 install instructions - Drop one-line helper scripts (update/rm submodules) that belong in README documentation Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The tanghaibao/SeqLib fork is gone, so recursive checkout aborted the workflow. The Python test suite already skips when the Splithunter C++ binary is absent, so running CI without the submodule still validates the Python side. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The pyproject.toml console_scripts wrappers call main() with no arguments; argparse then parses sys.argv on its own. Caught by running splithunter_run --help after pip install. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Modernizes the Splithunter project by moving to Python 3, updating packaging to pyproject.toml with console entry points, and replacing legacy Travis CI with GitHub Actions (plus refreshed Docker and tests).

Changes:

Migrate packaging from setup.py to pyproject.toml and expose splithunter_run / splithunter_report console scripts.
Update Python codebase for Python 3 (subprocess usage, pandas API updates, safer filesystem helpers).
Replace Travis with GitHub Actions CI and add pytest-based integration tests (currently gated on the C++ binary).

Reviewed changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
update_submodules.sh	Removed legacy submodule update helper script.
rm_submodules.sh	Removed legacy submodule removal helper script.
tests/test_splithunter.py	Added pytest tests for run/report output (skipped if binary missing).
tests.py	Removed legacy test runner script.
splithunter_run.py	Removed legacy script wrapper (replaced by console script entry point).
splithunter_report.py	Removed legacy script wrapper (replaced by console script entry point).
splithunter/utils.py	Simplified utilities; updated for Python 3.
splithunter/run.py	Refactored runner (Python 3, subprocess, argument handling, path handling).
splithunter/report.py	Refactored reporting (Python 3, pandas API updates, multiprocessing changes).
splithunter/init.py	Updated package metadata (email/license/version).
setup_helper.py	Removed legacy setup helper.
setup.py	Removed legacy setuptools build/install script.
pyproject.toml	Added modern packaging metadata + dependencies + console scripts.
docker/splithunter.dockerfile	Updated base image and Python 3 install flow; builds C++ binary then installs package.
docker/Makefile	Updated container testrun command to new console script.
README.md	Updated CI badge, install instructions, CLI usage, dev workflow, and license text.
.travis.yml	Removed Travis CI configuration.
.gitignore	Added ignores for Python/C++ build artifacts, workdirs, and editor files.
.github/workflows/ci.yml	Added GitHub Actions CI workflow (syntax check + pytest).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T01:36:51Z

        if op.exists(jsonfile):
            continue
        task_args.append((samplekey, bam, args))



--cpus can be passed as 0 (or a negative number), which makes cpus compute to 0 and causes the run to incorrectly report "All jobs already completed." even when there is pending work. Add argparse validation (e.g., type function enforcing >=1) or explicitly error when args.cpus < 1.

Suggested change

if args.cpus < 1:

p.error("--cpus must be >= 1")

Copilot · 2026-04-17T01:36:51Z

+        # Mode 2: just a list of BAM files
+        if header.endswith(".bam") and header.count(",") == 0:
+            fp.seek(0)
+            for row in fp:
+                bam = bam_path(row.strip())
+                samplekey = op.basename(bam).rsplit(".", 1)[0]
+                contents.append((samplekey, bam))
+            return contents
+
+        # Mode 3: CSV with samplekey,bam
        fp.seek(0)
        for row in fp:
-            bam = row.strip()
+            atoms = row.strip().split(",")
+            if len(atoms) < 2:
+                continue
+            samplekey, bam = atoms[:2]
            bam = bam_path(bam)
-            samplekey = op.basename(bam).rsplit(".", 1)[0]
-            contents.append((samplekey, bam))
-        return contents
-
-    # Mode 3: Continue reading, this is a CSV file
-    fp.seek(0)
-    for row in fp:
-        atoms = row.strip().split(",")
-        samplekey, bam = atoms[:2]
-        bam = bam_path(bam)
-        if bam.endswith(".bam"):
-            contents.append((samplekey, bam))
+            if bam.endswith(".bam"):
+                contents.append((samplekey, bam))


read_csv() supports .cram for the single-file mode, but Mode 3 (CSV with samplekey,bam) only keeps rows where the path ends with .bam. This makes .cram entries in CSV silently ignored. Consider accepting both extensions here (and in the Mode 2 “list of BAMs” detection) for consistency.

Copilot · 2026-04-17T01:36:52Z

@@ -129,10 +122,9 @@ def main(args):
    if files:
        nfiles = len(files)
        cpus = min(nfiles, args.cpus)
-        suffix = "JSON"
-        print >> sys.stderr, "Using {} cpus to parse {} {} files"\
-                .format(cpus, nfiles, suffix)
-        df = json_to_df(files, tsvfile, cpus)
+        print("Using {} cpus to parse {} JSON files".format(cpus, nfiles),
+              file=sys.stderr)
+        df = json_to_df(files, cpus)


--cpus has no lower-bound validation. If a user passes --cpus 0 (or negative), Pool(processes=cpus) will raise at runtime. Add argparse validation to enforce cpus >= 1, or clamp to at least 1 when files are provided.

Copilot · 2026-04-17T01:36:52Z

+      - uses: actions/checkout@v4
+
+      - name: Set up Python ${{ matrix.python-version }}
+        uses: actions/setup-python@v5
+        with:
+          python-version: ${{ matrix.python-version }}
+
+      - name: Install package
+        run: |
+          python -m pip install --upgrade pip
+          pip install -e ".[test]"
+
+      - name: Syntax check
+        run: |
+          python -m compileall -q splithunter
+
+      - name: Run unit tests
+        run: pytest tests/ -v


The GitHub Actions job runs pytest, but the only tests added are skipped unless the C++ Splithunter binary is built. Since the workflow doesn’t build the binary (and doesn’t fetch the src/SeqLib submodule), CI will pass without exercising the main functionality. Either compile the binary in CI (including submodules: recursive in actions/checkout) or add unit tests that don’t require the binary so CI provides meaningful coverage.

Copilot · 2026-04-17T01:36:52Z

+PACKAGE_ROOT = op.dirname(op.abspath(__file__))
+REPO_ROOT = op.dirname(PACKAGE_ROOT)
+SRC_DIR = op.join(REPO_ROOT, "src")
+DATA_DIR = get_abs_path(op.join(SRC_DIR, "data"))


HLI_BAMS is built from SRC_DIR/data, but this repository’s HLI_bams.csv.gz lives under splithunter/data/ (and there is no src/data/HLI_bams.csv.gz). As written, get_HLI_bam() will fail at runtime for @<SampleKey> inputs. Point HLI_BAMS at the packaged data file (e.g., via importlib.resources) or move/copy the CSV into src/data if that’s the intended location.

Suggested change

DATA_DIR = get_abs_path(op.join(SRC_DIR, "data"))

DATA_DIR = get_abs_path(op.join(PACKAGE_ROOT, "data"))

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6dd706d8d6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-04-17T01:38:42Z

+      - name: Install package
+        run: |
+          python -m pip install --upgrade pip
+          pip install -e ".[test]"


Build native binary before executing unit tests

This workflow installs only the Python package (pip install -e ".[test]") and then runs pytest, but tests/test_splithunter.py marks both tests with skipif(not _binary_available()), where _binary_available() requires the compiled src/Splithunter executable. Without a build step (for example cd src && make) in CI, the test job goes green while skipping the functional tests entirely, so regressions in the main run/report paths can slip through undetected.

Useful? React with 👍 / 👎.

tanghaibao and others added 4 commits April 16, 2026 18:03

Copilot AI review requested due to automatic review settings April 17, 2026 01:32

Copilot started reviewing on behalf of tanghaibao April 17, 2026 01:32 View session

tanghaibao merged commit 732e6d5 into main Apr 17, 2026
12 checks passed

tanghaibao deleted the modernize branch April 17, 2026 01:32

Copilot AI reviewed Apr 17, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Apr 17, 2026

View reviewed changes

Navigation Menu

Modernize - #2

Modernize#2
tanghaibao merged 4 commits into
mainfrom
modernize

tanghaibao commented Apr 17, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	DATA_DIR = get_abs_path(op.join(SRC_DIR, "data"))
	DATA_DIR = get_abs_path(op.join(PACKAGE_ROOT, "data"))

Conversation

tanghaibao commented Apr 17, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants