Skip to content

Address deferred review cleanup on staging#386

Merged
edyoshikun merged 10 commits into
modular-viscy-stagingfrom
fix/deferred-review-cleanup
Mar 25, 2026
Merged

Address deferred review cleanup on staging#386
edyoshikun merged 10 commits into
modular-viscy-stagingfrom
fix/deferred-review-cleanup

Conversation

@alxndrkalinin

@alxndrkalinin alxndrkalinin commented Mar 19, 2026

Copy link
Copy Markdown
Collaborator

Addressing issues leftover from integration of app-dynaclr and app-cytoland in #379 (comment)

Summary

  • viscy-utils treated anndata and wandb as core dependencies even though they are only needed for optional evaluation and embedding-export workflows; this forced every consumer to install heavier packages unnecessarily. This PR moves them behind optional extras, keeps tensorstore core because it is still on the CLI import path, and declares viscy-data explicitly because the embedding callbacks import viscy_data at runtime.
  • embedding snapshot code depended on the private EmbeddingWriter._collect_data_provenance() helper, and annotation.convert() both stacked xarray coords without explicit .data extraction and imported optional anndata/natsort at module import time. This PR exposes collect_data_provenance() as a public helper shared by both callbacks, makes the coordinate stacking explicit to avoid fragile xarray-to-numpy coercion, and lazy-imports the optional annotation dependencies with a clear runtime error only when conversion is invoked.
  • dynaclr used deprecated nn.functional.sigmoid() in classification prediction. This PR replaces it with torch.sigmoid().
  • dynaclr tests relied on a root-level pytest pythonpath hack so helpers.py could be imported as a top-level module, which leaked a test-only module into unrelated root test sessions. This PR folds the shared test helpers into conftest.py, updates imports, adds app-local pytest config for dynaclr, and removes the root pythonpath override so test setup stays local to the app.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR cleans up staging-related technical debt across the monorepo by tightening viscy-utils dependency metadata, exposing a shared provenance helper, improving xarray coordinate handling during annotation conversion, and removing a brittle pytest pythonpath leak in DynaCLR tests.

Changes:

  • Refactor viscy-utils dependency/extras metadata (move anndata/natsort/wandb to extras; add viscy-data runtime dep) and expose collect_data_provenance as a public helper.
  • Tighten xarray coordinate extraction in AnnData conversion and update callbacks to use the shared provenance helper.
  • Make DynaCLR tests self-contained by inlining shared helpers into conftest.py, updating imports, and adding app-local pytest pythonpath; replace deprecated nn.functional.sigmoid usage.

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
uv.lock Updates locked dependency/extras metadata for viscy-utils, including new viscy-data runtime dep and extras markers.
pyproject.toml Removes root-level pytest pythonpath entry that leaked DynaCLR test paths globally.
packages/viscy-utils/pyproject.toml Moves anndata to an extra, adds viscy-data runtime dep, and redefines extras/dev groups.
packages/viscy-utils/src/viscy_utils/evaluation/annotation.py Uses .data for embedding coordinate stacking during xarray→AnnData conversion.
packages/viscy-utils/src/viscy_utils/callbacks/embedding_writer.py Promotes data provenance extraction to a public helper and reuses it.
packages/viscy-utils/src/viscy_utils/callbacks/embedding_snapshot.py Switches to the shared provenance helper.
applications/dynaclr/tests/helpers.py Removes standalone helper module; functionality moved into conftest.py.
applications/dynaclr/tests/conftest.py Inlines shared constants/helpers and keeps pytest config/fixtures in one place.
applications/dynaclr/tests/test_*.py Updates imports to pull shared helpers/constants from conftest.
applications/dynaclr/src/dynaclr/classification.py Replaces deprecated nn.functional.sigmoid with torch.sigmoid.
applications/dynaclr/pyproject.toml Adds app-local pytest config (pythonpath = ["tests"]).

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

Comment thread packages/viscy-utils/src/viscy_utils/evaluation/annotation.py
@edyoshikun

Copy link
Copy Markdown
Member

Sorry, we can remove 69a1e93 from this PR.

the other two commits were to remove some miscellaneous folders and files that were left over. I did have to explicitly add the CI the test applictations/*.

I had issues running uv run pytest from root.

edyoshikun and others added 3 commits March 20, 2026 13:01
…s into the app directory before running pytest.
…assifier deps

Remove applications/*/tests from root testpaths — multiple app conftest.py
files collide as the same plugin name when collected together. Apps are now
tested per-directory via CI matrix (added in prior commit).

Lazy-import wandb and anndata in linear_classifier.py (same pattern as
annotation.py) so the module is importable without optional extras.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@alxndrkalinin
alxndrkalinin force-pushed the fix/deferred-review-cleanup branch from 36e9af7 to 6bcf130 Compare March 20, 2026 23:42
alxndrkalinin and others added 2 commits March 20, 2026 22:17
Remove pythonpath = ["tests"] from app configs and __init__.py from app
test dirs — both caused conftest plugin collisions when root pytest
collected multiple applications together. Replace all `from conftest
import` / `from .conftest import` with either inlined constants or
fixture factories so test files no longer need conftest on sys.path.

Root testpaths now includes applications/*/tests, so `uv run pytest`
runs all 836 tests (packages + applications) in a single invocation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace inlined SYNTH_C/D/H/W, IMG_H/W, N_T/Z/TRACKS, FCMAE_H/W, and
MIXED_LOSS_H/W constants with synth_dims and hcs_dims fixtures returning
dicts. Single source of truth in each app's conftest.py.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 28 out of 34 changed files in this pull request and generated 2 comments.


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

Comment thread pyproject.toml
Comment thread packages/viscy-utils/src/viscy_utils/callbacks/embedding_writer.py
alxndrkalinin and others added 2 commits March 23, 2026 12:51
Hatchling requires the package directory referenced in
[tool.hatch.build.targets.wheel] to exist. Without it, `uv build`
fails and `import viscy` breaks.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Keep empty src/viscy/__init__.py (required by hatchling wheel build).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@alxndrkalinin

alxndrkalinin commented Mar 23, 2026

Copy link
Copy Markdown
Collaborator Author

Latest changes:

  • Dropped 69a1e93 as out-of-scope WandB/training config commit.
  • Lazy-imported wandb and anndata in linear_classifier.py** following the same pattern as the annotation.py fix already in this PR
  • Eliminated all from conftest import across test files and replaced with inlined constants (via synth_dims/hcs_dims fixture dicts) and fixture factories for classes/functions. Removed pythonpath = ["tests"] from app configs and __init__.py from app test dirs. Moved constants to fixtures.
  • Root uv run pytest now collects all packages and applications (852 tests in one invocation)
  • Restored empty src/viscy/__init__.py because hatchling requires it for the wheel build target. Without it uv build fails.

All tests pass:

  • Root uv run pytest: 848 passed, 6 skipped, 1 xfailed
  • applications/dynaclr: 205 passed, 4 skipped
  • applications/cytoland: 21 passed
  • applications/qc: 15 passed

@alxndrkalinin

Copy link
Copy Markdown
Collaborator Author

@edyoshikun I fixed conftest issues and some other small inconsistencies, should be ready for review

@edyoshikun
edyoshikun merged commit 43e0a7e into modular-viscy-staging Mar 25, 2026
@edyoshikun
edyoshikun deleted the fix/deferred-review-cleanup branch March 25, 2026 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants