feat: add support for moveCountLimit - #2570
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the service termination configuration to support a new moveCountLimit hard cap, wiring it from platform config and per-request API through to solver TerminationConfig, with accompanying tests and user documentation updates.
Changes:
- Add
moveCountLimittoSolverTerminationConfigAPI model and propagate it throughTerminationService. - Disable diminished-returns termination whenever
moveCountLimitis set (mirroring existing step-count behavior) and allow combining step + move limits. - Update tests and the consumer guide to cover and document the new option.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| service/worker/src/test/java/ai/timefold/solver/service/worker/impl/termination/TerminationServiceTest.java | Extends worker termination tests to cover moveCountLimit behavior and its interaction with diminished returns. |
| service/worker/src/main/java/ai/timefold/solver/service/worker/impl/termination/TerminationService.java | Adds platform-level moveCountLimit support and forwards request/platform resolution into TerminationConfig. |
| service/worker/src/main/java/ai/timefold/solver/service/worker/impl/termination/TerminationConfigParams.java | Introduces the new platform config key for move count limit. |
| service/definition/src/test/java/ai/timefold/solver/service/definition/api/termination/SolverTerminationConfigTest.java | Adds API-model tests for new constructor/override behavior involving moveCountLimit. |
| service/definition/src/main/java/ai/timefold/solver/service/definition/api/termination/SolverTerminationConfig.java | Adds moveCountLimit to the public API record and updates override/conflict rules and schema docs. |
| docs/src/modules/ROOT/pages/running-timefold-solver/service/consumer-guide.adoc | Documents moveCountLimit, its constraints, and its combination behavior with stepCountLimit. |
5948fd4 to
cac6abc
Compare
cac6abc to
d032dd7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
service/test-model/src/build/openapi.json:11
- This PR is scoped to moveCountLimit but the committed OpenAPI snapshot also changes the API version/path to v1 and adds unrelated schema changes (for example MapsConfiguration.useTraffic), so please confirm these are intended or split/regenerate the snapshot to avoid unrelated API diffs.
"version" : "v1"
service/definition/src/main/java/ai/timefold/solver/service/definition/api/termination/SolverTerminationConfig.java:55
- The schema/docs state unimprovedSpentLimit must not be combined with stepCountLimit or moveCountLimit, but the canonical constructor currently accepts that invalid combination and it will be silently resolved/ignored later, so add the same mutual-exclusion validation used in override().
public SolverTerminationConfig {
if (minimumImprovementRatio != null && minimumImprovementRatio <= 0) {
throw new IllegalArgumentException(
"minimumImprovementRatio (" + minimumImprovementRatio + ") must be strictly positive.");
}
}
|
| "email" : "" | ||
| }, | ||
| "version" : "v2-beta" | ||
| "version" : "v1" |
There was a problem hiding this comment.
yes, but not related to this pr, it only adds the new fields. It will be removed once #2571 is merged.
| } else if (stepCountLimit != null || moveCountLimit != null) { | ||
| // stepCountLimit and moveCountLimit are both hard, deterministic caps used for benchmarking; | ||
| // they are OR-composed and may be combined so whichever is reached first terminates the solver. | ||
| List<String> limits = new ArrayList<>(2); |
There was a problem hiding this comment.
var limits = new ArrayList<>(2);
There was a problem hiding this comment.
that would break the String.join(" or ", limits)`
zepfred
left a comment
There was a problem hiding this comment.
LGTM! Thank you for implementing the feature so quickly!
| public static final String TERMINATION_MAXIMUM_UNIMPROVED_SPENT_LIMIT = | ||
| "ai.timefold.platform.termination.maximum-unimproved-spent-limit"; | ||
| public static final String TERMINATION_STEP_COUNT_LIMIT = "ai.timefold.platform.termination.step-count-limit"; | ||
| public static final String TERMINATION_MOVE_COUNT_LIMIT = "ai.timefold.platform.termination.move-count-limit"; |
There was a problem hiding this comment.
isn't this being renamed to avoid platform name in the properties? https://github.com/TimefoldAI/timefold-solver/pull/2545/changes#diff-63b1e917bd1fd3d3dd458d719056346ef3d51d23086b99b84b326ee7a0bf163cR5



Related to https://github.com/TimefoldAI/timefold-platform/issues/5271