admin/prepare-bioformats_reader-to-work-with-new-bioformats_jar-based-on-scyjava #402

tlambert03 · 2022-05-23T14:57:47Z

Description

This PR prepares the bioformats reader to work with the scyjava-based bioformats_jar update coming in tlambert03/bioformats_jar#4. Mostly this just means updating the error messages and readme a bit.

With this PR, aicsimageio should work both with the current version of bioformats_jar and the new one after that is released

Pull request recommendations:

Name your pull request your-development-type/short-description. Ex: feature/read-tiff-files
Link to any relevant issue in the PR description. Ex: Resolves [gh-], adds tiff file format support
Provide relevant tests for your feature or bug fix.
Provide or update documentation for any feature added by your pull request.

Thanks for contributing!

tlambert03 · 2022-05-23T15:00:37Z

@JacksonMaxfield, are these Credentials could not be loaded errors something I need to fix?

evamaxfield · 2022-05-23T15:24:20Z

Uhhhhh yea I will investigate the credentials. Wonder if someone on DevOps rolled the creds without telling us

AetherUnbound

I can't comment much on the creds issue, but here's a few thoughts!

README.md

AetherUnbound · 2022-05-23T18:16:21Z

aicsimageio/readers/bioformats_reader.py

-        "Install with `pip install bioformats_jar`"
-    )
+        "Install with `pip install bioformats_jar` or `conda install bioformats_jar`"
+    ) from e


💯 love this!

AetherUnbound · 2022-05-23T18:17:56Z

aicsimageio/readers/bioformats_reader.py

+        raise RuntimeError(MAVEN_ERROR_MSG) from e
+    except jpype.JVMNotFoundException as e:
+        raise RuntimeError(JAVA_ERROR_MSG) from e


These verbose error messages are fantastic!

AetherUnbound · 2022-05-23T18:18:44Z

setup.cfg

@@ -29,6 +29,7 @@ xfail_strict = true
 filterwarnings = 
 	ignore::UserWarning
 	ignore::FutureWarning
+	ignore:distutils Version classes are deprecated:


What code is bringing about this warning?

both xarray and setuptools:

================================================================= warnings summary ================================================================= ../../../../miniconda3/envs/aics/lib/python3.9/site-packages/xarray/core/pycompat.py:22 aicsimageio/tests/readers/extra_readers/test_bioformats_reader.py::test_bioformats_reader[DICOM_samples_MR-MONO2-8-16x-heart.dcm-PRIMARY-expected_scenes16-expected_shape16-uint8-TCZYX-expected_channel_names16-expected_physical_pixel_sizes16-LOCAL] aicsimageio/tests/readers/extra_readers/test_bioformats_reader.py::test_bioformats_reader[DICOM_samples_MR-MONO2-8-16x-heart.dcm-PRIMARY-expected_scenes16-expected_shape16-uint8-TCZYX-expected_channel_names16-expected_physical_pixel_sizes16-LOCAL] aicsimageio/tests/readers/extra_readers/test_bioformats_reader.py::test_bioformats_reader[DICOM_samples_MR-MONO2-8-16x-heart.dcm-PRIMARY-expected_scenes16-expected_shape16-uint8-TCZYX-expected_channel_names16-expected_physical_pixel_sizes16-LOCAL] /Users/talley/miniconda3/envs/aics/lib/python3.9/site-packages/xarray/core/pycompat.py:22: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead. duck_array_version = LooseVersion(duck_array_module.__version__) ../../../../miniconda3/envs/aics/lib/python3.9/site-packages/xarray/core/pycompat.py:37 ../../../../miniconda3/envs/aics/lib/python3.9/site-packages/xarray/core/pycompat.py:37 /Users/talley/miniconda3/envs/aics/lib/python3.9/site-packages/xarray/core/pycompat.py:37: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead. duck_array_version = LooseVersion("0.0.0") ../../../../miniconda3/envs/aics/lib/python3.9/site-packages/setuptools/_distutils/version.py:351 ../../../../miniconda3/envs/aics/lib/python3.9/site-packages/setuptools/_distutils/version.py:351 ../../../../miniconda3/envs/aics/lib/python3.9/site-packages/setuptools/_distutils/version.py:351 ../../../../miniconda3/envs/aics/lib/python3.9/site-packages/setuptools/_distutils/version.py:351 /Users/talley/miniconda3/envs/aics/lib/python3.9/site-packages/setuptools/_distutils/version.py:351: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead. other = LooseVersion(other) ../../../../miniconda3/envs/aics/lib/python3.9/site-packages/xarray/core/npcompat.py:82 /Users/talley/miniconda3/envs/aics/lib/python3.9/site-packages/xarray/core/npcompat.py:82: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead. if LooseVersion(np.__version__) >= "1.20.0": ../../../../miniconda3/envs/aics/lib/python3.9/site-packages/xarray/core/pdcompat.py:45 /Users/talley/miniconda3/envs/aics/lib/python3.9/site-packages/xarray/core/pdcompat.py:45: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead. if LooseVersion(pd.__version__) < "0.25.0": -- Docs: https://docs.pytest.org/en/stable/warnings.html

Ahh gotcha. I've been getting a lot of those LooseVersion warnings on other projects. Good to know I can borrow this ignoration 😁

evamaxfield · 2022-05-23T19:41:58Z

i have asked AICS devops team to look into what is going on with credentials 🤷‍♀️

evamaxfield · 2022-05-23T22:28:50Z

No idea if it will work but @tlambert03 could you try adding this to the test-and-lint.yml?

aws-actions/configure-aws-credentials#271 (comment)

evamaxfield · 2022-05-24T17:48:42Z

Hey @tlambert03 could you switch from install the branch of bioformats_jar with git + ssh to git + https? I don't think this is the reason for all the other build failures because we have fail-fast turned off but 🤷‍♀️ who knows

tlambert03 · 2022-05-24T19:29:41Z

whoops... fixed. But yeah, it was failing before even when using the one from pypi

tlambert03 · 2022-05-24T19:30:43Z

updated test-and-lint

tlambert03 · 2022-05-24T23:23:06Z

Still no :/

evamaxfield · 2022-05-24T23:30:29Z

I am trying to change the permissions and see what happens -- I just don't understand what is happening. No logs for the issue is really weird

evamaxfield · 2022-05-25T15:05:03Z

how timely that github adds a feature to enable debug logging on failed jobs. Rerunning and seeing if there is any new info...

my prs are running / build is completing on main which means AWS creds are still valid
your prs are failing but at AWS creds step seems to indicate secrets access / permissions
nothing has changed at least on the repo side of things...

evamaxfield · 2022-05-25T22:25:31Z

~~@tlambert03 sorry to bother for time again, can you try removing the aws setup credentials task for the tests? I dont know if we even need creds to read the data....~~

Scratch that, I will try it.

tlambert03 · 2022-05-25T23:03:09Z

here if you need me to try something!

evamaxfield · 2022-05-26T16:45:42Z

@tlambert03 the credentials bugfix / feature / admin PR has been merged. Can you merge main?

tlambert03 · 2022-05-26T17:18:53Z

maybe encouraging? Note: don't merge this until I change back the bit in setup.py (just want to temporarily test the new bioformats_jar main before releasing it to pypi

codecov-commenter · 2022-05-26T17:21:38Z

Codecov Report

Merging #402 (4416594) into main (dca7a41) will decrease coverage by 0.08%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main     #402      +/-   ##
==========================================
- Coverage   94.65%   94.56%   -0.09%     
==========================================
  Files          45       45              
  Lines        3741     3756      +15     
==========================================
+ Hits         3541     3552      +11     
- Misses        200      204       +4

Impacted Files	Coverage Δ
...ts/readers/extra_readers/test_bioformats_reader.py	`96.72% <66.66%> (-3.28%)`	⬇️
aicsimageio/readers/bioformats_reader.py	`85.30% <72.72%> (-0.29%)`	⬇️

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 dca7a41...4416594. Read the comment docs.

evamaxfield · 2022-05-26T17:27:42Z

@tlambert03 i unfortunately think you will see failing bioformats tests because you haven't added the pip install from git to the tox config: https://github.com/AllenCellModeling/aicsimageio/blob/main/tox.ini#L11

-- or rather, they will just be using the old version (current release) of bioformats

Sorry for the doubly added dependencies... if you know of a better way to manage such a giant test suite I am all ears and can get to it next week.

tlambert03 · 2022-05-26T17:34:59Z

hah. ok, well, they're passing with the current release of bioformats_jar. so that's good. Lemme test with the new one.

if you know of a better way to manage such a giant test suite I am all ears and can get to it next week.

well, I would have said just declare the additional extras (rather than setting them as deps)... But because we don't actually provide an extra .... 😒 this is fine :)

evamaxfield · 2022-05-26T17:36:11Z

well, I would have said just declare the additional extras (rather than setting them as deps)... But because we don't actually provide an extra ....

If only....

tlambert03 · 2022-05-26T19:38:07Z

ok! caught one more (simple) issue with the new bioformats_jar and our tests here. It's now fixed, and the tox.ini change is reverted. Assuming stuff passes here, this is good to go. Only after it is merged and released will I cut the new bioformats_jar release

evamaxfield

All looks good to me!

evamaxfield · 2022-05-26T19:43:02Z

Once tests finish up I will merge and release. Thanks @tlambert03

evamaxfield · 2022-05-26T20:33:57Z

Disregard the failing checks -- they are all codecov upload errors. I don't think codecov is built to handle a test matrix like this.

tlambert03 · 2022-05-26T21:24:24Z

btw, I'm seeing a lot of macos codecov errors too. there's an open issue: codecov/codecov-action#745

evamaxfield · 2022-05-27T00:33:56Z

Released 4.8.0, feel free to release a new bioformats_jar whenever

tlambert03 added 3 commits May 23, 2022 10:50

make tests work with either version

e48512e

Merge branch 'main' into update-bioformats

9e346b3

update readme

bc142a1

tlambert03 changed the title ~~Prepare bioformats_reader to work with new bioformats_jar based on scyjava~~ admin/prepare-bioformats_reader-to-work-with-new-bioformats_jar-based-on-scyjava May 23, 2022

tlambert03 added 2 commits May 23, 2022 11:30

remove cache

d685610

test from github

a6e6827

AetherUnbound reviewed May 23, 2022

View reviewed changes

remove newline

6639ffc

use https

690c4df

update test and lint

e56961b

evamaxfield mentioned this pull request May 25, 2022

feature-and-admin/create-aicsimage-objects-with-fs-kwargs-and-remove-need-for-creds #407

Merged

4 tasks

tlambert03 added 2 commits May 26, 2022 13:05

Merge branch 'main' into update-bioformats

e110a31

remove test-and-lint change

30eb2de

test new bioformats_jar in tox

8d9ca5c

tlambert03 added 2 commits May 26, 2022 15:35

fix remaining dicom test

392f90a

revert change to tox

4416594

evamaxfield approved these changes May 26, 2022

View reviewed changes

evamaxfield added documentation Improvements or additions to documentation enhancement New feature or request admin Various administrative tasks for the package labels May 26, 2022

toloudis approved these changes May 26, 2022

View reviewed changes

evamaxfield merged commit 5df9060 into AllenCellModeling:main May 26, 2022

tlambert03 deleted the update-bioformats branch May 26, 2022 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

admin/prepare-bioformats_reader-to-work-with-new-bioformats_jar-based-on-scyjava #402

admin/prepare-bioformats_reader-to-work-with-new-bioformats_jar-based-on-scyjava #402

tlambert03 commented May 23, 2022 •

edited

Loading

tlambert03 commented May 23, 2022

evamaxfield commented May 23, 2022

AetherUnbound left a comment

AetherUnbound May 23, 2022

AetherUnbound May 23, 2022

AetherUnbound May 23, 2022

tlambert03 May 23, 2022

AetherUnbound May 27, 2022

evamaxfield commented May 23, 2022

evamaxfield commented May 23, 2022

evamaxfield commented May 24, 2022

tlambert03 commented May 24, 2022

tlambert03 commented May 24, 2022

tlambert03 commented May 24, 2022

evamaxfield commented May 24, 2022

evamaxfield commented May 25, 2022

evamaxfield commented May 25, 2022 •

edited

Loading

tlambert03 commented May 25, 2022

evamaxfield commented May 26, 2022

tlambert03 commented May 26, 2022

codecov-commenter commented May 26, 2022 •

edited

Loading

evamaxfield commented May 26, 2022 •

edited

Loading

tlambert03 commented May 26, 2022

evamaxfield commented May 26, 2022

tlambert03 commented May 26, 2022

evamaxfield left a comment

evamaxfield commented May 26, 2022

evamaxfield commented May 26, 2022

tlambert03 commented May 26, 2022

evamaxfield commented May 27, 2022

admin/prepare-bioformats_reader-to-work-with-new-bioformats_jar-based-on-scyjava #402

admin/prepare-bioformats_reader-to-work-with-new-bioformats_jar-based-on-scyjava #402

Conversation

tlambert03 commented May 23, 2022 • edited Loading

Description

Pull request recommendations:

tlambert03 commented May 23, 2022

evamaxfield commented May 23, 2022

AetherUnbound left a comment

Choose a reason for hiding this comment

AetherUnbound May 23, 2022

Choose a reason for hiding this comment

AetherUnbound May 23, 2022

Choose a reason for hiding this comment

AetherUnbound May 23, 2022

Choose a reason for hiding this comment

tlambert03 May 23, 2022

Choose a reason for hiding this comment

AetherUnbound May 27, 2022

Choose a reason for hiding this comment

evamaxfield commented May 23, 2022

evamaxfield commented May 23, 2022

evamaxfield commented May 24, 2022

tlambert03 commented May 24, 2022

tlambert03 commented May 24, 2022

tlambert03 commented May 24, 2022

evamaxfield commented May 24, 2022

evamaxfield commented May 25, 2022

evamaxfield commented May 25, 2022 • edited Loading

tlambert03 commented May 25, 2022

evamaxfield commented May 26, 2022

tlambert03 commented May 26, 2022

codecov-commenter commented May 26, 2022 • edited Loading

Codecov Report

evamaxfield commented May 26, 2022 • edited Loading

tlambert03 commented May 26, 2022

evamaxfield commented May 26, 2022

tlambert03 commented May 26, 2022

evamaxfield left a comment

Choose a reason for hiding this comment

evamaxfield commented May 26, 2022

evamaxfield commented May 26, 2022

tlambert03 commented May 26, 2022

evamaxfield commented May 27, 2022

tlambert03 commented May 23, 2022 •

edited

Loading

evamaxfield commented May 25, 2022 •

edited

Loading

codecov-commenter commented May 26, 2022 •

edited

Loading

evamaxfield commented May 26, 2022 •

edited

Loading