tlambert03 / bioformats_jar

(deprecated) meta-package to import bioformats_jar from scyjava
GNU General Public License v2.0
0 stars 0 forks source link

use scyjava, remove all the stuff #4

Closed tlambert03 closed 2 years ago

tlambert03 commented 2 years ago

Closes #2 ... uses scyjava to source the bioformats jar.

With this PR, bioformats_jar becomes a pretty much useless package, which is great! (It was my own naiveté that precipitated it in the first place, and both issues like #2 and https://github.com/napari/npe2/issues/136 have revealed the problems with directly shipping this jar and starting a jvm in a shared environment).

This PR and the next release aim to be invisible to downstream users (like aicsimageio), but will make it so that they will automatically get the newest bioformats version available (see https://github.com/AllenCellModeling/aicsimageio/issues/400) ... which can also be set via environment variable (BIOFORMATS_VERSION).

We can decide together whether this package serves any purpose after that, or if the next version should emit a warning to just use scyjava directly.

tlambert03 commented 2 years ago

cc @ctrueden

tlambert03 commented 2 years ago

@jacksonmaxfield, if this is merged and released, it would have some effects for aicsimageio 1) it would immediately start using the latest version of bioformats (adjustable with the BIOFORMATS_VERSION env var) 2) if we left it at LATEST, then the DICOM test would fail in aicsimageio, since it appears they changed their series index from 0 to PRIMARY or something like that 3) users will also need to have maven installed in their path, since scyjava/jgo depends on it... so we need to update the docs and exceptions. 4) we could actually start offering the BSD-licensed stuff by default using the BIOFORMATS_LICENSE env var. ... more

evamaxfield commented 2 years ago

if we left it at LATEST, then the DICOM test would fail in aicsimageio, since it appears they changed their series index from 0 to PRIMARY or something like that

more reason to move to plugins :woozy_face:

users will also need to have maven installed in their path, since scyjava/jgo depends on it... so we need to update the docs and exceptions.

I think this is fine just needs to be documented.

we could actually start offering the BSD-licensed stuff by default using the BIOFORMATS_LICENSE env var. ... more

FASCINATING... still sad that aicspylibczi and readlif need to be GPL but :shrug: we take want we can.

All in all, I say good changes and whenever this is merged + released as new bioformats_jar version I will bump the version up on aicsimageio and release a minor release. Its not a fully breaking change for us just on you but come on... that doesn't warrent a major change on us does it?

evamaxfield commented 2 years ago

cc @toloudis

evamaxfield commented 2 years ago

I can't push to this branch but can I request a minor change: bump jpype from ~=1.2 to ~=1.4. They just released windows + py310 bugfixes today

tlambert03 commented 2 years ago

sure, actually... any reason not to unpin altogether and just let scyjava determine it? https://github.com/scijava/scyjava/blob/26ad79bc023f0435db821458c270c7d562f445eb/setup.cfg#L42

currently they say ≥1.3

evamaxfield commented 2 years ago

sure, actually... any reason not to unpin altogether and just let scyjava determine it? https://github.com/scijava/scyjava/blob/26ad79bc023f0435db821458c270c7d562f445eb/setup.cfg#L42

currently they say ≥1.3

I am fine with it. I will likely make a PR over to them and bump it.

ctrueden commented 2 years ago

I will likely make a PR over to them and bump it.

@JacksonMaxfield I was going to update it now, so that you don't have to... but then I thought: what is the rationale for actually requiring 1.4.0? JPype 1.3.0 works for all other scenarios, just not Windows + Python 3.10, right? Forcing it to 1.4.0+ seems a bit restrictive. Unless there is new API we need to take advantage of?

evamaxfield commented 2 years ago

JPype 1.3.0 works for all other scenarios, just not Windows + Python 3.10

Yea I think that is really it. So :woman_shrugging: your call. I am fine if you want to leave it as is.

ctrueden commented 2 years ago

Hmm, well, the PyImageJ project has a special note in its Troubleshooting section about the Windows + Python 3.10 problem... so I guess forcing >=1.4.0 will let us update PyImageJ too and remove that note, which is a plus. So upon reflection I'm fine with it. We just need to test all the platforms of course.

ctrueden commented 2 years ago

scijava/scyjava@61bc020b694a376460c29f6923e979f73c61a2d1

ctrueden commented 2 years ago

JPype 1.4.0 has not yet propagated to conda-forge, though; we'll need to wait till that happens before we can cut a new scyjava release.