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

Remove workaround when ConvertService can operate on python types #220

Closed
hinerm opened this issue May 12, 2023 · 4 comments · Fixed by #270
Closed

Remove workaround when ConvertService can operate on python types #220

hinerm opened this issue May 12, 2023 · 4 comments · Fixed by #270
Milestone

Comments

@hinerm
Copy link
Member

hinerm commented May 12, 2023

This commit adds a workaround to a JPype limitation. Without it, when trying to convert python types (even if a valid converter implementation exists) we get this error:

TypeError: No matching overloads found for org.scijava.convert.ConvertService.supports(tuple,_jpype._JClass), options are:
        public default boolean org.scijava.convert.ConvertService.supports(java.lang.Class,java.lang.Class)
        public default boolean org.scijava.convert.ConvertService.supports(java.lang.Object,java.lang.Class)
        public default boolean org.scijava.plugin.HandlerService.supports(java.lang.Object)
        public default boolean org.scijava.convert.ConvertService.supports(java.lang.Class,java.lang.reflect.Type)
        public default boolean org.scijava.convert.ConvertService.supports(java.lang.Object,java.lang.reflect.Type)

When this is fixed in Jpype we can remove the workaround

@hinerm hinerm added this to the unscheduled milestone May 12, 2023
@ctrueden
Copy link
Member

ctrueden commented May 12, 2023

There are valid Converter plugin implementations for python types like tuple? Where? And what Java type does a tuple present as on the Java side? This magic feels dangerous to me in general... I feel like the commit making the line lambda obj: isjava(obj) and ij().convert().supports(obj, jc.DatasetView) is actually correct... or else it should be conceptually lambda obj: ij().convert().supports(ij.py.to_java(obj), jc.DatasetView) although that way would end up doing lots of pointless Python-to-Java conversions in the background.

@Thrameos
Copy link

JPype likely won't ever support conversion of a tuple because it has no Java type unless there is some generic container type what it could be mapped to. Can I ask what code triggered this particular error? (what is in the tuple and what should it convert to?)

@gselzer
Copy link
Collaborator

gselzer commented May 16, 2023

@ctrueden @hinerm the commit making that line isn't wrong per se, it just stinks that python objects can reach that code. scyjava.to_python doesn't ensure that the argument is a java object, so calling scyjava.to_python(my_java_object) results in us having to defensively program within the predicates.

It's worth noting that this happens because in pyimagej there's a redundant conversion in java_to_xarray (which I fixed in imagej/pyimagej#257), that just must have not released yet. But that doesn't fix things in the general case; we'd need a fix to scyjava for that.

@Thrameos it's definitely not an issue with JPype, and I think your reasoning about not supporting tuple conversion is correct. The tuple contains napari Layer (i.e. Python) objects, and it should not convert to anything because the method we're trying to call is meant to convert java objects into another type of java object.

@gselzer
Copy link
Collaborator

gselzer commented May 26, 2023

Note that scijava/scyjava#60 should enable us, when we can depende on it, to remove these checks

ctrueden added a commit that referenced this issue Jul 25, 2023
And bump minimum scyjava version to 1.9.1.

As of scyjava 1.9.1, the work from scijava/scyjava#60 is integrating,
meaning such defensive checks around conversion are no longer necessary.

This reverts commit 85f0d5c,
titled "Work around jpype convert limitation", and closes #220.

It also removes some other usages of isjava, which are also not needed.
ctrueden added a commit that referenced this issue Jul 25, 2023
And bump minimum scyjava version to 1.9.1.

As of scyjava 1.9.1, the work from scijava/scyjava#60 is integrated,
meaning such defensive checks around conversion are no longer necessary.

This reverts commit 85f0d5c,
titled "Work around jpype convert limitation", and closes #220.

It also removes some other usages of isjava, which are also not needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants