Make dense layout algorithm deterministic when run in parallel #13133
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The dense layout algorithm is trying to find the densest k subgraph of the connectivity graph. To do this it performs a BFS from each node in the in graph of k nodes to determine the subgraph with the most number of edges. But in cases of ties where there are subgraphs with the same number of edges the exact output would be determined by the iteration order that we're evaluating a BFS search. However, this algorithm runs in parallel in most cases and the exact iteration order isn't going to be stable when running in parallel. It will depend on which threads finish first. This commit fixes this potential non-determinism in the algorithm by defaulting to the lower node index's trial results instead of relying on the execution order. This should mean we return identical results regardless of how many threads are run or how quickly they execute.
|
One or more of the following people are relevant to this code:
|
mtreinish
added
Changelog: Bugfix
Pull Request Test Coverage Report for Build 10820177047Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
henryzou50
reviewed
Sep 11, 2024
There was a problem hiding this comment.
LGTM! This resolves the non-determinism issue in the reduce_fn function. By introducing the index field in the SubsetResult struct, the comparison now has a consistent tie-breaking mechanism when count values are equal. The index ensures that, even when multiple subgraphs have the same number of edges, the result is deterministic, as the subgraph with the lower index is always chosen.
This removes the dependency on thread execution speed and order, making the function’s behavior predictable in parallel environments. Thanks!
|
Some notes after reviewing it again. Need to add release notes and add a check for the error aware case. |
jakelishman
approved these changes
Sep 19, 2024
mergify bot
pushed a commit
that referenced
this pull request
Sep 19, 2024
* Make dense layout algorithm deterministic when run in parallel The dense layout algorithm is trying to find the densest k subgraph of the connectivity graph. To do this it performs a BFS from each node in the in graph of k nodes to determine the subgraph with the most number of edges. But in cases of ties where there are subgraphs with the same number of edges the exact output would be determined by the iteration order that we're evaluating a BFS search. However, this algorithm runs in parallel in most cases and the exact iteration order isn't going to be stable when running in parallel. It will depend on which threads finish first. This commit fixes this potential non-determinism in the algorithm by defaulting to the lower node index's trial results instead of relying on the execution order. This should mean we return identical results regardless of how many threads are run or how quickly they execute. * Add release note * Also handle the use_error case too (cherry picked from commit 65bb09e)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 19, 2024
… (#13190) * Make dense layout algorithm deterministic when run in parallel The dense layout algorithm is trying to find the densest k subgraph of the connectivity graph. To do this it performs a BFS from each node in the in graph of k nodes to determine the subgraph with the most number of edges. But in cases of ties where there are subgraphs with the same number of edges the exact output would be determined by the iteration order that we're evaluating a BFS search. However, this algorithm runs in parallel in most cases and the exact iteration order isn't going to be stable when running in parallel. It will depend on which threads finish first. This commit fixes this potential non-determinism in the algorithm by defaulting to the lower node index's trial results instead of relying on the execution order. This should mean we return identical results regardless of how many threads are run or how quickly they execute. * Add release note * Also handle the use_error case too (cherry picked from commit 65bb09e) Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 19, 2024
… (#13190) * Make dense layout algorithm deterministic when run in parallel The dense layout algorithm is trying to find the densest k subgraph of the connectivity graph. To do this it performs a BFS from each node in the in graph of k nodes to determine the subgraph with the most number of edges. But in cases of ties where there are subgraphs with the same number of edges the exact output would be determined by the iteration order that we're evaluating a BFS search. However, this algorithm runs in parallel in most cases and the exact iteration order isn't going to be stable when running in parallel. It will depend on which threads finish first. This commit fixes this potential non-determinism in the algorithm by defaulting to the lower node index's trial results instead of relying on the execution order. This should mean we return identical results regardless of how many threads are run or how quickly they execute. * Add release note * Also handle the use_error case too (cherry picked from commit 65bb09e) Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The dense layout algorithm is trying to find the densest k subgraph of the connectivity graph. To do this it performs a BFS from each node in the in graph of k nodes to determine the subgraph with the most number of edges. But in cases of ties where there are subgraphs with the same number of edges the exact output would be determined by the iteration order that we're evaluating a BFS search. However, this algorithm runs in parallel in most cases and the exact iteration order isn't going to be stable when running in parallel. It will depend on which threads finish first. This commit fixes this potential non-determinism in the algorithm by defaulting to the lower node index's trial results instead of relying on the execution order. This should mean we return identical results regardless of how many threads are run or how quickly they execute.
Details and comments