Add missing parallel=:threads implementations
#429
Conversation
|
What is the issue with using |
|
Multithreading typically has lower overhead since it's shared memory. Distributed is usually only worthwhile if you are trying to distribute across multiple machines. |
|
Also, Distributed is a stdlib, but stdlibs are still dependencies, and as we have started moving stlibs out of the sysimage, they are becoming more like regular packages. |
topolarity
force-pushed
the
ct/missing-threaded-impls
branch
from
b5b55a0 to
c109686
Compare
|
Would this require also bumping the minimum requirement to julia=1.9? I am generally in favor of moving more dependencies to optional dependencies. It makes compilation times much more pleasant as well. |
topolarity
force-pushed
the
ct/missing-threaded-impls
branch
2 times, most recently
from
2e9b9f3 to
a7db672
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #429 +/- ##
==========================================
- Coverage 97.31% 97.24% -0.07%
==========================================
Files 117 117
Lines 6956 7004 +48
==========================================
+ Hits 6769 6811 +42
- Misses 187 193 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
These implementations are extremely basic, but they try to follow the patterns in the other parts of the Parallel module. My real motivation is to be able to move the Distributed implementations into an extension, so that Graphs.jl does not depend on Distributed.
topolarity
force-pushed
the
ct/missing-threaded-impls
branch
from
fc4a1b9 to
cbda59e
Compare
There are compatibility pathways where we only switch to an extension on 1.9+ and keep the hard dependency for earlier versions I think this PR should be ready for review now - it is passing tests for me locally |
FWIW, that kwarg is already there on a lot of other methods, so it's not a new design choice exactly - it was just never provided for these particular operations (because they only had a |
There is some new multi threaded code here - so we probably should take a bit of time to review it - as it can lead to notoriously difficult to spot bugs. |
| g::AbstractGraph, | ||
| vs=vertices(g), | ||
| distmx::AbstractMatrix{T}=weights(g); | ||
| parallel=:distributed, |
There was a problem hiding this comment.
| parallel=:distributed, | |
| parallel::Symbol=:distributed, |
There was a problem hiding this comment.
I know we haven't done that in other places - but perhaps to make this code more future proof would it make sense to use a specific structure to specify the parallel policy? Then we could later add things such as the number of threads/cores used.
| distr_eccentricity(g, vs, distmx) | ||
| else | ||
| error( | ||
| "Unsupported parallel argument '$(repr(parallel))' (supported: ':threads' or ':distributed')", |
There was a problem hiding this comment.
We should also add a test for the case that someone uses the wrong argument.
There was a problem hiding this comment.
We repeat the same error multiple times - Perhaps we can throw a custom error type instead?
| g::AbstractGraph, vs=vertices(g), distmx::AbstractMatrix{T}=weights(g) | ||
| ) where {T<:Number} | ||
| vlen = length(vs) | ||
| eccs = SharedVector{T}(vlen) | ||
| @sync @distributed for i in 1:vlen | ||
| eccs[i] = maximum(Graphs.dijkstra_shortest_paths(g, vs[i], distmx).dists) | ||
| d′ = Graphs.dijkstra_shortest_paths(g, vs[i], distmx) |
There was a problem hiding this comment.
I am a bit unsure about the usage of d′ - it is hardly distinguishable from d' - in that case ' is the adjoint operator.
| [1, 3] | ||
| @test @inferred(Graphs.radius(y)) == @inferred(Parallel.radius(g, distmx1)) == 4.2 | ||
| @test @inferred(Graphs.center(y)) == @inferred(Parallel.center(g, distmx1)) == [2] | ||
| for parallel in [:threads, :distributed] |
There was a problem hiding this comment.
It might make sense to write the test as
@testset "Parallel.Distance" for parallel in [:threads, :distributed]
[...]
end| end | ||
|
|
||
| function threaded_random_greedy_color(g::AbstractGraph{T}, reps::Integer) where {T<:Integer} | ||
| local_best = Any[nothing for _ in 1:reps] |
There was a problem hiding this comment.
Is there a reason to use Any[nothing for _ in 1:reps] instead of
Vector{Graphs.Coloring{T}}(undef, reps)or
Vector{Union{Nothing, Graphs.Coloring{T}}}(nothing, reps)?
There was a problem hiding this comment.
Looks mostly good to me. I don't see any race condition
I have added some comments. They are mostly suggestions. In addition:
- We probably should write some documentation for the new
parallelargument. Although I admit that this module is fairly undocumented anyway - and as we don't export anything yet we don't have to be so rigorous.
These implementations are basic, but I tried to follow the patterns in the other parts of the Parallel module.
My real motivation is to be able to move the Distributed implementations into an extension, so that Graphs.jl does not depend on Distributed.