imagej / pyimagej

Use ImageJ from Python
https://pyimagej.readthedocs.io/
Other
473 stars 82 forks source link

Converting a Java String to a Python String creates an empty File #160

Closed gselzer closed 2 years ago

gselzer commented 2 years ago

@ctrueden @elevans here is an MCVE:

import os
import imagej
from scyjava import config, jimport

config.add_option(f'-Dimagej.dir={os.getcwd()}')
ij = imagej.init()

String = jimport('java.lang.String')

s = String("foo")

# This is the problematic line
ij.py.from_java(s)

This seems to happen with SciJava Common versions 2.87.0 and 2.87.1

elevans commented 2 years ago

From the PyImageJ layer the bug is triggered when from_java checks what the incoming data type is with ij.convert().supports()

if not sj.isjava(data): return data
try:
    if self._ij.convert().supports(data, Dataset): # triggers bug
ctrueden commented 2 years ago

Thanks @elevans. I was able to reproduce this in pure Java using jgo from the command line:

$ jgo net.imagej:imagej:2.3.0:org.scijava.script.ScriptREPL
Welcome to the SciJava REPL!
...
language -> Groovy
> s = "asdf"
asdf
> new java.io.File(s).exists()
false
> ij.convert().supports(s, net.imagej.Dataset.class)
false
> new java.io.File(s).exists()
true
ctrueden commented 2 years ago

The bug-fix I was thinking of was scijava/scijava-common@c4cb3a71f6e5c02d78d27bcee728adb096f579ef. But this bug-fix was included in scijava-common 2.87.0:

$ git tag --contains c4cb3a71f6e5c02d78d27bcee728adb096f579ef
scijava-common-2.87.0
scijava-common-2.87.1
scijava-common-2.87.2

And looking at the patch, this doesn't actually address the problem I mentioned where files get created too eagerly. I thought we had done something to prevent this issue, but now I'm not sure.

ctrueden commented 2 years ago

Oh ho, the plot thickens:

$ jgo org.scijava:scijava-common:org.scijava.script.ScriptREPL+org.scijava:scripting-groovy+net.imagej:imagej-common
...
language -> Groovy
> sj.convert().supports("fdsa", net.imagej.Dataset.class)
false
> new java.io.File("fdsa").exists()
false

In other words: just having scijava-common and imagej-common on the classpath is not enough to trigger the bug. It must be some other converter outside these libraries, but which is part of ImageJ2. I will use an IDE to debug further.

ctrueden commented 2 years ago

The problem is caused by SCIFIO's FileToDatasetConverter and StringToDatasetConverter, which attempts to check, for a given Location (in this case a FileLocation), whether SCIFIO can open it. If SCIFIO can open it, then it can be converted to a Dataset, and those converters report true for canConvert/supports.

I verified that nothing in the framework itself is creating the file in question—rather, one of the SCIFIO format plugins is doing it, when this code is called.

Here is a more minimal example with jgo:

$ jgo org.scijava:scijava-common:2.87.1:org.scijava.script.ScriptREPL+org.scijava:scripting-groovy:0.4.1+io.scif:scifio:0.41.2
...
language -> Groovy
> scifio.datasetIO().canOpen("asdf")
false
> new java.io.File("asdf").exists()
true

Next steps:

ctrueden commented 2 years ago

The bug does not happen in SCIFIO 0.43.1. Looking over the changes between 0.41.2 and 0.43.1, I am guessing the commit which fixed the issue is scifio/scifio@6ace9fa10880995543025711a6518b8b25d30477. Thanks @hinerm!

So this issue will magically disappear once we get a new ImageJ2 release out the door. It's on my list.

imagejan commented 2 years ago

Related: https://github.com/scifio/scifio/issues/415, which is part of scifio-0.43.0 and later.

ctrueden commented 2 years ago

I am going to close this issue, since there is no more action required on the pyimagej side.