ome / napari-ome-zarr

A napari plugin for zarr backed OME-NGFF images
https://www.napari-hub.org/plugins/napari-ome-zarr
BSD 3-Clause "New" or "Revised" License
27 stars 21 forks source link

Add vispy to requirements #26

Closed DragaDoncila closed 2 years ago

DragaDoncila commented 2 years ago

Hi team,

As part of our work on the new napari plugin engine, we’ve been running some automated tests to see whether plugins will be easily converted to the new plugin engine. While running these tests, we noticed that your plugin fails to be imported after installation in a fresh environment due to a missing requirement. You can see details in this github action run.

This PR therefore adds vispy to your install requirements so that users can install your plugin more confidently.

Note that this may not be a complete list - to check whether your plugin can be easily installed on other machines, we recommend using GitHub workflows to test your plugin in a fresh environment - the napari plugin cookiecutter provides an example of such a workflow.

If you believe this PR has been opened in error, please feel free to close it and let us know where we went wrong!

codecov-commenter commented 2 years ago

Codecov Report

Merging #26 (1cd5e1e) into main (a27ee1d) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #26   +/-   ##
=======================================
  Coverage   92.77%   92.77%           
=======================================
  Files           3        3           
  Lines         180      180           
=======================================
  Hits          167      167           
  Misses         13       13           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a27ee1d...1cd5e1e. Read the comment docs.

joshmoore commented 2 years ago

Thanks, @DragaDoncila! Happy to re-add. I think the vispy dependency was dropped from the ome-zarr-py repo due to dependency issues, but here happy to follow your lead.

This repo does have a test_and_deploy.yml workflow with a minimal diff from upstream. Any ideas why that didn't trigger the issue you were seeing? Do we need to additionally start testing wit the new engine?

DragaDoncila commented 2 years ago

Hi @joshmoore, I think it is good to have the explicit dependency on vispy because you are importing directly from vispy.color in your _reader. An alternative to this would be to depend on napari itself which would ensure vispy was available in clean envs, but that's a much bigger dependency.

This repo does have a test_and_deploy.yml workflow with a minimal diff from upstream. Any ideas why that didn't trigger the issue you were seeing?

The reason the test_and_deploy.yml workflow didn't surface this is because napari is installed as a dependency in tox.ini here, which then goes on to install vispy.

We don't explicitly add napari to requirements in the cookiecutter template because there are some plugin packages providing functionality outside of just plugin functionality. However, looking through the different plugins as part of this conversion process, we may need to start providing a lightweight napari dependency for typing/layer models/etc. that doesn't also install a qt backend, etc.

Do we need to additionally start testing wit the new engine?

We recommend moving your plugin to the new plugin architecture as soon as possible. The conversion process is simple and is explained in this guide - note that this is all still a work in progress however, and there may be frequent changes made to the docs.

The testing process for plugins should not change however, save for needing an explicit dependency on npe2, for now.

joshmoore commented 2 years ago

Filed #27 to capture the update, but merging for now. Thanks, again.