perf: add ability to ignore inconsistent solutions

[Christopher-Chianelli]

Added a new preview feature IGNORE_INCONSISTENT_SOLUTIONS.
When enabled, when a move causes the solution to be in an inconsistent state, that move is effectively ignored; calculateScore will return a score with the minimum init value possible (i.e. a score that is less than all other scores) and no shadow variables will be updated.

Technically, this potentially introduces a score trap: all inconsistent solutions get the same -infinity score. However, this doesn't seem to be a problem in the benchmarks I performed.

TopologicalGraph#commitChanges signature has changed to return a boolean which is true iff the graph has loops (and thus the solution inconsistent.

Benchmarks show significant improvements for Machine Job Scheduling on Enterprise.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
77.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[triceo]

What backwards incompatible behavior we are guarding against here using IGNORE_INCONSISTENT_SOLUTIONS? This feature is harmless; or can be made harmless if we ensure that a move which causes an invalid score never actually interacts with the user in any way. If the changes the move has caused are immediately undone, which they will be, you are already 90 % of the way there.

1. Should that move count towards move speed?
2. Should that move show up in the benchmarker?
3. Should that move be logged any differently than productive moves?

I want to argue 1-YES, 2-YES, 3-YES. It is a move like any other, and it does not leave the solver in any inconsistent state. The user should see it. But the score is pretty much pointless, which means the logging should - instead of showing a score - show some other message.

Score traps should not be a problem either. If this move never influences any of the solver's algorithms, it is as if it never existed, and therefore it can not cause a score trap. It will only reduce move speed, a vampiric loss.

Should the move be counted towards tabu search? IMO NO.
Should the move be counted towards late list? IMO NO.
We should create a separate category of "invalid moves" and ensure that our internal algorithms are not misguided by them.
(Although maybe not - if the move is never accepted, it will never enter these structures anyway.)

IMO we should work towards silently enabling this in the solver. We can do this backwards compatibly.

[triceo]

Observation (JS lta21, first 5 minutes):

Notice that the one method at the top takes 8x time of the second worst. This deserves optimization, not necessarily in this PR.

[Christopher-Chianelli]

What backwards incompatible behavior we are guarding against here using IGNORE_INCONSISTENT_SOLUTIONS? This feature is harmless; or can be made harmless if we ensure that a move which causes an invalid score never actually interacts with the user in any way. If the changes the move has caused are immediately undone, which they will be, you are already 90 % of the way there.

Some users might want to get the score of a planning solution with some inconsistent variables; that is currently allowed and is handled via @ShadowVariablesInconsistent (as one solution can be more inconsistent than another). Thing change effectively makes @ShadowVariablesInconsistent deprecated.

Observation (JS lta21, first 5 minutes): ...

TBH I am shocked that method is even being called; Entities can never be looped with how this is designed. My guess is it might still be iterating all the shadow variables on the entities to check if they are looped.

[triceo]

Some users might want to get the score of a planning solution with some inconsistent variables; that is currently allowed and is handled via @ShadowVariablesInconsistent (as one solution can be more inconsistent than another). Thing change effectively makes @ShadowVariablesInconsistent deprecated.

Fair point. Even though I'm not worried about this scenario, we can solve this without introducing any preview features:

• We deprecate the annotation.
• But if it is present, we activate the old code.
• If the annotation is not present, we use the new mechanism.
• The documentation will avoid mentioning the annotation prominently and over time, this will result in most users never even starting to use it.

Unfortunately, this means we will have to keep the MNR complexity, but so be it. We will also have to investigate all the things I mentioned above - these invalid moves must never interact with the solver's logic around how future moves are generated, or how new best solutions are selected.

[triceo]

More consequences to investigate:

• What should happen when the solver is given an inconsistent solution from the outside?
• What happens when you run score analysis on an inconsistent solution?
• What should happen when SolutionManager.updateShadowVariables() is called on an inconsistent problem?
• Any impacts on ConstraintVerifier?
• Any impacts on neighborhoods testing?

I'm starting to think that the solution should carry an inconsistency flag. @ShadowVariablesInconsistent, but on solution level. Optionally. That way, many problems can be avoided.

[Christopher-Chianelli]

More consequences to investigate:

* What should happen when the solver is given an inconsistent solution from the outside?

Currently, if the feature is enabled, it fail-fast on solve start.

* What happens when you run score analysis on an inconsistent solution?

I think it uses the old behaviour.

* What should happen when `SolutionManager.updateShadowVariables()` is called on an inconsistent problem?

Ditto.

* Any impacts on `ConstraintVerifier`?

None (unless we remove the ability to solve/calculate the score of inconsistent solutions

* Any impacts on neighborhoods testing?

I'm starting to think that the solution should carry an inconsistency flag. @ShadowVariablesInconsistent, but on solution level. Optionally. That way, many problems can be avoided.

How would that help? People cannot check the solution in their constraints; I would rather throw an exception in that case so users don't forget to check a flag.

[triceo]

Good to go to the feature branch when comments resolved.