feat: add environment mode per phase - #2595
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces per-phase EnvironmentMode overrides so individual solver phases can run with stricter assertion modes (e.g., FULL_ASSERT) without imposing that cost on the entire solve, and it refactors score director factory creation/lifecycle to support per-phase score directors.
Changes:
- Add
environmentModetoPhaseConfig, wire it into XSDs, and document configuration usage. - Refactor solver/phase construction so each phase can run under its own environment mode and corresponding score director (via
DelegateScoreDirectorFactory). - Fix list move selectors to avoid stale
ListVariableStateSupplyreferences when score directors are rebuilt, by introducingListVariableStateSupplyHolder.
Reviewed changes
Copilot reviewed 61 out of 61 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/benchmark/src/main/resources/benchmark.xsd | Adds environmentMode element to phase config schema. |
| docs/src/modules/ROOT/pages/running-timefold-solver/solver-diagnostics.adoc | Documents per-phase environment mode overrides with an example. |
| core/src/test/java/ai/timefold/solver/core/impl/solver/SolverMetricsIT.java | Updates latch-await assertions to JUnit assertDoesNotThrow. |
| core/src/test/java/ai/timefold/solver/core/impl/solver/DefaultSolverTest.java | Adds tests covering per-phase environment mode behavior and context restoration. |
| core/src/test/java/ai/timefold/solver/core/impl/solver/DefaultSolverFactoryTest.java | Adds validation tests for environment mode constraints across phases. |
| core/src/test/java/ai/timefold/solver/core/impl/score/director/stream/ConstraintStreamsBavetScoreDirectorSemanticsTest.java | Updates to use DelegateScoreDirectorFactory. |
| core/src/test/java/ai/timefold/solver/core/impl/score/director/incremental/IncrementalScoreDirectorTest.java | Mocks getEnvironmentMode() and adjusts no-op listener methods. |
| core/src/test/java/ai/timefold/solver/core/impl/score/director/incremental/IncrementalScoreDirectorSemanticsTest.java | Updates to use DelegateScoreDirectorFactory. |
| core/src/test/java/ai/timefold/solver/core/impl/score/director/easy/EasyScoreDirectorSemanticsTest.java | Updates to use DelegateScoreDirectorFactory. |
| core/src/test/java/ai/timefold/solver/core/impl/score/director/DelegateScoreDirectorFactoryTest.java | Renames/extends tests for the new delegate factory behavior. |
| core/src/test/java/ai/timefold/solver/core/impl/neighborhood/NeighborhoodsTest.java | Adapts acceptor/phase builder APIs to pass environment mode. |
| core/src/test/java/ai/timefold/solver/core/impl/localsearch/decider/acceptor/AcceptorFactoryTest.java | Updates acceptor factory API usage to include environment mode. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/RandomSubListSwapMoveSelectorTest.java | Ensures selectors receive phaseStarted lifecycle for fresh supplies. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/RandomSubListChangeMoveSelectorTest.java | Ensures selectors receive phaseStarted lifecycle for fresh supplies. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/RandomListChangeIteratorTest.java | Ensures destination selector receives phaseStarted lifecycle. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/ListSwapMoveSelectorTest.java | Ensures selector receives phaseStarted lifecycle for fresh supplies. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/ListChangeMoveSelectorTest.java | Ensures selector receives phaseStarted lifecycle for fresh supplies. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/list/RandomSubListSelectorTest.java | Ensures selector receives phaseStarted lifecycle for fresh supplies. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelectorTest.java | Ensures selector receives phaseStarted lifecycle for fresh supplies. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/ListVariableStateSupplyHolderTest.java | Adds unit test for ListVariableStateSupplyHolder demand/cancel behavior. |
| core/src/main/resources/solver.xsd | Adds environmentMode element to phase config schema. |
| core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java | Routes assertScoreFromScratch through score director instance. |
| core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecallerFactory.java | Delegates assertion enabling to BestSolutionRecaller. |
| core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java | Adds enableAssertions(EnvironmentMode) method. |
| core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolverFactory.java | Introduces default environment mode, delegate factory, and env-mode validation across phases. |
| core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java | Refactors construction to carry default context and delegate factory; logs default env mode. |
| core/src/main/java/ai/timefold/solver/core/impl/solver/AbstractSolver.java | Adds per-phase context swapping to run phases under different environment modes. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/ScoreDirectorFactory.java | Adds getEnvironmentMode() and adjusts builder generics; removes factory-level assert method. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/InnerScoreDirector.java | Adds assertScoreFromScratch and a counted increment method. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/DelegateScoreDirectorFactory.java | Replaces ScoreDirectorFactoryFactory and centralizes score director creation per env mode. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirectorFactory.java | Implements getEnvironmentMode() and moves score-from-scratch assertion off the factory. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java | Stores environment mode, adjusts tracking/assert logic, and implements assertScoreFromScratch. |
| core/src/main/java/ai/timefold/solver/core/impl/phase/Phase.java | Adds getEnvironmentMode() to phase API. |
| core/src/main/java/ai/timefold/solver/core/impl/phase/custom/DefaultCustomPhaseFactory.java | Resolves per-phase environment mode and passes it into phase builder. |
| core/src/main/java/ai/timefold/solver/core/impl/phase/custom/DefaultCustomPhase.java | Logs environment mode and threads it through the builder hierarchy. |
| core/src/main/java/ai/timefold/solver/core/impl/phase/AbstractPossiblyInitializingPhase.java | Threads environment mode into initializing phase builder base. |
| core/src/main/java/ai/timefold/solver/core/impl/phase/AbstractPhaseFactory.java | Adds shared resolveEnvironmentMode helper for phase factories. |
| core/src/main/java/ai/timefold/solver/core/impl/phase/AbstractPhase.java | Stores phase environment mode and uses it for assertion enabling. |
| core/src/main/java/ai/timefold/solver/core/impl/partitionedsearch/DefaultPartitionedSearchPhaseFactory.java | Passes resolved environment mode into enterprise partitioned search builder. |
| core/src/main/java/ai/timefold/solver/core/impl/localsearch/DefaultLocalSearchPhaseFactory.java | Propagates environment mode into decider/acceptor construction. |
| core/src/main/java/ai/timefold/solver/core/impl/localsearch/DefaultLocalSearchPhase.java | Logs environment mode and threads it through phase builder. |
| core/src/main/java/ai/timefold/solver/core/impl/localsearch/decider/acceptor/tabu/AbstractTabuAcceptor.java | Enables tabu assertions based on environment mode. |
| core/src/main/java/ai/timefold/solver/core/impl/localsearch/decider/acceptor/AcceptorFactory.java | Adds environment mode parameter and enables assertions accordingly. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/RuinRecreateConstructionHeuristicPhaseFactory.java | Passes environment mode into decider construction (root mode by default). |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/RuinRecreateConstructionHeuristicPhaseBuilder.java | Threads environment mode into builder construction/copying. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/ruin/ListRuinRecreateMoveSelector.java | Switches to ListVariableStateSupplyHolder to avoid stale supply across phase swaps. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/ListSwapMoveSelector.java | Switches to ListVariableStateSupplyHolder and phase lifecycle hooks. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/ListChangeMoveSelector.java | Switches to ListVariableStateSupplyHolder and phase lifecycle hooks. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/kopt/KOptListMoveSelector.java | Switches to ListVariableStateSupplyHolder and phase lifecycle hooks. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/RandomSubListSelector.java | Switches to ListVariableStateSupplyHolder and phase lifecycle hooks. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelector.java | Switches to ListVariableStateSupplyHolder and phase lifecycle hooks. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/HeuristicConfigPolicy.java | Renames/adjusts copying methods used by phase/child-thread policy creation. |
| core/src/main/java/ai/timefold/solver/core/impl/exhaustivesearch/DefaultExhaustiveSearchPhaseFactory.java | Resolves per-phase environment mode and propagates it into decider and phase builder. |
| core/src/main/java/ai/timefold/solver/core/impl/exhaustivesearch/DefaultExhaustiveSearchPhase.java | Logs environment mode and threads it through phase builder. |
| core/src/main/java/ai/timefold/solver/core/impl/exhaustivesearch/decider/AbstractExhaustiveSearchDecider.java | Enables decider assertions based on environment mode. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ListVariableStateSupplyHolder.java | Introduces helper to demand/cancel list state supply per phase start/end. |
| core/src/main/java/ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhaseFactory.java | Resolves per-phase environment mode and propagates it into decider and phase builder. |
| core/src/main/java/ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhase.java | Logs environment mode and threads it through phase builder. |
| core/src/main/java/ai/timefold/solver/core/enterprise/TimefoldSolverEnterpriseService.java | Extends enterprise partitioned search API to accept environment mode. |
| core/src/main/java/ai/timefold/solver/core/config/phase/PhaseConfig.java | Adds per-phase environmentMode config with JAXB/XSD support and inheritance. |
| core/src/build/revapi-differences.json | Ignores the JAXB @XmlType.propOrder change for PhaseConfig. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d5debf9 to
10a9562
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 61 changed files in this pull request and generated no new comments.
Suppressed comments (5)
core/src/test/java/ai/timefold/solver/core/impl/solver/SolverMetricsIT.java:228
- Using JUnit's assertDoesNotThrow violates the repository test convention requiring AssertJ assertions (see CONSTITUTION.md), so switch this to AssertJ and remove the JUnit import.
core/src/test/java/ai/timefold/solver/core/impl/solver/SolverMetricsIT.java:291 - Using JUnit's assertDoesNotThrow violates the repository test convention requiring AssertJ assertions (see CONSTITUTION.md), so switch this to AssertJ and remove the JUnit import.
core/src/test/java/ai/timefold/solver/core/impl/solver/SolverMetricsIT.java:440 - Using JUnit's assertDoesNotThrow violates the repository test convention requiring AssertJ assertions (see CONSTITUTION.md), so switch this to AssertJ and remove the JUnit import.
core/src/main/java/ai/timefold/solver/core/impl/solver/AbstractSolver.java:133 - AbstractSolver.preparePhase() always builds a new ScoreDirectorFactory/ScoreDirector when switching into a non-default environment mode, which contradicts the PR description of lazily reusing factories per distinct mode and may add avoidable overhead when phases switch modes repeatedly.
core/src/test/java/ai/timefold/solver/core/impl/solver/SolverMetricsIT.java:138 - Using JUnit's assertDoesNotThrow violates the repository test convention requiring AssertJ assertions (see CONSTITUTION.md), so switch this to AssertJ and remove the JUnit import.
This issue also appears in the following locations of the same file:
- line 228
- line 291
- line 440
b786b7b to
1a23c8e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 84 out of 84 changed files in this pull request and generated 5 comments.
Suppressed comments (3)
core/src/test/java/ai/timefold/solver/core/impl/solver/SolverMetricsIT.java:228
- CONSTITUTION.md forbids JUnit assertions in tests, so replace this assertDoesNotThrow call with an AssertJ equivalent.
core/src/test/java/ai/timefold/solver/core/impl/solver/SolverMetricsIT.java:291 - CONSTITUTION.md forbids JUnit assertions in tests, so replace this assertDoesNotThrow call with an AssertJ equivalent.
core/src/test/java/ai/timefold/solver/core/impl/solver/SolverMetricsIT.java:440 - CONSTITUTION.md forbids JUnit assertions in tests, so replace this assertDoesNotThrow call with an AssertJ equivalent.
9090a4d to
dca0bc9
Compare
triceo
left a comment
There was a problem hiding this comment.
- The changes to the benchmarker should go away; benchmarker should fail fast when this is detected. (Considering how tiny benchmarker test coverage is, we should not be touching it unless we absolutely have to.
- The Delegate factory is IMO overly complex, and overused.
- The SolverContextManager is poorly designed; its resource management is leaking to the rest of the solver.
- The global env mode should not be decided by the phases.
- Plus usual smaller comments.
| @@ -302,35 +303,62 @@ public List<String> getWarningList() { | |||
| List<String> warningList = new ArrayList<>(); | |||
| String javaVmName = System.getProperty("java.vm.name"); | |||
| if (javaVmName != null && javaVmName.contains("Client VM")) { | |||
There was a problem hiding this comment.
Client VMs have not existed for probably a decade now. This can be completely removed.
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| @NullMarked | ||
| public abstract class GenericListMoveSelector<Solution_> extends GenericMoveSelector<Solution_> { |
There was a problem hiding this comment.
Abstract classed, by convention, start by Abstract.
It appears we broke that convention already with the parent, fix that too.
| // Constraint Stream factory requires a new factory if the environment changes | ||
| this.requireNewFactoryOnDifferentEnvironment = config.getConstraintProviderClass() != null; |
There was a problem hiding this comment.
This is very brittle. You are making the assumption that only CS will ever need this, and when something in the other score directors changes, this immediately introduces a silent bug which nobody notices.
IMO it is safer for this to always be true, and therefore it doesn't need to exist at all.
| * <p> | ||
| * Since a solver phase may override the solver's environment mode, | ||
| * {@link #createScoreDirectorBuilder(EnvironmentMode)} may be called with a different mode than the default one. | ||
| * Most delegates only pass the environment mode on to the score director they build, | ||
| * so they can serve any mode and are reused as they are. | ||
| * The exception is {@link BavetConstraintStreamScoreDirectorFactory}, | ||
| * which builds its constraint network from the environment mode up front; | ||
| * for it, a separate delegate is built for the requested mode, | ||
| * then cached and shared like the default one, as building it is expensive. |
There was a problem hiding this comment.
See comment below. IMO this should not be the case.
| assertCorrectDirectorFactory(config); | ||
| this.solutionDescriptor = solutionDescriptor; | ||
| this.globalEnvironmentMode = environmentMode; | ||
| this.metricsRequiringConstraintMatchList = metricsRequiringConstraintMatchList; |
There was a problem hiding this comment.
The class should not be dealing with this. That is the delegate's problem.
Everything but the environment mode is the delegate's problem.
|
|
||
| A phase's environment mode must be at least as strict as the solver's environment mode; it can never be less strict. | ||
| Any number of phases can override it, including all of them: | ||
| the solver's environment mode still applies outside the phases. |
There was a problem hiding this comment.
| the solver's environment mode still applies outside the phases. | |
| the solver's global environment mode still applies outside these phases. |
| A phase's environment mode must be at least as strict as the solver's environment mode; it can never be less strict. | ||
| Any number of phases can override it, including all of them: | ||
| the solver's environment mode still applies outside the phases. | ||
| If the solver's environment mode is `<<environmentModeNonReproducible,NON_REPRODUCIBLE>>`, no phase can override it, |
There was a problem hiding this comment.
| If the solver's environment mode is `<<environmentModeNonReproducible,NON_REPRODUCIBLE>>`, no phase can override it, | |
| If the solver's global environment mode is `<<environmentModeNonReproducible,NON_REPRODUCIBLE>>`, no phase can override it, |
| If every phase ends up in the same environment mode, that mode becomes the solver's environment mode too, | ||
| since no phase is left running in the configured one. |
There was a problem hiding this comment.
IMO wrong and unexpected, as explained above.
| [NOTE] | ||
| ==== | ||
| The solver's environment mode also applies outside the phases, | ||
| including to xref:using-timefold-solver/modeling-planning-problems.adoc[`SolutionManager`] operations, | ||
| so a stricter solver-level mode makes those slower as well. | ||
| ==== |
There was a problem hiding this comment.
What does this mean? How is SolutionManager affected by solver config phases?
| [NOTE] | ||
| ==== | ||
| A phase that runs in a different environment mode than the solver gets its own score director. | ||
| With the xref:constraints-and-score/score-calculation.adoc#constraintStreams[Constraint Streams] API, | ||
| that means a second constraint network is built for that mode. | ||
| It is built once and reused, but it is not free: prefer overriding the phases you actually want to inspect. | ||
| ==== |
There was a problem hiding this comment.
| [NOTE] | |
| ==== | |
| A phase that runs in a different environment mode than the solver gets its own score director. | |
| With the xref:constraints-and-score/score-calculation.adoc#constraintStreams[Constraint Streams] API, | |
| that means a second constraint network is built for that mode. | |
| It is built once and reused, but it is not free: prefer overriding the phases you actually want to inspect. | |
| ==== | |
| NOTE: A phase that runs in a different environment mode than the solver gets its own score director. | |
| With the xref:constraints-and-score/score-calculation.adoc#constraintStreams[Constraint Streams] API, | |
| that means a second constraint network is built for that mode. | |
| It is built once and reused, but it is not free: prefer overriding the phases you actually want to inspect. |

Allows each solver phase (Construction Heuristic, Local Search, Exhaustive Search, Partitioned Search, Custom) to override the solver's
environmentModewith a stricter mode of its own viaPhaseConfig.withEnvironmentMode(...). This makes it possible to run a suspect phase underFULL_ASSERT(or another stricter mode) for debugging, without paying that performance cost for the whole solving run.NON_REPRODUCIBLE, no phase can override it (every other mode is reproducible, hence stricter).SolutionManager, the integrations).Both rules are enforced when the
SolverFactoryis built, so a misconfiguration fails there rather than during solving.Key changes
PhaseConfig: newenvironmentModefield/getter/setter/withEnvironmentMode(...), added to the@XmlTypepropOrder, and wired intosolver.xsd/benchmark.xsd. Includes arevapi-differences.jsonignore entry for the resulting@XmlType.propOrderchange. The accessors are@Nullable(null means "run in the solver's mode") and carry javadoc stating the rules above and the cost below.ScoreDirectorFactoryFactory→DelegateScoreDirectorFactory: renamed and reworked. It stays the single entry point that picks the score calculation implementation (easy / incremental / Constraint Streams) and hides the choice from callers. One delegate is still built eagerly for the solver's environment mode. Most delegates merely pass the mode on to the score director they build, so they serve any mode and are reused as they are; onlyBavetConstraintStreamScoreDirectorFactorybuilds its constraint network from the mode up front, so for it a separate delegate is built per requested mode and cached.DefaultSolverFactorycontinues to expose the factory built for the solver's own mode to consumers decoupled from the solving lifecycle (SolverManager, Quarkus DI injectingConstraintMetaModel).SolverContextManager(new): owns theInnerScoreDirectorthe solver is working with and swaps it when a phase requires a different environment mode. It compares the phase's mode to the current one atphaseStartedand only builds a replacement when they differ, carrying the working solution, the score calculation count, theSolverScope's view of both directors, and theBestSolutionRecaller's assertion level across the swap. Score directors themselves are not cached — caching lives one level down in the factory, which is the expensive part to build. Also fixes score director closing on the failure path, whereouterSolvingEndednever runs.AbstractSolver/DefaultSolver/ phase factories: restructured so each phase is built with, and runs under, its own config policy and score director rather than always reusing the solver-level one.Bundled fix — stale
ListVariableStateSupplyreferences in list move selectors (ElementDestinationSelector,RandomSubListSelector,ListChangeMoveSelector,ListSwapMoveSelector,KOptListMoveSelector,ListRuinRecreateMoveSelector). These previously cached the supply once at selector construction, which breaks as soon as a phase runs on a different score director. They now acquire it per phase inphaseStartedfrom the phase's own score director — which owns the supply and allocates it once — and drop it inphaseEnded.Benchmark report:
<environmentMode>is valid on phase configs inside benchmark solver configs, so the report can no longer read the solver-level mode and call it the truth. A newEnvironmentModeResolverin core is now the single answer to "which mode does this config actually run in" (validate+ a totalresolve, plus the per-phase and strictest-mode views), used by bothDefaultSolverFactoryand the benchmark instead of each deriving its own. The report now warns per solver benchmark on the strictest mode any of its phases runs in, naming the offending phase, and warns when the solver benchmarks in one report do not all resolve to the same mode, since their results are then not comparable.Docs: new "Using different environment modes per phase" section in
solver-diagnostics.adocwith a config example, the rules, and the cost note below.User-visible notes
SolutionManageroperations.PHASE_ASSERT (localSearch: FULL_ASSERT)rather than always a bare enum name, so anything parsing that column downstream needs to cope with the suffix.