jep icon indicating copy to clipboard operation
jep copied to clipboard

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

Open jonathanindig opened this issue 2 years ago • 6 comments

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

jonathanindig avatar May 31 '22 21:05 jonathanindig

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 avatar Jun 01 '22 01:06 bsteffensmeier

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.

bsteffensmeier avatar Jun 01 '22 05:06 bsteffensmeier

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

jonathanindig avatar Jun 01 '22 13:06 jonathanindig

@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
"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?

jonathanindig avatar Jun 01 '22 15:06 jonathanindig

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.

bsteffensmeier avatar Jun 02 '22 03:06 bsteffensmeier

@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!

jeremyrsmith avatar Jun 02 '22 16:06 jeremyrsmith

This was fixed in jep 4.1

bsteffensmeier avatar Apr 14 '23 23:04 bsteffensmeier