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

tanghaibao · 2026-04-17T01:18:45Z

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

This PR modernizes the project by migrating to Python 3 + PEP 517/518 packaging, refreshing the CLI entry points and CI, and cleaning out legacy scripts/config.

Changes:

Replace setup.py/Travis-era packaging with pyproject.toml and console scripts (splithunter_run, splithunter_report).
Update core modules for Python 3 compatibility and safer subprocess/file handling.
Add GitHub Actions CI and new pytest-based tests; update Dockerfile and README accordingly.

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 helper script.
tests/test_splithunter.py	New pytest suite (currently binary-dependent).
tests.py	Removed old test runner.
splithunter_run.py	Removed legacy script wrapper in favor of console entry point.
splithunter_report.py	Removed legacy script wrapper in favor of console entry point.
splithunter/utils.py	Python 3 shebang + simplification/removal of unused utilities.
splithunter/run.py	Modernized runner (paths, subprocess usage, multiprocessing, argparse).
splithunter/report.py	Modernized report generation and pandas usage.
splithunter/init.py	Updated metadata (email/license/version).
setup_helper.py	Removed legacy setup helper.
setup.py	Removed legacy setuptools entry.
rm_submodules.sh	Removed legacy helper script.
pyproject.toml	New build metadata, dependencies, and console scripts.
docker/splithunter.dockerfile	Updated base image + Python 3 install/build flow.
docker/Makefile	Updated test run command to new CLI.
README.md	Updated CI badge, install and usage instructions, dev workflow.
.travis.yml	Removed Travis CI config.
.gitignore	Added modern Python/C++ build artifacts and run outputs.
.github/workflows/ci.yml	Added GitHub Actions CI workflow.

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

Copilot · 2026-04-17T01:24:23Z

+DATA_DIR = get_abs_path(op.join(SRC_DIR, "data"))
+HLI_BAMS = op.join(DATA_DIR, "HLI_bams.csv.gz")


HLI_BAMS is constructed as SRC_DIR/data/HLI_bams.csv.gz, but the repository’s HLI_bams file is under splithunter/data/HLI_bams.csv.gz (and src/data/ does not contain it). This will raise a FileNotFoundError when using the @<samplekey> input mode. Consider resolving HLI_bams.csv.gz from package data (e.g., via importlib.resources) and/or falling back to src/data if present.

Suggested change

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

HLI_BAMS = op.join(DATA_DIR, "HLI_bams.csv.gz")

def resolve_hli_bams():

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

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

for data_dir in (package_data_dir, legacy_data_dir):

hli_bams = op.join(data_dir, "HLI_bams.csv.gz")

if op.exists(hli_bams):

return hli_bams

return op.join(package_data_dir, "HLI_bams.csv.gz")

HLI_BAMS = resolve_hli_bams()

Copilot · 2026-04-17T01:24:23Z

+    # Mode 1: single BAM/CRAM file
+    if csvfile.endswith((".bam", ".cram")):
+        bam = bam_path(csvfile)
        if args.workflow_execution_id and args.sample_id:
            samplekey = "_".join((args.workflow_execution_id, args.sample_id))
        else:
            samplekey = op.basename(bam).rsplit(".", 1)[0]
        return [(samplekey, bam)]

-    fp = open(csvfile)
-    # Mode 2: See if the file contains JUST list of BAM files
-    header = fp.next().strip()
    contents = []
-    if header.endswith(".bam") and header.count(",") == 0:
+    with open(csvfile) as fp:
+        header = fp.readline().strip()
+
+        # 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 a single .cram input (Mode 1), but Mode 2 (list file) only detects .bam headers and Mode 3 only appends rows where the BAM path ends with .bam. This makes list/CSV inputs containing CRAMs silently produce no samples. Consider treating .cram the same as .bam in these modes (both for header detection and row filtering).

Copilot · 2026-04-17T01:24:23Z

        task_args.append((samplekey, bam, args))

-    cpus = min(args.cpus, len(task_args))
+    cpus = min(args.cpus, len(task_args)) if task_args else 0


--cpus can be set to 0 (or a negative number). In that case cpus = min(args.cpus, len(task_args)) becomes 0 even when there is pending work, causing the run to log "All jobs already completed" and skip execution. Consider validating args.cpus >= 1 (argparse type/custom check) and/or coercing to at least 1 before computing the pool size.

Suggested change

cpus = min(args.cpus, len(task_args)) if task_args else 0

if task_args:

cpus = min(max(1, args.cpus), len(task_args))

else:

cpus = 0

Copilot · 2026-04-17T01:24:24Z

        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)
        df_to_tsv(df, tsvfile)


If a user passes --cpus 0, cpus = min(nfiles, args.cpus) becomes 0 and Pool(processes=cpus) will raise a ValueError. Consider validating args.cpus >= 1 (and/or clamping cpus to at least 1 when files is non-empty).

Copilot · 2026-04-17T01:24:24Z

+      - 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 unit tests are marked skipif when the C++ Splithunter binary is not built, but this workflow never builds it (no make -C src). As a result, pytest will typically skip the new tests and CI won’t exercise the core functionality. Consider adding a step to initialize submodules and build the binary (and any required system deps) before running pytest, or adjust the tests to cover pure-Python behavior that can run in CI without the binary.

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:24:25Z

+
+on:
+  push:
+    branches: [master, modernize]


Include main branch in CI push triggers

The workflow only runs on pushes to master and modernize, so pushes to the repository’s active main branch won’t execute CI at all. This creates a blind spot where regressions can be merged without any automated checks; adding main to the push branch filter restores expected coverage.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-04-17T01:24:25Z

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


Build Splithunter binary before running test suite

The workflow installs Python dependencies and runs pytest, but it never builds the C++ Splithunter executable first. In this commit the tests are guarded by skipif when that binary is missing, so CI can pass while skipping the core end-to-end tests entirely; adding a cd src && make step (or equivalent) before pytest is needed to actually exercise runtime behavior.

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:18

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

tanghaibao merged commit dd6eb45 into master Apr 17, 2026
6 checks passed

Copilot AI reviewed Apr 17, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Apr 17, 2026

View reviewed changes

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

Navigation Menu

Modernize - #1

Modernize#1
tanghaibao merged 4 commits into
masterfrom
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!

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"))
		HLI_BAMS = op.join(DATA_DIR, "HLI_bams.csv.gz")

-DATA_DIR = get_abs_path(op.join(SRC_DIR, "data"))
-HLI_BAMS = op.join(DATA_DIR, "HLI_bams.csv.gz")
+def resolve_hli_bams():
+    package_data_dir = get_abs_path(op.join(PACKAGE_ROOT, "data"))
+    legacy_data_dir = get_abs_path(op.join(SRC_DIR, "data"))
+    for data_dir in (package_data_dir, legacy_data_dir):
+        hli_bams = op.join(data_dir, "HLI_bams.csv.gz")
+        if op.exists(hli_bams):
+            return hli_bams
+    return op.join(package_data_dir, "HLI_bams.csv.gz")
+HLI_BAMS = resolve_hli_bams()

-    cpus = min(args.cpus, len(task_args)) if task_args else 0
+    if task_args:
+        cpus = min(max(1, args.cpus), len(task_args))
+    else:
+        cpus = 0

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!

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