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

feat: add solver job start event #1084

Merged
merged 8 commits into from
Sep 19, 2024
Merged

Conversation

zepfred
Copy link
Contributor

@zepfred zepfred commented Sep 12, 2024

This pull request adds a new job configuration that allows an action to be run when the solver starts the solving process. The current approach uses a PhaseLifecycleListenerAdapter to trigger the event because the API SolverEventListener would need to be changed in order to add a new solver event.

@zepfred zepfred requested a review from triceo September 12, 2024 17:24
@zepfred zepfred linked an issue Sep 12, 2024 that may be closed by this pull request
@triceo
Copy link
Contributor

triceo commented Sep 16, 2024

We've already added events before, haven't we? The solution initialization event comes to mind.
What makes this one different, why are we suddenly afraid of changing the API for it?

@zepfred
Copy link
Contributor Author

zepfred commented Sep 16, 2024

We've already added events before, haven't we? The solution initialization event comes to mind.

We used the PhaseLifecycleListenerAdapter for this specific event, which was the appropriate choice as it relates to the phase itself.

What makes this one different, why are we suddenly afraid of changing the API for it?

In this case, it is a solver event and should be called once during DefaultSolver#solvingStarted, but it can also be solved using the phase event lifecycle as proposed by this PR. My goal was to keep this pull request simple because changing the contract SolverEventListener would impact many other areas of the solver.

We may consider creating a separate issue to refactor SolverEventListener to make the solver events system more extensible.

Copy link
Contributor

@triceo triceo left a comment

Choose a reason for hiding this comment

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

Looks good so far.

Copy link
Contributor

@triceo triceo left a comment

Choose a reason for hiding this comment

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

Please switch to listen to the proper event. After that, LGTM.

triceo
triceo previously approved these changes Sep 17, 2024
@triceo triceo modified the milestone: 1.15.0 Sep 19, 2024
Copy link

@triceo triceo merged commit 0bf63f6 into TimefoldAI:main Sep 19, 2024
27 checks passed
@zepfred zepfred deleted the new-solver-event branch September 19, 2024 13:34
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.

Feat: Solver job notification for "solver started"
2 participants