mbeddr.core icon indicating copy to clipboard operation
mbeddr.core copied to clipboard

CombinedInterpreter sorting is not stable

Open abstraktor opened this issue 4 years ago • 4 comments

Issue 1: CombinedInterpreter sorting is not stable

Given I have an interpreter Int1A in category category1 And I have an interpreter Int1B in category category1 And I have an interpreter Int2 in category category2 And all of them have an evaluator for concept MyConcept

When run new CombinedInterpreter(InterpreterEvaluationHelper.getInterpreter("category1"), InterpreterEvaluationHelper.getInterpreter("category2")).listEvaluators() Then I sometimes get [Int1A.MyConcept, Int1B.MyConcept, Int2.MyConcept] And, surprisingly, I sometimes get [Int2.MyConcept, Int1A.MyConcept, Int1B.MyConcept]

Issue 2: Nesting CombinedInterpreters breaks declared relationships

Given I have all the interpreters from Issue 1 And I have a relationship in Int1A expressed to make it run before Int2 When I run the same as in Issue 1 Then I get the same unstable results as in Issue 1

Analysis

  • InterpreterEvaluationHelper.getInterpreter returns a CombinedInterpreter
  • as a result, the above-written is comparing two CombinedInterpreters, which don't have any relationship
  • it seems that InterpreterClassSorter.sort is not a stable sorting algorithm (http://127.0.0.1:63320/node?ref=r%3Aea6cf71d-29d2-478d-8027-a9f4a4de53c4%28com.mbeddr.mpsutil.interpreter.rt%29%2F2447795128874040475)
  • Adding a relationship (Issue 2) doesn't help, since the sorting only affects sibling interpreters of the same combined interpreter. To solve it, we would need to make the sorting on a flattened list of interpreters (or later on the list of evaluators).

Severity

  • The interaction of the two issues made it complicated and took me some time to figure out
  • There is a workaround
  • It literally breaks the relationship-contract which it promised
  • I need to combine my interpreters at runtime. Nesting combined-interpreters is the easiest way to do it. Sometimes, all three should run, sometimes, only the category 1 should run. With this sorting working fine, I could rely on the interpreter caching instead of having to lookup the executable myself…

Workaround

Instead of grafik

I need to flatten the interpreter-structure and all is good:

grafik

abstraktor avatar Jul 22 '20 12:07 abstraktor

You can always use "runs before" and "runs after" dependencies in case you have a specific order requirement, which sorting of the evaluators then takes into account.

coolya avatar Jul 22 '20 21:07 coolya

Yes, that's the sorting which is performed by InterpreterClassSorter.sort. However, these rules only express dependencies between usual interpreters, not betweenCombinedIntepreters.

abstraktor avatar Jul 23 '20 06:07 abstraktor

Yes because we only support dependencies within a single category of interpreters which is represented as one combined interpreter. I would ague you use case of trying to combine different categories of interpreters and then have cross category sorting is rather exotic and not something we ever intended as a use case. It looks to me more or less like missing documentation and we could add a error message if an interpreter has a dependency to a generator from a different category to indicate that it will have no effect, would this have helped you to figure out the problem earlier?

coolya avatar Jul 23 '20 07:07 coolya

There is such a warning. That's why I'm fine with keeping Issue 2 asside. I suggest to only fixing Issue 1 for now.

Independently of that, there is no warning when declaring no relationships at all. I think, the fact that a CombinedInterpreter looks like being nestable is the issue here. Yet an error message would not really have helped me. I suggest to just make the sorting stable.

abstraktor avatar Jul 23 '20 09:07 abstraktor