Skip to content

Cross-package test-dep leak: viscy-data tests import viscy-transforms #436

@edyoshikun

Description

@edyoshikun

Context

In #435 we added `viscy-transforms` to `packages/viscy-data`'s test group to unblock CI on the 0.5 refactor (`modular-viscy-staging`). That fix is correct in the short term but documents a deeper layering question worth revisiting after 0.5 lands.

The leak

The runtime dependency graph is clean and one-way:

`viscy-data` runtime deps `viscy-data` test deps
Imports `viscy-transforms`? no yes (3 test files)

`viscy-transforms` does not import `viscy-data` in either direction. So the leak is asymmetric and lives only at test time.

Affected tests (3 files, ~3 tests)

Test Needs from `viscy-data` Needs from `viscy-transforms`
`test_fg_mask_aligned_after_gpu_spatial_augmentation` `HCSDataModule._inject_mask_keys` `BatchedRandFlipd`
`test_fg_mask_aligned_after_cpu_rand_spatial_crop` `HCSDataModule._inject_mask_keys` `RandSpatialCropd`
`test_sliding_window_timepoint_statistics_normalize` `SlidingWindowDataset` `NormalizeSampled`

These are genuinely cross-package integration tests — they verify that the data module correctly invokes a transform passed via constructor (fg_mask alignment after a real spatial op, NormalizeSampled receiving the right per-timepoint stats). Mocking the transforms would test the wiring against a fake contract instead of the real one, which the project's testing philosophy explicitly discourages (CLAUDE.md §"Use Real Integration Tests").

Options to consider (not for this issue to resolve)

  1. Status quo — `viscy-data` test extras include `viscy-transforms`. Honest about the coupling. This is what's deployed.
  2. Flip the direction — move the 3 tests to `packages/viscy-transforms/tests/` and add `viscy-data` to `viscy-transforms`' test extras. Same kind of leak, just on the other side.
  3. Split — `_inject_mask_keys` could be unit-tested against a stub transform with a mutable `keys` list (no real spatial op), leaving only the "still aligned after applying" tests as genuine integration tests. Those would still need both deps somewhere.
  4. Top-level integration tests dir — `tests/integration/` at repo root, depends on both packages. Most architecturally clean, adds one new CI target.
  5. Make `viscy-transforms` a runtime dep of `viscy-data` — if the design intent is that `viscy-data` ships with the batched transform layer it's built to interoperate with, declare the dep at runtime instead of test-time.

My read

Option 1 is defensible (sibling packages in a monorepo testing their integration is normal — see FastAPI/starlette, etc.), but it leans on the convention that "data is the orchestrator that consumes transforms" rather than enforcing it at the package boundary. Worth a design discussion, not a refactor right now.

Acceptance / closeout

Pick option 1/2/3/4/5 deliberately. If 1, close as "won't do" with the rationale recorded here.


Surfaced during the CI green-up for PR #435 (`ci/staging-green-up` → `modular-viscy-staging`).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions