Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add vispy to requirements #26

Merged
merged 1 commit into from
Jan 6, 2022
Merged

Add vispy to requirements #26

merged 1 commit into from
Jan 6, 2022

Conversation

DragaDoncila
Copy link
Contributor

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
Copy link

codecov-commenter commented Dec 21, 2021

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
Copy link
Member

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
Copy link
Contributor Author

DragaDoncila commented Dec 23, 2021

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
Copy link
Member

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

@joshmoore joshmoore merged commit 63a535b into ome:main Jan 6, 2022
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