-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add delete subgraph functionality #1053
Conversation
99a505e
to
e66b611
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking very good. After you fix a few nits and typos and address one question this should be good to merge. Thanks a lot!
e66b611
to
00a4f2f
Compare
672f7a8
to
59ebdfb
Compare
5c83c07
to
ad84d79
Compare
After finding a segfault centered around not deleting elements from the by_outedge member of the resource graph metadata, I added in a few new changes to solve this issue.
|
ad84d79
to
ab8b192
Compare
Pincer maneuver complete |
ab8b192
to
fe91ecb
Compare
@milroy I add the most recent changes requested including the two TODO messages and spacing fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! LGTM.
@zekemorton since PR #1080 is merged you can rebase this PR and set MWP. |
Problem: adding elasicity into fluxion requires the addition of a new feature to remove subgraphs from the resource graph. This implementation removes nodes from the resource graph metadata and clears the node of edges. This makes those nodes essentially unreachable through traversal or look up from the metadata. Actually deleting nodes from the resource graph would invalidate the vertex descriptors and resource graph metadata. This is because the resource graph is using a vecS instead of a listS for the Boost adjacency list.
Problem: the remove subgraph feature is a part of the readers but does not have functionality in resource query Add support for remove graph in resource query
problem: the remove subgrapgh feature added new code that needs to be tested as a part of automated ci add tests for this new feature
fe91ecb
to
e9c4bfc
Compare
Approving reviews have been dismissed because this pull request
was updated.
Codecov Report
@@ Coverage Diff @@
## master #1053 +/- ##
========================================
+ Coverage 71.6% 71.7% +0.1%
========================================
Files 89 89
Lines 11569 11665 +96
========================================
+ Hits 8289 8371 +82
- Misses 3280 3294 +14
|
@milroy looks like it might need another approval since doing the rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving again in the hopes that this will trigger MWP action.
This PR adds delete subgraph functionality to the jgf reader and resource query. This is a part of the elasticity effort and allows removal of a piece of the resource graph. This function works by passing in a path of a node in the graph (ex /medium0/rack3) and will remove the tree with that node as root (ex /medium0/rack3/*).
This implementation requires two main steps. The first step is to identify all boost graph nodes in the subgraph. This is done via a DFS on the out edges of each node. Note that one of the out edges of a node is its parent, so in this case a string comparison of the path was used to determine that an out edge is not the parent. The second step is to remove the nodes from the graph. Because we are using listS instead of vecS, actually deleting the nodes would invalidate the vertex descriptors, which would invalidate the resource graph metadata used for vertex descriptor look ups. Instead, this PR clears all edges from "deleted nodes" and removes them from the resource graph metadata. This makes them unaccessible from traversals from the main graph and unaccessible from resource graph metadata look ups.
This PR also adds this functionality to resource query and adds new tests for these features.
WIP to do items: