-
Notifications
You must be signed in to change notification settings - Fork 245
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
Illegal reflective access by kryo #3659
Comments
Hi @juliangilbey, the version of |
Hi @sbesson, yes indeed - we plan to always update to the latest Bio-Formats with each release. We hope 0.3.x will be available late March. It should be possible to drag Ideally it would be possible for users to update Bio-Formats independently of QuPath in an easier way. I remain a bit torn in the best to make Bio-Formats available through QuPath - improvements to our current approach would be very welcome. See also (Also @melvingelbard ) |
Hi @juliangilbey, please do let us know if updating bioformats brings stability or if there are issues. Best, |
(Ah, correction, kryo is now at 5.0.3. My bad.) @petebankhead @mahdilamb We'll try this suggestion, thanks. Though now I'm confused (and this is a QuPath question, not a BioFormats one): QuPath built quite happily, including And where would I put bioformats_package.jar (on Linux)? Would it go into Thanks! |
@juliangilbey QuPath is using the 'other' way to include Bio-Formats as a library, so it is split across multiple jars (including |
Hi @petebankhead Super thanks! Could I just change the |
@juliangilbey Yup, that should work. (From memory, dragging the jar onto QuPath to add it as an extension may well work too - I think it does pick up the newer version, but possibly more by luck than design... you can see the version under Help → Installed extensions). |
Hi @petebankhead and @mahdilamb. So I rebuilt QuPath with |
Hello again, this resurfaces more problematically in Java 16, thanks to JEP 396: Strongly Encapsulate JDK Internals by Default. When opening an .ndpi file with memoization enabled, I see an error:
Since QuPath v0.3 will use Java 16, I've been exploring workarounds. One option that seems to work is to pass I'm not sure where Since it seems to be a cached value that will be replaced if |
Fix missing Java options in console build for Windows (which meant OpenSlide couldn't be loaded). Avoid memoization errors with --add-opens, see ome/bioformats#3659 Note that reflection may cause more problems with kryo/gson/groovy (possibly).
Hi @petebankhead thanks for the feedback. A few comments from our side:
|
Thanks @sbesson, I spent some hours exploring this in Bio-Formats. I can confirm that setting the import java.io.IOException;
import loci.formats.FormatException;
import loci.formats.ImageReader;
import loci.formats.Memoizer;
import loci.formats.MetadataTools;
import loci.formats.meta.IMetadata;
/**
* Simple class to confirm whether memoization causes errors in Java 16+.
*/
public class CheckForMemoizationErrors {
public static void main(String[] args) {
if ( args == null || args.length< 1 ){
System.err.println("Name required");
System.exit(1);
}
for (String path : args) {
try (Memoizer memoizer = new Memoizer(new ImageReader())) {
System.out.println("Reading: " + path);
IMetadata metadata = MetadataTools.createOMEXMLMetadata();
memoizer.setMetadataStore(metadata);
memoizer.setId(path);
System.out.println("Image count: " + metadata.getImageCount());
} catch (IOException | FormatException e) {
e.printStackTrace();
}
}
}
} This works with a warning on Java 11, and fails with an error in Java 16. However, I couldn't replicate it with a very easy unit test added to the existing
Nevertheless, without me making any changes whatsoever I also saw that
fails with Java 16. The messages are
The cause of the problem seems to be the same. The easy workaround in that case is to make I can submit a pull request to set Regarding QuPath quickly adopting new Java versions, this is because Java 11 doesn't have jpackage - and we need that for creating our installers. We've resisted using other new features so our code should be compatible with Java 11. We skipped Java 15 because too much had changed, but we are aiming for Java 16 so we're ready for 17 when it comes later this year... and hopefully we can stick with LTS releases afterwards. |
This issue has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/stardist-qupath-3-0-snaphot-detection-for-bigger-annotations/51978/2 |
just chiming in to say that I see this as well using bioformats 6.7.0, in python via jpype.
import jpype
jpype.startJVM(classpath='path/to/bioformats_package.jar')
loci = jpype.JPackage("loci")
loci.common.DebugTools.setRootLevel("ERROR")
r = loci.formats.Memoizer(loci.formats.ImageReader())
r.setId('whatever.tif')
|
I am experiencing this again in Java 17 (LTS) - except I haven't been able to find a way past it because the previous workaround of Using
Without solving this, I don't think it's possible to use memoization on Java 17. |
@petebankhead I just opened #3815 which includes a minimal change to the inner The change should be tested in our CI tomorrow also including #3796 which fixes the other known Java 17 memoization issue. Would be interested to hear if that fixes your use case as well. |
Thanks @sbesson that sounds good. Would it be possible to also incorporate the change making Without that, there are still Java 17 problems unless the application is launched with |
By incorporating, do you mean merging #3815 and #3796 into a single PR to simplify the review? |
Sorry, ignore me, I read it too quickly and missed that both are covered! |
I'm not sure where I should report this issue, so please feel free to tell me if it should go somewhere else.
I'm running QuPath, and am opening lots of images using the BioFormats library to create thumbnail images. On one image, I get the following warning:
The only use of the kryo library in QuPath is as a dependency of BioFormats. I had a look at the kryo GitHub page, and it's currently on version 6.0.1 or so; version 2.4.0 was released in 2014. So if it's a bug in kryo, it may well have been fixed by now.
I am happy to share my code, in case that helps. Unfortunately, though, this issue also seems to be transient: I repeated the extraction and did not get any warnings the second time around. But my colleague, using the same code, is also sporadically getting this same warning.
The text was updated successfully, but these errors were encountered: