Parallelize solve_problems - #195
MaxenceGollier wants to merge 6 commits into
Conversation
|
The race condition causes issue, for example, see MaxenceGollier/Penelopt.jl#57. I will fix the BSD tests as well. |
a9f9ba9 to
09a20fa
Compare
tmigot
left a comment
tmigot
left a comment
There was a problem hiding this comment.
@MaxenceGollier If I understand correctly you propose to do multi-threading over problems. This assumes that problems are thread-safe, which is the case for CUTEst and (in theory) OptimizationProblems.jl, however it also needs the solvers to be thread safe, which I don't think we really test.
If we are going for a multi-threaded version, I think it should be a different function than solve_problems also, because it will never be safe in general (for any set of problems, and any set of solvers).
| stats = DataFrame(names .=> [T[] for T in types]) | ||
| stats_lock = ReentrantLock() |
There was a problem hiding this comment.
Would it be better to have one stats per thread and merge all of them in the end ?
|
The idea would be that your version is faster when it is possible, and #194 would be safer. So, both approaches might be complementary. |
09a20fa to
a3392f3
Compare
This reverts commit a3392f3.
|
Hi @tmigot, i separated in two distinct functions. My issue is that there is a lot of repeated code which makes it harder to maintain... |
There was a problem hiding this comment.
Pull request overview
Adds a threaded execution path for benchmarking by parallelizing solve_problems over problems, and wires it into bmark_solvers behind a new parallel keyword.
Changes:
- Introduces
solve_problems_parallelinsrc/run_solver.jlusingThreads.@spawnand a lock to protect writes to the sharedstatsDataFrame. - Adds
parallel::Bool=falsetobmark_solversto selectsolve_problems_parallelwhen multiple Julia threads are available.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/run_solver.jl | Adds a new parallel solver runner and a locked per-problem execution helper. |
| src/bmark_solvers.jl | Adds a parallel keyword to switch between serial and threaded benchmarking. |
Suppressed comments (1)
src/run_solver.jl:330
_run_problemis called inside the spawned task, but there is no_run_problemdefinition in the codebase. This will throw aUndefVarErrorwhen the parallel section runs.
_run_problem(
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Make a first serial run until first_problem is false | ||
| final_id = 0 | ||
| for (id, problem) in enumerate(problems) | ||
| first_problem, nb_unsuccessful_since_start = _run_problem( |
| problem, | ||
| stats::DataFrame, | ||
| solver, | ||
| solver_name::Symbol, |
| # Make a first serial run until first_problem is false | ||
| final_id = 0 | ||
| for (id, problem) in enumerate(problems) |
| if parallel && Threads.nthreads() > 1 | ||
| stats[name] = solve_problems_parallel(solver, name, args...; kwargs...) | ||
| else | ||
| parallel && @warn "SolverBenchmarks.jl: parallel is set to true but the number of threads is $(Threads.nthreads()). Running in serial mode." |
|
@MaxenceGollier Sorry I have very limited time for JSO. But I'd be happy to make another round of review. What is the status here as I see some unadressed comments in the PR ? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #195 +/- ##
==========================================
- Coverage 76.71% 73.40% -3.32%
==========================================
Files 12 10 -2
Lines 292 391 +99
==========================================
+ Hits 224 287 +63
- Misses 68 104 +36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No problem @tmigot. This is the current status of the PR. I have coded a workflow in the repo : https://github.com/MaxenceGollier/Penelopt.jl that basically runs my benchmarks on each PR, using SolverBenchmark.jl. I tried using this PR to make my benchmarks faster. |
|
Ok so we switch this to a draft waiting for an actual use ? |
@dpo, @tmigot
#120
(hopefully) supersedes #127, #167, #176.
To parallelize over solvers instead of problems, CUTEst problems make it very very difficult, i have tried multiple times but failed. I think it is fine just to parallelize over problems.
To make things clear,
_run_problemwhich just performs one iteration of the loop.first_problemis set to false to avoid race conditions on this variable and other variables accessed whenfirst_problemis true.