Skip to content

Consolidate the PhIP-Seq suite into a single repository #203

Description

@jaredgalloway

Consolidate the PhIP-Seq suite into a single repository

Important

This spec has not been reviewed by a human. It was drafted from a
codebase exploration and answers to a handful of scoping questions, then
raised without a review pass.

Before implementing anything, re-review this spec end to end and
confirm it with the maintainer. Treat its claims as findings to verify,
not as settled decisions. Pay particular attention to:

  • The three-repository fates, especially keeping matsengrp/phip-flow
    alive as a release mirror. That constraint drives most of the design.
  • The two id_coordinate_from_query_df defects described under
    Porting the viz app. Reproduce both before fixing them.
  • The Out of scope section — particularly that this work does not
    fix the .phip pickle incompatibility that phip-viz issue 15 reports.

If re-review changes the shape of the work, update this spec (and the
issue body) before writing an implementation plan.

Goal

Make matsengrp/phippery the single development home for the PhIP-Seq suite — the phippery Python library, the phip-flow Nextflow pipeline, and the phip-viz Streamlit app — so that the three components carry one version, one CI, and one issue tracker.

Two outcomes define success:

  1. A change to phippery that breaks the pipeline or the viz app fails CI
    in the pull request that introduces it. Today nothing catches this.
  2. nextflow run matsengrp/phip-flow -r VX.XX keeps working, for every
    tag that works today and for every future release.

Why now

The coupling is already failing in front of outside users. phip-viz issue 15 (June 2025) reports that the published quay.io/matsengrp/phip-viz container cannot open output from current phippery. The app pins phippery==1.0.6 while the library is at 1.3.1.

That pin is not merely conservative — it is load-bearing. streamlit_app.py calls two functions that no longer exist:

Call site Function Present in 1.3.1?
streamlit_app.py:48 sample_id_coordinate_from_query no
streamlit_app.py:51 peptide_id_coordinate_from_query no

Unpinning produces NameError at runtime. The app is dead code against HEAD, not merely stale.

The pipeline is coupled harder still. phippery is embedded as inline Python inside Nextflow process bodies — workflows/statistics.nf:29,45,68 and workflows/edgeR_BEER.nf:64 — as well as imported in three bin/ scripts. All four of those inline sites use from phippery.utils import *, as do two of the three bin/ scripts, so the dependency surface cannot be enumerated statically. A rename in phippery/utils.py surfaces as a NameError inside a Nextflow work directory, at run time, in a user's analysis.

Neither phip-flow nor phip-viz has any CI at all.

Meanwhile the documentation was consolidated years ago. phippery/docs already contains alignments-pipeline.rst and streamlit-app.rst; phip-flow has no docs of its own, and even its parameter reference lives on the phippery docs site. This work makes the code match a consolidation that already happened in the docs.

Layout

phippery/
├── phippery/            library — PyPI package, unchanged
├── pipeline/            main.nf, workflows/, bin/, templates/, data/
├── viz/                 streamlit app, ported to the 1.3.1 API
├── docs/                already documents all three
├── test/                unit tests + new integration test
├── pixi.toml            dev environment and run tasks
└── .github/workflows/   one CI, plus the pipeline mirror job

pip install phippery is unchanged: [tool.setuptools] packages = ["phippery"] already names the single package explicitly, so adding sibling directories does not alter the distribution. pipeline/ and viz/ are run from the repository or from containers; neither is packaged for PyPI.

What happens to the three repositories

Repository Fate
matsengrp/phippery Development home for all three components.
matsengrp/phip-flow Stays live as a release mirror. Existing tags are never touched. V1.15 onward are pushed from monorepo pipeline/.
matsengrp/phip-viz Frozen and deprecated. A README banner points at the monorepo. No code changes and no further releases.

phip-flow must stay live because Nextflow resolves owner/repo to a repository root: nextflow run matsengrp/phip-flow -r V1.12 clones that repository and looks for main.nf at its top level. The command appears in the project README and, in all likelihood, in published methods sections. It cannot be allowed to break.

phip-viz needs no such treatment. It has never been tagged, has no releases to preserve, and its only documented entry point is a container image that the monorepo will supersede.

Stopping the version skew

Moving directories does not by itself prevent skew. Three mechanisms do, and they are the substance of this work.

An integration test that runs the pipeline against HEAD. A CI job runs the pipeline end to end on the bundled pan-cov-example data, against the working tree's phippery rather than a pinned release. This is the gate that catches a wildcard import breaking. It is the single most valuable piece of this spec: it converts a class of silent, user-facing runtime failure into a red pull request.

Concretely, the job must:

  • Build the pipeline container from the working tree — installing
    phippery from the checkout, not from git+https://...@1.3.1 as the current Dockerfile does — so the run exercises HEAD.
  • Run with --run_zscore_fit_predict true and --output_tall_csv true in
    addition to the defaults. The defaults leave run_zscore_fit_predict false, which would skip bin/fit-predict-zscore.py — one of the three bin/ scripts that import phippery, and therefore one of the sites this test exists to protect.
  • Assert a zero exit status, and assert that the expected outputs exist
    and are non-empty: the binary .phip, the wide CSVs, and the tall CSV.
  • Assert that the .phip output loads: phippery.load(...) returns a
    dataset whose sample and peptide counts match the input tables.

A bare "the pipeline exits zero" assertion is not sufficient. Nextflow processes that fail inside a container can still leave a green run under some configurations, and an empty-but-present output file would pass a mere existence check.

Containers pinned by digest, built from the repository's own lockfile. The pipeline currently defaults to quay.io/matsengrp/phip-flow:latest, a mutable tag. Pinning a workflow revision therefore does not pin its software; -r V1.12 can silently pair with any image. The release process records an image digest in nextflow.config, and the image is built from the same lockfile that defines the pixi environment, so there is one source of truth for dependency versions.

One version number. Stamped across phippery/__init__.py, the pipeline manifest, and the image tag. The manifest today has neither name nor version, so a completed run cannot report which pipeline version produced it. Both are added.

Porting the viz app

The app must work against current phippery on the day the monorepo lands. A monorepo that ships a component which crashes on import contradicts its own premise.

The migration target is not a straight substitution, because the natural replacement is itself broken. phippery/utils.py:377 defines:

def id_coordinate_from_query_df(ds, query_df):
    sq = list(query_df.loc[query_df["dimension"] == "sample", "expression"].values)
    sid = id_query(ds, "sample", " & ".join(sq))
    ...

against the signature def id_query(ds, query, dim="sample") at line 465. The arguments are transposed: the dimension is passed as the query and the joined expression as the dimension. This is a distinct defect from the known argument-order issue in dataset_from_csv (issue 197) and must be fixed as part of this work, with a regression test.

There is a second defect in the same function. When a user has queried only one dimension — the common case in the viz UI — the other dimension's join produces an empty string, and pandas.DataFrame.query("") raises ValueError: expr cannot be an empty string. Verified directly. The fix is to return all coordinates along a dimension when no expression constrains it.

Both fixes need regression tests, in test/test_utils.py alongside the existing test_query. Three concrete cases:

  • Transposed arguments. Use the existing make_hardcoded_ds() fixture
    from test/sim_test_generator.py and reuse the expressions already pinned by test_query, whose expected answers are known: participant_id == 1 selects sample ids [4, 5, 6, 7], and is_wt == True selects peptide ids [0, 5]. Build a query frame with those two rows and assert id_coordinate_from_query_df returns exactly that pair. Against the current code this fails: the expression is passed as dim and reaches get_annotation_table(ds, "participant_id == 1"), so the test fails loudly rather than silently returning wrong rows.

  • Single-dimension query. Pass a frame containing only the
    participant_id == 1 sample row and no peptide row. Assert the sample ids are [4, 5, 6, 7] and the peptide ids are the fixture's complete peptide coordinate set. This is the case that currently raises ValueError: expr cannot be an empty string, and it is the ordinary path through the viz UI.

  • Empty frame is identity. Pass a frame with no rows at all. Assert
    both returned id lists equal the dataset's full coordinates. This pins down the boundary the previous case implies, so a future fix cannot satisfy the single-dimension test by returning an empty list.

A useful invariant across all three: the ids returned for a dimension must always be a subset of that dimension's coordinates, and adding a second expression to a dimension must never grow the result — the function composes its expressions with logical AND.

These fixes are not viz-only. id_coordinate_from_query_df has a second caller in shipped code: phippery/cli.py:315, inside the query-table command (imported at cli.py:22). That command is broken today in the same transposed-argument way, so repairing it is a user-visible behavior change to the CLI — a query-table invocation that currently returns wrong coordinates will start returning correct ones, and a single-dimension expression table that currently raises ValueError will start working. Release notes must say so, and the acceptance criteria must cover the CLI path alongside the viz path.

The empty-frame case does not reach the CLI: cli.py:306 rejects an empty expression table before the call. It is still worth pinning by test, because the viz path has no such guard.

With those two fixes in place, the viz port replaces its dead calls:

# before — neither function exists in 1.3.1
sid = sample_id_coordinate_from_query(ds, sq)
pid = peptide_id_coordinate_from_query(ds, pq)

# after
sid, pid = id_coordinate_from_query_df(ds, query_df)

The port also replaces from phippery.utils import * with explicit imports. The wildcard is what allowed these call sites to rot invisibly: there is no import-time error and no attribution at the call site.

Remaining viz work, all already staged on the existing 16-bit-rot-cleanup branch and worth carrying over rather than redoing: move off the end-of-life quay.io/matsengrp/python3.7 base image, replace the abandoned altair_saver with vl-convert-python, pin the dependencies, and delete the dead Attic/ directory and the orphaned config.json.

Two cautions about that branch. It pins phippery==1.2.0, which is still below the current 1.3.1 and would fail this spec's own acceptance criterion — the port must drop that pin in favour of the monorepo's own version. And the phippery==1.0.6 figure quoted under Why now describes phip-viz main, not this branch. Whichever ref is chosen for the subtree import determines which of the two is true, so the Migration section must name the ref explicitly; importing main would discard all of the cleanup work.

Pixi

Overlaps issue #199 ("Add pixi support for development environment and installation", currently a stub). That issue owns the base pixi surface: the [tool.pixi.*] tables, a committed pixi.lock, and sourcing the scientific stack from conda-forge, with PEP 621 metadata and PyPI publishing left intact.

This spec does not restate that work. What it adds on top is the suite-level piece that only exists once the repositories are merged: run tasks that launch the pipeline and the viz app, and the requirement that the pipeline container be built from the same lockfile. Whoever picks up these two issues should settle the ordering first — the natural sequence is #199 establishing the environment, then this issue adding the tasks — and consolidate if that proves simpler than coordinating.

Pixi manages the development environment and provides local run commands:

pixi run test        pytest
pixi run pipeline    nextflow run ./pipeline/main.nf -profile docker
pixi run viz         streamlit run viz/streamlit_app.py
pixi run docs        sphinx build

An important boundary: pixi governs the development and local-run environment, but the pipeline's execution environment remains containers, which Nextflow pulls per process. If the container and the pixi environment disagree about the phippery version, the skew this spec exists to eliminate reappears one layer down. Building the container from the same lockfile is what prevents that.

Per the repository convention, pipeline/ must never import phippery by relative path. Nextflow processes execute inside containers where only the installed package is present. The monorepo makes a local-path shortcut tempting; it would work locally and fail in every real run.

Migration

History-preserving git subtree merges bring phip-flow and phip-viz into the monorepo. The histories are small — 28 MiB is the largest — so this is inexpensive. Note that git log --follow pipeline/<file> reaches pre-merge history, but plain git blame needs -C/--follow for files that also move within the subtree; "history preserved" does not mean blame works unqualified.

The refs to import, which are not both main:

git remote add phip-flow https://github.com/matsengrp/phip-flow
git fetch phip-flow main
git subtree add --prefix=pipeline phip-flow main

git remote add phip-viz https://github.com/matsengrp/phip-viz
git fetch phip-viz 16-bit-rot-cleanup
git subtree add --prefix=viz phip-viz 16-bit-rot-cleanup

For phip-viz the ref is 16-bit-rot-cleanup, not main — importing main would discard the modernization work described above. Both source repos have root-level Dockerfile, README.md, and LICENSE that would collide with phippery's; --prefix places them under pipeline/ and viz/, so no collision occurs.

One detail that is easy to get wrong: git subtree add --prefix=pipeline/ places the source repository's root at pipeline/, so main.nf lands at pipeline/main.nf as intended. But the reverse direction is what the mirror depends on. git subtree split --prefix=pipeline/ reconstructs a history whose root is pipeline/'s contents, which is exactly the layout matsengrp/phip-flow needs — main.nf back at the repository root, where Nextflow expects it. Confirm this on a scratch branch before relying on it.

The release workflow therefore does, in outline:

git subtree split --prefix=pipeline/ -b <release-branch>
git push <phip-flow-remote> <release-branch>:main
gh release create VX.XX --repo matsengrp/phip-flow

This mirror is the plan's main risk: if the export job breaks, pipeline releases silently stop reaching users, and the failure mode is invisible. The workflow must fail loudly rather than skip on error, and the release checklist must verify that the mirrored main.nf actually runs — a nextflow run matsengrp/phip-flow -r <new-tag> -profile docker against the bundled example data, after the push.

Paths inside the pipeline must stay relative to the pipeline root, not the monorepo root, or the mirrored copy breaks while the monorepo copy works. main.nf already uses $baseDir and $projectDir for its default sample_table, peptide_table, and public_epitopes_csv paths, which resolve correctly in both layouts; preserve that pattern.

Out of scope

The .phip pickle format. phippery.dump uses pickle (utils.py:535), so the serialized file embeds numpy and xarray class paths. This is the true root cause of the ModuleNotFoundError: numpy._core that issue 15 reports, and consolidation does not fix it: the monorepo makes versions move together going forward, but archived .phip files remain version-locked. A migration to netCDF or zarr, with load() retaining the ability to read legacy pickles, deserves its own spec. This spec must not be described as fixing that bug.

The broken Quay build. The hdc-workflows phippery image build has been failing since November 2024 and requires Quay organization admin access that this repository cannot grant. Tracked separately in _ignore/quay-escalation.md.

Decoupling the pipeline from phippery internals. Roughly half-started in the existing code: the newest subworkflow (AGG, templates/aggregate_organisms.py) is deliberately built on exported CSVs with plain pandas and touches no phippery, and there is an explicit // TODO move to bin script remove from phippery at workflows/alignment.nf:94. Finishing that migration is worthwhile but independent; this spec makes the coupling visible and tested rather than removing it.

Acceptance criteria

  • Renaming phippery.utils.load on a scratch branch turns the integration-test
    job red. Record the run URL as evidence, then revert. Stated as a hypothetical PR this criterion has no artifact to check, so run it deliberately once.
  • The viz app launches and renders a heatmap against a .phip file
    produced by the current pipeline, with no phippery version pin below the monorepo's own version.
  • id_coordinate_from_query_df has regression tests covering the
    transposed-argument fix, the single-dimension query case, and the empty-frame identity case. Each fails against the current implementation and passes after the fix.
  • The pipeline integration test runs with --run_zscore_fit_predict true,
    so all three phippery-importing bin/ scripts are exercised, and it asserts on output contents rather than exit status alone.
  • nextflow run matsengrp/phip-flow -r V1.12 still runs.
  • A release produces a mirrored, runnable tag on matsengrp/phip-flow.
  • The pipeline manifest reports a name and version.
  • pixi run test, pixi run pipeline, and pixi run viz all work from a
    fresh clone.
  • phippery query-table returns correct coordinates for both a
    single-dimension and a two-dimension expression table.
  • No wildcard import of phippery remains in the merged pipeline or viz
    code: grep -rn 'from phippery.utils import \*' pipeline/ viz/ returns nothing.
  • pip install phippery installs the same distribution as before.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions