ninia / jep

Embed Python in Java
Other
1.3k stars 147 forks source link

Method Resolution Order error when working with Scala types in Python with Jep 4.0.0+ #406

Closed jonathanindig closed 1 year ago

jonathanindig commented 2 years ago

We're running into an exception with Jep 4.0.0+ in Polynote when certain Scala types are made available to Jep. For example, here's what we get with a Scala List type:

jep.JepException: <class 'TypeError'>: Cannot create a consistent method resolution
order (MRO) for bases HasNewBuilder, GenTraversableLike

I have a hunch that this may be related to https://github.com/ninia/jep/commit/151c748f6c3674bc1ef292156fa82224a208fa3c, because I see that there's some Java class inheritance inspection stuff going on.

The same code works with Jep 3.9.1, so I know it's something new in 4.0.0. I tried with Python 3.7 and 3.9.

Hopefully this is enough to go by 😬 If not, I can try to come up with a minimal Scala repro.

FYI @jeremyrsmith

bsteffensmeier commented 2 years ago

That is bad. You have accurately identified the change that causes the problem. The goal of that change was to create a Python type hierarchy that mirrors the Java class hierarchy. The change was mostly implemented as a way to fix #307 by allowing a PyJObject to inherit special python functionality from multiple Java interfaces. Conceptually there are other features I was hoping to build off the mirrored type hierarchy.

I am not very familiar with how scala classes work and specifically how they are translated into Java classes but I have managed to get the same error using only Java interfaces, as shown below. I suspect if I can resolve this use case then the Scala use case will also be resolved.

class Test {

    public static interface Root {
    }

    public static interface Child extends Root{
    }

    public static class ProblemClass implements Root, Child {
    }

    public static void main(String[] args) throws JepException {
        try (SharedInterpreter interpreter = new SharedInterpreter()) {
            interpreter.set("test", new ProblemClass());
        }
    }
}

The problem is that I assumed that any type hierarchy in Java would also be a valid Python type hierarchy. I can easily demonstrate that this is not the case just by reproducing the problematic Java hierarchy in pure Python, shown below. Whether the type hierarchy is defined in Java or Python it violates the default Python Method Resolution order(MRO) and cannot be represented in Python. Fortunately it is possible to define a custom MRO by defining a metaclass. I will need to do more research but I think it would be acceptable to use a custom metaclass for all the pyjtypes which would use a simplified MRO. I do not think the actual MRO used by Python is important to the functionality of pyjtype since each type currently includes all methods defined by supertypes(which is necessary to handle overloaded methods) and JNI is actually handling the method resolution.

>>> class Root: pass
>>> class Child(Root): pass
>>> class ProblemClass(Root,Child): pass
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Cannot create a consistent method resolution
order (MRO) for bases Root, Child
bsteffensmeier commented 2 years ago

I've made an initial attempt at a custom MRO that works for my small test case and submitted it as #407, The scala class hierarchy is much more interesting than my small example so it would be fantastic if you can test this change with scala to see if I have correctly identified the problem.

jonathanindig commented 2 years ago

Amazing @bsteffensmeier , thanks so much for your quick response! I will try to test this out today.

jonathanindig commented 2 years ago

@bsteffensmeier I set up a test in Scala with some crazy type hierarchies (including testing List, which has a pretty complex hierarchy).

Scala test code ```scala "Jep" in { val jep = new SharedInterpreter trait Root trait Child extends Root trait ProblemClass extends Root with Child jep.set("problem", new ProblemClass {}) // Seems to work in Scala, even though the equivalent Java doesn't? // Let's try to make it fail trait A trait B trait C extends A with B trait D extends B with A // <- causes failure, as the order is reversed object E extends C with D a[JepException] shouldBe thrownBy { jep.set("e", E) // fails! } a[JepException] shouldBe thrownBy { jep.set("a_list", List(1)) // Fails due to crazy Scala collection type hierarchy } // Let's recreate that crazy collection hierarchy! trait _HasNewBuilder trait _GenTraversableOnce trait _Parallelizable trait _Function1 trait _Equals trait _Serializable trait _FilterMonadic trait _Product extends _Equals trait _PartialFunction extends _Function1 trait _GenTraversableLike extends _GenTraversableOnce with _Parallelizable trait _GenIterableLike extends _GenTraversableLike trait _GenericTraversableTemplate extends _HasNewBuilder trait _GenTraversable extends _GenTraversableLike with _GenTraversableOnce with _GenericTraversableTemplate trait _GenIterable extends _GenIterableLike with _GenTraversable with _GenericTraversableTemplate trait _GenSeqLike extends _GenIterableLike with _Equals with _Parallelizable trait _GenSeq extends _GenSeqLike with _GenIterable with _Equals with _GenericTraversableTemplate trait _TraversableOnce extends _GenTraversableOnce trait _TraversableLike extends _HasNewBuilder with _FilterMonadic with _TraversableOnce with _GenTraversableLike with _Parallelizable trait _IterableLike extends _Equals with _TraversableLike with _GenIterableLike trait _SeqLike extends _IterableLike with _GenSeqLike with _Parallelizable trait _LinearSeqLike extends _SeqLike trait _LinearSeqOptimized extends _LinearSeqLike trait _Traversable extends _TraversableLike with _GenTraversable with _TraversableOnce with _GenericTraversableTemplate trait _Iterable extends _Traversable with _GenIterable with _GenericTraversableTemplate with _IterableLike trait _Seq extends _PartialFunction with _Iterable with _GenSeq with _GenericTraversableTemplate with _SeqLike trait _AbstractTraversable extends _Traversable trait _AbstractIterable extends _AbstractTraversable with _Iterable trait _AbstractSeq extends _AbstractIterable with _Seq trait _scala_collection_LinearSeq extends _Seq with _GenericTraversableTemplate with _LinearSeqLike trait _LinearSeq extends _Seq with _scala_collection_LinearSeq with _GenericTraversableTemplate with _LinearSeqLike trait _List extends _AbstractSeq with _LinearSeq with _Product with _GenericTraversableTemplate with _LinearSeqOptimized with _Serializable a[JepException] shouldBe thrownBy { jep.set("_list", new _List {}) // fails! } } ```

This test throws exceptions with Jep 4.0.3 (Note the a[JepException] shouldBe thrownBy assertions).

It looks like your changes in #407 fix the problem though, as I no longer get exceptions after installing your branch with pip3 install git+https://github.com/bsteffensmeier/jep.git@pyjtype_mro!

@jeremyrsmith can you think of any other weird inheritance hierarchies that Scala supports that aren't covered by the test I wrote?

bsteffensmeier commented 2 years ago

Thanks for taking the time to test it, I am glad I solved the right problem instead of finding a new one.

I am not sure if there is a better MRO algorithm to use for Java classes than the one I proposed. Since Java mostly ignores interfaces I don't think the MRO really matters as long as the non-interface classes are in order but I'd like to give it more thought before merging, If anyone else has thoughts I am open to ideas, especially if there any other cases where scala might show problems more easily than java alone.

jeremyrsmith commented 2 years ago

@jonathanindig the only weird Scala-specific thing I can think of is abstract override. But, in a concrete class, that will end up being encoded in ways that are pretty much tested by your code.

IIRC, in Scala 2.13 (and maybe 2.12?), there is some further Java-8-ification of certain trait methods as interface default methods. So it might be worth adding some default method cases to the test in #407 @bsteffensmeier (I'll comment there).

Thanks for looking into this so quickly!

bsteffensmeier commented 1 year ago

This was fixed in jep 4.1