Skip to content

feat: add support for moveCountLimit - #1147

Closed
cristianonicolai wants to merge 1 commit into
TimefoldAI:developmentfrom
cristianonicolai:feat/movecount
Closed

feat: add support for moveCountLimit#1147
cristianonicolai wants to merge 1 commit into
TimefoldAI:developmentfrom
cristianonicolai:feat/movecount

Conversation

@cristianonicolai

Copy link
Copy Markdown
Contributor

Propagate OpenAPI changes from TimefoldAI/timefold-solver#2570

Copilot AI lite review requested due to automatic review settings August 5, 2026 07:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR propagates upstream OpenAPI contract changes into the school timetabling quickstart, adding the new moveCountLimit solver termination option to the generated OpenAPI specification and applying minor formatting cleanups in related Java sources.

Changes:

  • Update OpenAPI schema for SolverTerminationConfig to include moveCountLimit and adjust related descriptions/constraints.
  • Normalize import grouping/formatting in TimetableResource.
  • Reorder an import and remove trailing whitespace in Timetable.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
getting-started/service/src/main/java/com/acme/schooltimetabling/rest/TimetableResource.java Minor import formatting (group separation).
getting-started/service/src/main/java/com/acme/schooltimetabling/domain/Timetable.java Import order cleanup and whitespace-only formatting fix.
getting-started/service/src/build/openapi.json Adds moveCountLimit to SolverTerminationConfig and updates termination option descriptions accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cristianonicolai
cristianonicolai requested a review from rsynek August 5, 2026 08:23
@triceo

triceo commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

@rsynek Do we really need to update quickstarts every time we add something to the API? IMO this crates more work than benefit.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@cristianonicolai

Copy link
Copy Markdown
Contributor Author

@triceo @rsynek anything I need to do to retry/trigger these failing checks?

@mswiderski

Copy link
Copy Markdown

@rsynek Do we really need to update quickstarts every time we add something to the API? IMO this crates more work than benefit.

I agree we need to find a way to make this smoother.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (4)

getting-started/service/src/build/openapi.json:2177

  • The moveCountLimit property description says "Must be at least 1", but the OpenAPI schema does not include a numeric constraint ("minimum": 1). Adding it improves correctness and helps client-side validation.
          "moveCountLimit" : {
            "format" : "int64",
            "description" : "Maximum number of moves evaluated by the solver. Must be at least 1. The solver will stop solving after a pre-determined amount of moves have been evaluated. Use when you require results independently of the hardware resources performance. Use this termination if you want to benchmark your models, not recommended for production use. If set, unimprovedSpentLimit must be empty. Can be combined with stepCountLimit: the solver stops as soon as either limit is reached. Warning: using this option will disable the default diminished returns termination which is recommended for most use cases.",
            "type" : "integer",
            "example" : 100000

use-cases/conference-scheduling/src/build/openapi.json:2723

  • The schema description for moveCountLimit says "Must be at least 1", but the OpenAPI property definition does not include a corresponding numeric constraint ("minimum": 1). Adding it would make the schema self-descriptive and improve client-side validation.
          "moveCountLimit" : {
            "format" : "int64",
            "description" : "Maximum number of moves evaluated by the solver. Must be at least 1. The solver will stop solving after a pre-determined amount of moves have been evaluated. Use when you require results independently of the hardware resources performance. Use this termination if you want to benchmark your models, not recommended for production use. If set, unimprovedSpentLimit must be empty. Can be combined with stepCountLimit: the solver stops as soon as either limit is reached. Warning: using this option will disable the default diminished returns termination which is recommended for most use cases.",
            "type" : "integer",
            "example" : 100000

use-cases/conference-scheduling/src/build/openapi.json:2717

  • The schema now states stepCountLimit "Must be at least 1", but the OpenAPI property definition does not enforce this (no "minimum": 1). This makes the contract inconsistent with the description and may lead to clients sending invalid values without early validation.

This issue also appears on line 2719 of the same file.

          "stepCountLimit" : {
            "format" : "int32",
            "description" : "Maximum solver step count. Must be at least 1. The solver will stop solving after a pre-determined amount of steps. Use when you require results independently of the hardware resources performance. Use this termination if you want to benchmark your models, not recommended for production use. If set, unimprovedSpentLimit must be empty. Warning: using this option will disable the default diminished returns termination which is recommended for most use cases.",
            "type" : "integer",
            "example" : 1000

getting-started/service/src/build/openapi.json:2171

  • The schema now states stepCountLimit "Must be at least 1", but the OpenAPI property definition does not enforce this constraint (no "minimum": 1). This makes the contract inconsistent with the description and may reduce usefulness for generated clients.

This issue also appears on line 2173 of the same file.

          "stepCountLimit" : {
            "format" : "int32",
            "description" : "Maximum solver step count. Must be at least 1. The solver will stop solving after a pre-determined amount of steps. Use when you require results independently of the hardware resources performance. Use this termination if you want to benchmark your models, not recommended for production use. If set, unimprovedSpentLimit must be empty. Warning: using this option will disable the default diminished returns termination which is recommended for most use cases.",
            "type" : "integer",
            "example" : 1000

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

@rsynek

rsynek commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Ad API changes: that's the unfortunate side effect of having the backward compatibility check for diffs in the openapi.json. We don't need the file for anything else, although it's good for a model developer to see the resulting specs before merging a PR.

Options:
A) download the previous version's model descriptor, unzip zip and take the openapi.json file from there. But we need to deploy the model descriptor in some maven repository in the first place. Complicates the setup, especially for 3rd party models.

B) Make the diff checking optional via a maven profile and disable it for quickstarts (but does not solve the general problem).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

@rsynek

rsynek commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

@cristianonicolai could you kindly reopen the PR from a feature branch in the blessed repo?

@rsynek

rsynek commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Closed in favor of #1164.

@rsynek rsynek closed this Aug 21, 2026
@cristianonicolai
cristianonicolai deleted the feat/movecount branch August 21, 2026 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants