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

CombinedInterpreter sorting is not stable #2139

Open
abstraktor opened this issue Jul 22, 2020 · 4 comments
Open

CombinedInterpreter sorting is not stable #2139

abstraktor opened this issue Jul 22, 2020 · 4 comments

Comments

@abstraktor
Copy link
Contributor

abstraktor commented Jul 22, 2020

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

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

@coolya
Copy link
Collaborator

coolya commented Jul 22, 2020

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.

@abstraktor
Copy link
Contributor Author

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

@coolya
Copy link
Collaborator

coolya commented Jul 23, 2020

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?

@abstraktor
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants