Graphs.jl icon indicating copy to clipboard operation
Graphs.jl copied to clipboard

Add missing `parallel=:threads` implementations

Open topolarity opened this issue 9 months ago • 10 comments

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.

topolarity avatar May 14 '25 12:05 topolarity

What is the issue with using Distrubuted? As far as I know it is a standard library.

simonschoelly avatar May 14 '25 13:05 simonschoelly

Multithreading typically has lower overhead since it's shared memory. Distributed is usually only worthwhile if you are trying to distribute across multiple machines.

oscardssmith avatar May 14 '25 13:05 oscardssmith

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.

oscardssmith avatar May 14 '25 13:05 oscardssmith

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.

Krastanov avatar May 14 '25 13:05 Krastanov

Codecov Report

Attention: Patch coverage is 94.33962% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.38%. Comparing base (6130332) to head (d8a8730). Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
src/Parallel/distance.jl 94.73% 1 Missing :warning:
src/Parallel/shortestpaths/dijkstra.jl 94.44% 1 Missing :warning:
src/Parallel/traversals/greedy_color.jl 93.75% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
+ Coverage   97.31%   97.38%   +0.07%     
==========================================
  Files         117      120       +3     
  Lines        6956     7007      +51     
==========================================
+ Hits         6769     6824      +55     
+ Misses        187      183       -4     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar May 14 '25 14:05 codecov[bot]

Would this require also bumping the minimum requirement to julia=1.9?

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

topolarity avatar May 14 '25 16:05 topolarity

a new public feature (the parallel kwarg)

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 parallel=:distributed implementation)

topolarity avatar May 14 '25 17:05 topolarity

LGTM, but I am a bit new to the maintainer team, and this does introduce a new public feature (the parallel kwarg), so I will wait a bit for dissent from folks with more seniority.

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.

simonschoelly avatar May 14 '25 17:05 simonschoelly

The PR Pre-Commit Bot failure is a problem with the github action, not this PR. (Probably due to this coming from a fork -- I will look into fixing the action)

Krastanov avatar Jun 06 '25 12:06 Krastanov

Anything else needed from me here?

topolarity avatar Jun 12 '25 11:06 topolarity

(polite) bump - anything still blocking merge?

topolarity avatar Jul 01 '25 00:07 topolarity

Thanks, @topolarity ! Sorry for the slow response, we are a bit low on volunteers right now and Simon is the only current maintainer with actual deep knowledge of the codebase.

Looking through the code, I do not see any issues, all of Simon's comments have been addressed, and any documentation requirements or API restructuring needs are reported as separate posts on the issue tracker. This seems to be complete. I will rerun the tests and merge after they pass.

Krastanov avatar Jul 02 '25 11:07 Krastanov

The patch codecov is failing, but the overall project coverage is increasing, so it seems to be good on that front too. Merging. Thank you for fixing this up!

Krastanov avatar Jul 02 '25 12:07 Krastanov