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

Update/fix CI setup. #437

Merged
merged 1 commit into from
Feb 19, 2023
Merged

Update/fix CI setup. #437

merged 1 commit into from
Feb 19, 2023

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 24, 2023

I went for the big omnibus PR...

  • Move list of dependencies into GHA yaml files; restrict to strict minimum for CI, not including black, flake8, twine, etc. (Migration to GitHub actions needed #393)
  • Test both with no optional deps and with all optional deps, except pylibtiff (because it seems tricky to install on CI -- it tries to look for the tifffile header and fails to find it). Note that the pylibtiff README explicitly states that tifffile is as efficient.
  • Pass -ra to pytest to indicate why tests are skipped.
  • Drop old, now-unused travis conf.
  • Fix for API changes re: imageio.v3 (closes Account for deprecation warnings in imageio #411), matplotlib layout engines (closes MNT: transition matplotlib code to set_layout_engine #429, also works around FIX: adjust_bbox should not modify layout engine matplotlib/matplotlib#24971), pyav format long_name, jpype threads and buffers (now exposed using the buffer protocol, not numpy arrays).
  • imageio now emits a warning if loading a tiff and tifffile is not separately installed (they deprecated their vendored copy of tifffile), let's just add tifffile as a dependency.
  • Pin bioformats jar to 6.7.0 (last working version); pre-download it before running tests to avoid download warnings causing the test suite to fail. (See loci_tools.jar issue #426; likely moving to bioformats_package.jar could still be a good idea?)
  • AFAICT the bioformats reader needs to explicitly construct a memoryview around the data returned by openBytes, otherwise the underlying buffer can become dangling (this leads to brittle, sometimes failing tests on CI).
  • Work around the upcoming deprecation of distutils by adding a dependency on packaging instead (technically it could be made optional as it is only needed for bioformats, but it is likely very widely installed, not too heavy, and always depending on it seems just simpler).
  • Mark all tests that pass plugin=... as requiring skimage (otherwise they emit a warning that plugin=... is ignored).
  • Explicitly close file at teardown of bioformats tests to avoid a ResourceWarning. Likewise, imageio-ffmpeg seems to have some reference cycle(?) which makes a gc.collect() sometimes necessary to avoid a ResourceWarning.

Unless there's strong reasons to do otherwise, I would also suggest dropping support for pylibtiff (and perhaps also just pick a side between pillow's tiff and tifffile); and likewise pick one movie reader (pyav seems favored by skimage https://scikit-image.org/docs/stable/user_guide/video.html#pyav, and has wheels so installation should be easy in practice).

@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2023

Codecov Report

Base: 61.64% // Head: 77.96% // Increases project coverage by +16.31% 🎉

Coverage data is based on head (f525608) compared to base (5ec734d).
Patch coverage: 81.63% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #437       +/-   ##
===========================================
+ Coverage   61.64%   77.96%   +16.31%     
===========================================
  Files          31       31               
  Lines        4391     4516      +125     
===========================================
+ Hits         2707     3521      +814     
+ Misses       1684      995      -689     
Impacted Files Coverage Δ
pims/pyav_reader.py 89.25% <ø> (+68.17%) ⬆️
pims/image_reader.py 87.93% <60.00%> (+6.29%) ⬆️
pims/bioformats.py 71.86% <69.23%> (+59.54%) ⬆️
pims/tests/test_display.py 99.33% <75.00%> (+36.67%) ⬆️
pims/display.py 64.41% <90.90%> (+31.17%) ⬆️
pims/tests/test_bioformats.py 73.29% <100.00%> (+34.04%) ⬆️
pims/tests/test_imseq.py 100.00% <100.00%> (ø)
pims/tests/test_multidimensional.py 99.28% <0.00%> (+0.01%) ⬆️
pims/tests/test_process.py 97.72% <0.00%> (+0.10%) ⬆️
pims/process.py 97.22% <0.00%> (+0.16%) ⬆️
... and 18 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

- Move list of dependencies into GHA yaml files; restrict to strict
  minimum for CI, not including black, flake8, twine, etc.
- Test both with no optional deps and with all optional deps, except
  pylibtiff (because it seems tricky to install on CI -- it tries to
  look for the tifffile header and fails to find it).  Note that the
  pylibtiff README explicitly states that tifffile is as efficient.
- Pass -ra to pytest to indicate why tests are skipped.
- Drop old, now-unused travis conf.
- Fix for API changes re: imageio.v3, matplotlib layout engines, pyav
  format long_name, jpype threads and buffers (now exposed using the
  buffer protocol, not numpy arrays).
- imageio now emits a warning if loading a tiff and tifffile is not
  separately installed (they deprecated their vendored copy of
  tifffile), let's just add tifffile as a dependency.
- Pin bioformats jar to 6.7.0 (last working version); pre-download it
  before running tests to avoid download warnings causing the test suite
  to fail.
- AFAICT the bioformats reader needs to explicitly construct a
  memoryview around the data returned by openBytes, otherwise the
  underlying buffer can become dangling (this leads to brittle,
  sometimes failing tests on CI).
- Work around the upcoming deprecation of distutils by adding a
  dependency on packaging instead (technically it could be made optional
  as it is only needed for bioformats, but it is likely very widely
  installed, not too heavy, and always depending on it seems just
  simpler).
- Mark all tests that pass plugin=... as requiring skimage (otherwise
  they emit a warning that plugin=... is ignored).
- Explicitly close file at teardown of bioformats tests to avoid a
  ResourceWarning. Likewise, imageio-ffmpeg seems to have some reference
  cycle(?) which makes a gc.collect() sometimes necessary to avoid a
  ResourceWarning.
@anntzer anntzer changed the title Update CI setup. Update/fix CI setup. Jan 24, 2023
@nkeim
Copy link
Contributor

nkeim commented Feb 19, 2023

Wow!!! What a gift for pims! Thanks @anntzer !!

I see no reason not to merge now, especially since this PR has had some time to season, and because we really could use working CI again.

I see that you are in support of some further changes, which I summarize as

  1. dropping pylibtiff
  2. dropping moviepy
  3. switching to bioformats_package.jar instead of loci_tools.jar

I see no problem with any of those given how much the package ecosystem has evolved since pims was first written, but I agree that those should be done in separate PRs since they would require some careful changes to the code, and of course there could be some users out there for whom these would be big problems. We already have #426 to discuss the bioformats change; should we open additional issues to discuss the first two?

@nkeim nkeim merged commit bb64ef4 into soft-matter:main Feb 19, 2023
@anntzer
Copy link
Contributor Author

anntzer commented Feb 19, 2023

I would also drop imageio-ffmpeg (per https://imageio.readthedocs.io/en/v2.25.1/_autosummary/imageio.plugins.ffmpeg.html#module-imageio.plugins.ffmpeg).

While I appreciate that this may be disruptive for some users, I simply notice that maintaining working CI for pims seems to have been challenging recently, and e.g. in the PR above I had to make various fixes for a number of readers, but I have no idea of whether the pylibtiff reader works as even installing that package properly is challenging (and I had limited interest in fixing that when the package README itself suggests that other tiff libraries may be better suited). Also note that my interest in getting CI fixed is actually to help the review of my other PRs (right now only #434; I could perhaps also give a try at fixing #436 now that CI is fixed).

You can open separate issues to keep track of the decision process but I don't really have much to add on top of that.

@anntzer anntzer deleted the ci branch February 19, 2023 21:31
@nkeim nkeim added the maintenance Caused by newer versions of dependencies label Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Caused by newer versions of dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MNT: transition matplotlib code to set_layout_engine Account for deprecation warnings in imageio
3 participants