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

fix: Use custom thread factor class in SolverManager #875

Conversation

Christopher-Chianelli
Copy link
Contributor

No description provided.

@Christopher-Chianelli Christopher-Chianelli force-pushed the use-thread-factory-class-in-solver-manager branch from 26f3004 to bce9ba0 Compare June 11, 2024 20:47
Copy link

@Christopher-Chianelli Christopher-Chianelli merged commit 73bfaa6 into TimefoldAI:main Jun 12, 2024
18 checks passed
@triceo
Copy link
Contributor

triceo commented Jun 12, 2024

Why is this being done? Do we have a request somewhere, a justification?
This is highly unusual.

@triceo
Copy link
Contributor

triceo commented Jun 12, 2024

If this is something for the Python solver, I'd prefer if we had a code branch specifically for the Python solver, and leave the original behavior of the Java solver unchanged.

@Christopher-Chianelli
Copy link
Contributor Author

Christopher-Chianelli commented Jun 12, 2024

Two reasons:

  • It a genuine bug: threadFactoryClass on SolverManagerConfig literally did nothing before when used on SolverManager
  • Due to how Python/JVM interacts, Python Solver requires daemon threads to terminate when used with Control-C/interrupts.

@Christopher-Chianelli
Copy link
Contributor Author

(If thread factory unset in the SolverManagerConfig, original behaviour is unchanged; Executors.newFixedThreadPool(parallelSolverCount, Executors.defaultThreadFactory()) == Executors.newFixedThreadPool(parallelSolverCount)

@zepfred
Copy link
Contributor

zepfred commented Jun 12, 2024

@triceo, this change does not affect the default behavior, which already sets the thread factory to Executors.defaultThreadFactory() when calling Executors.newFixedThreadPool(parallelSolverCount);.

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.

3 participants