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 scikit-image[data] to install_requires, because it's required by builtins #5329

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

psobolewskiPhD
Copy link
Member

@psobolewskiPhD psobolewskiPhD commented Nov 11, 2022

Description

This PR adds scikit-image[data] to the install_requires of napari, because it's required by builtins to ensure all the Sample data in File > Open Samples actually work.
builtins doesn't have it's own dependencies/requirements, because while it works as a plugin, it's not installed as such.

Note: at the moment, scikit-image[data] is just pooch a small pure python package, whose dependencies are already part of the napari requirements list. I didn't add pooch directly, because if scikit-image[data] changes how samples are loaded, then we're covered and scikit-image[data] is what builtins requires, so if in the future builtins is spun-off to an installed plugin, then this dependency can be removed.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

References

Closes #5328

#4706 suggests ultimately moving to builtins as it's own repo as a plugin, then could have it's own requirements and this would not be needed.

  • example: the test suite for my feature covers cases x, y, and z
  • example: all tests pass with my change
  • example: I check if my changes works with both PySide and PyQt backends
    as there are small differences between the two Qt bindings.

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@goanpeca
Copy link
Contributor

Looks good @psobolewskiPhD !

Any chance we could add a test to catch this in the future?

@codecov
Copy link

codecov bot commented Nov 11, 2022

Codecov Report

Merging #5329 (93edda7) into main (cd5c314) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5329   +/-   ##
=======================================
  Coverage   89.08%   89.08%           
=======================================
  Files         582      582           
  Lines       49311    49311           
=======================================
  Hits        43929    43929           
  Misses       5382     5382           
Impacted Files Coverage Δ
...layers/image/experimental/_chunked_image_loader.py 88.00% <0.00%> (-8.00%) ⬇️
napari/layers/image/_image_slice.py 97.61% <0.00%> (-2.39%) ⬇️
napari/layers/image/image.py 95.23% <0.00%> (-0.26%) ⬇️
napari/utils/theme.py 89.79% <0.00%> (+0.68%) ⬆️
napari/utils/info.py 83.33% <0.00%> (+1.11%) ⬆️
napari/_qt/__init__.py 55.17% <0.00%> (+6.89%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@psobolewskiPhD
Copy link
Member Author

psobolewskiPhD commented Nov 11, 2022

Looks good @psobolewskiPhD !

Any chance we could add a test to catch this in the future?

Sample data is tested:
https://github.com/napari/napari/blob/7e6197212e2c1b1d507b6b5c14b5739691454b87/napari/plugins/_tests/test_sample_data.py
but CI installs pooch as part of testing install requirements:

napari/setup.cfg

Lines 110 to 123 in cd5c314

testing =
babel>=2.9.0
fsspec
hypothesis>=6.8.0
lxml
matplotlib
pooch>=1.6.0
pytest-cov
pytest-qt
pytest>=7.0.0
tensorstore>=0.1.13
torch>=1.7
xarray
zarr

So I'm not sure how to test this specific interaction.

(not sure why pooch is higher there than what scikit-image requires, maybe pooch is also used for some other test?)

@goanpeca
Copy link
Contributor

Got it! Thanks for digging it :)

@andy-sweet
Copy link
Member

(not sure why pooch is higher there than what scikit-image requires, maybe pooch is also used for some other test?)

I think it's because pooch added progress bars in v1.6.0: https://github.com/fatiando/pooch/releases/tag/v1.6.0

For the relativity small scikit-image data these aren't too important, unless you're on a slow connection. But as files get larger, having some messaging around progress becomes almost required. We added some examples like that in examples/dev to help drive forward the async slicing efforts.

@psobolewskiPhD psobolewskiPhD merged commit 6e489ee into napari:main Nov 23, 2022
@psobolewskiPhD psobolewskiPhD deleted the psobolewskiPhD-patch-1 branch November 23, 2022 19:52
@Czaki Czaki added this to the 0.5.0 milestone Nov 30, 2022
@psobolewskiPhD psobolewskiPhD added the bugfix PR with bugfix label Dec 11, 2022
@Czaki Czaki mentioned this pull request Jun 7, 2023
@Czaki Czaki modified the milestones: 0.5.0, 0.4.18 Jun 16, 2023
@Czaki Czaki added the triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process label Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR with bugfix installation Issues with installation triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process
Projects
None yet
4 participants