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)
- Status quo — `viscy-data` test extras include `viscy-transforms`. Honest about the coupling. This is what's deployed.
- 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.
- 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.
- Top-level integration tests dir — `tests/integration/` at repo root, depends on both packages. Most architecturally clean, adds one new CI target.
- 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`).
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-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)
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)
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`).