Add missing `parallel=:threads` implementations
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.
What is the issue with using Distrubuted? As far as I know it is a standard library.
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.
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.
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.
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.
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
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)
LGTM, but I am a bit new to the maintainer team, and this does introduce a new public feature (the
parallelkwarg), 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.
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)
Anything else needed from me here?
(polite) bump - anything still blocking merge?
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.
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!