Add unit tests for `lib/utils/graph` #842

lambda7xx · 2023-07-07T09:14:12Z

Description of changes:
This PR is for the repo-refactor utils.

Before merging:

finish the GraphView::unsafe_create(IGraphView const &) and unsafe(IMultiDiGraphView const &) and unsafe(IDiGraphView const&)
add test for test_adjacency_multidigraph.cc
add test for algorithm

This change is

lockshaw

In general, be more thorough about making sure you are more aware of existing code. You've got a few places where you made a fix I requested, but only to one out of multiple places, or you implement a method of which multiple analogues exist but you don't match how they're implemented (not that this is always wrong, but it is usually). Before immediately jumping into and implementing things, use grep or some fuzzy search of the code to check if something similar has already been done.

Also, I recommend looking at the existing code and trying to match its style. Generally we prefer to draw from containers.h when possible rather than writing a bunch of raw loops, which are harder to maintain and easier to mess up. Also, refs in loops, etc.

And please run ./scripts/format.sh on your code before requesting reviews 🙂

lib/CMakeLists.txt

lib/utils/include/utils/graph/adjacency_digraph.h

lib/utils/include/utils/graph/adjacency_multidigraph.h

lib/utils/include/utils/containers.h

lib/utils/include/utils/graph/digraph.h

lib/utils/test/src/test_algorithms.cc

lockshaw · 2023-07-10T12:11:14Z

lib/utils/test/src/test_algorithms.cc

 }

 TEST_CASE("traversal") {
  AdjacencyDiGraph g;
-  std::vector<Node> const n = add_nodes(g, 4);
+  std::vector<Node> n;


Why not add_nodes?

change to use add_nodes.

lib/utils/test/src/test_algorithms.cc

lockshaw · 2023-07-10T12:12:02Z

lib/utils/test/src/test_algorithms.cc

  }
+//   std::cout<<"********"<<std::endl;


Please review future PRs for random commented-out code

You removed this code, but the next 4 lines are also commented out?

lockshaw · 2023-07-10T12:12:18Z

lib/utils/test/src/test_algorithms.cc


-  std::vector<Node> ordering = bfs_ordering(g, {n[0]});
+  for(DirectedEdge edge: edges) {


Suggested change

for(DirectedEdge edge: edges) {

for(DirectedEdge const &edge: edges) {

lockshaw

In general, be more thorough about making sure you are more aware of existing code. You've got a few places where you made a fix I requested, but only to one out of multiple places, or you implement a method of which multiple analogues exist but you don't match how they're implemented (not that this is always wrong, but it is usually). Before immediately jumping into and implementing things, use grep or some fuzzy search of the code to check if something similar has already been done.

Also, I recommend looking at the existing code and trying to match its style. Generally we prefer to draw from containers.h when possible rather than writing a bunch of raw loops, which are harder to maintain and easier to mess up. Also, refs in loops, etc.

And please run ./scripts/format.sh on your code before requesting reviews 🙂

lockshaw

Next time ideally try to address more of the requested changes before requesting another PR--I do appreciate going for quick feedback, but it takes a while to review a whole PR when you've only changed a couple things. If you have questions on individual comments, instead just leave a reply there. I've closed all of the comments that have actually been resolved, re-request review when you think you've addressed most or all of the remaining comments, and feel free to ask questions in the replys to the comments--this is the easiest way to get clarification on a single comment

lockshaw · 2023-07-11T12:36:42Z

lib/utils/include/utils/graph/adjacency_digraph.h

@@ -27,14 +26,13 @@ class AdjacencyDiGraph : public IDiGraph {
    return new AdjacencyDiGraph(this->next_node_idx, this->adjacency);
  }

+  AdjacencyDiGraph() = default;


Generally we put the constructors at the top of the class

lockshaw · 2023-07-11T12:38:25Z

lib/utils/include/utils/graph/adjacency_multidigraph.h

-      }
-
-private:
+  AdjacencyMultiDiGraph(std::size_t next_node_idx,  ContentsType const & adjacency, std::size_t next_node_port=0)


Suggested change

AdjacencyMultiDiGraph(std::size_t next_node_idx, ContentsType const & adjacency, std::size_t next_node_port=0)

AdjacencyMultiDiGraph(std::size_t next_node_idx, std::size_t next_node_port_idx, ContentsType const & adjacency)

lockshaw · 2023-07-11T12:44:05Z

lib/utils/src/graph/algorithms.cc

@@ -437,6 +501,36 @@ optional<Node> imm_post_dominator(MultiDiGraphView const &g,
  return get_imm_post_dominators(g).at(n);
 }

+tl::optional<Node> get_imm_post_dominator(DiGraphView const & g, Node const & n) {


Okay, but tl::optional is still used in the code three lines down--I generally don't leave a comment on every occurence of an issue (as it just gets repetitive), so if I leave a comment please check for any other code with that issue and fix it as well

lockshaw · 2023-07-11T12:46:47Z

lib/utils/include/utils/graph/digraph.h

@@ -14,6 +14,9 @@ struct DirectedEdge {
  Node src;
  Node dst;
 };
+
+std::ostream &operator<<(std::ostream &s, DirectedEdge const &e);


Replace with fmt

lockshaw · 2023-07-11T12:49:13Z

lib/utils/src/graph/algorithms.cc

  return get_imm_post_dominators(g).at(n);
 }


 tl::optional<Node> get_imm_post_dominator(DiGraphView const & g, std::unordered_set<Node> const & nodes ){
-    std::unordered_set<Node> commonDoms = get_post_dominators(g).at(*nodes.begin());
+    std::unordered_set<Node> commonDoms, currDoms, intersec;


Generally don't forward declare, it's tends to be confusing

lockshaw · 2023-07-11T12:54:13Z

lib/utils/src/graph/digraph.cc

@@ -70,10 +69,16 @@ std::unordered_set<DirectedEdge> DiGraphView::query_edges(EdgeQuery const & quer
 DiGraphView unsafe_create(IDiGraphView const &graphView) {
  std::shared_ptr<IDiGraphView const> ptr((&graphView),
  [](IDiGraphView const *){});
+  /*


Suggested change

/*

// Set the shared_ptr's destructor to a nop so that effectively there is no ownership

Also move above the line of code, not below

lockshaw · 2023-07-11T12:57:51Z

lib/utils/include/utils/graph/algorithms.h

@@ -16,6 +16,7 @@
 namespace FlexFlow {

 std::vector<Node> add_nodes(Graph &, int);
+std::vector<Node> add_nodes(IGraph &g, int num_nodes);


Why was this added?

lockshaw · 2023-07-11T12:59:19Z

lib/utils/test/src/test_adjacency_multidigraph.cc

+
+TEST_CASE("AdjacencyMultiDiGraph:remove_node") {
+  AdjacencyMultiDiGraph g;


Ideally don't repeat this code over and over again. Either do it as part of the same test or use subcases

lockshaw · 2023-07-11T12:59:42Z

lib/utils/test/src/test_algorithms.cc

-  NodePort p1 = g.add_node_port();
-  NodePort p2 = g.add_node_port();
-  NodePort p3 = g.add_node_port(); 
+  NodePort p0{0};


lockshaw · 2023-07-11T13:01:25Z

lib/utils/test/src/test_algorithms.cc

  }
+//   std::cout<<"********"<<std::endl;


You removed this code, but the next 4 lines are also commented out?

lambda7xx · 2023-08-05T01:38:25Z

lib/utils/include/utils/graph/multidigraph.h line 58 at r20 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Yes

if we delete, how to add many nodeports?

lambda7xx · 2023-08-05T01:38:45Z

lib/utils/include/utils/graph/multidigraph.h line 62 at r20 (raw file):

Previously, lockshaw (Colin Unger) wrote…

MultiDiGraph::add_edges should be deleted

if we delete, how to add many edges?

lambda7xx · 2023-08-05T01:40:34Z

lib/utils/include/utils/graph/node.h line 25 at r29 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why was this added?

remove

lambda7xx · 2023-08-05T01:40:48Z

lib/utils/include/utils/graph/node.h line 27 at r20 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why couldn't you do the following:

template <typename T>
typename std::enable_if<is_streamable<T>::value, std::ostream &>::type
operator<<(std::ostream &os, std::unordered_set<T> const &s) {
   ...
}

still has some problem.

lambda7xx · 2023-08-05T01:46:17Z

lib/utils/include/utils/graph/query_set.h line 85 at r10 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Would it not be possible to use query_values here instead of query_keys?

currently it only use query_values

lambda7xx · 2023-08-05T01:50:32Z

lib/utils/src/graph/algorithms.cc line 11 at r21 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Yes, you need to add an implicit conversion from DiGraph to Graph on DiGraph

I convert DiGraph to Graph, but we still has some problem use add_nodes(Graph &, int)

Code snippet:

  DiGraph g = DiGraph::create<AdjacencyDiGraph>();

  std::vector<Node> n = add_nodes(g, 4);
  
  //we want to add nodes for this DiGraph, but :45:35: error: cannot bind non-const lvalue reference of type ‘FlexFlow::Graph&’ to an rvalue of type ‘FlexFlow::Graph’
   //std::vector<Node> n = add_nodes(g, 4);
   //if we convert DiGraph g to Graph g, then we have to implement the Graph::add_node
   
   Node Graph::add_node(){
      return this->ptr.get_mutable()->add_node();
     }
     
     //but ptr is a IGraph, IGraph is a pure class
                                   ^

lambda7xx · 2023-08-05T01:53:52Z

lib/utils/src/graph/algorithms.cc line 623 at r21 (raw file):

Previously, lockshaw (Colin Unger) wrote…

What are you confused about?

I am confused your code because currently it makes me hard to read your code. maybe you could paste your full implementation here.

lambda7xx · 2023-08-05T01:55:34Z

lib/utils/src/graph/digraph.cc line 77 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

I still don't see the change

how to assert DirectedEdgeQuery

Code snippet:

struct DirectedEdgeQuery {
  query_set<Node> srcs;
  query_set<Node> dsts;

  static DirectedEdgeQuery all();
};

//use assert(!is_matchall(q.srcs); 
// assert(!is_matchall(q.dsts)

lambda7xx · 2023-08-05T01:55:55Z

lib/utils/src/graph/multidigraph.cc line 152 at r21 (raw file):

Previously, lockshaw (Colin Unger) wrote…

My suggestion is to delete both MultiDiGraph::add_nodes and MultiDiGraph::add_node_ports

I reply in other channels

lambda7xx · 2023-08-05T01:58:28Z

lib/utils/src/graph/multidigraph.cc line 178 at r21 (raw file):

Previously, lockshaw (Colin Unger) wrote…

The suggestion is to delete MultiDiGraph::add_edges

if we delete it, I guess you want to use a function

Code snippet:

void add_edges(Graph &, std::vector<DirectedEdge> const & edges) 

//first, the edge in DiGraph, MultiDiGraph is different, one is DirectedEdge, another is MultiDiEdge

//second, we will meet problmes like add_nodes(Graph &, size_t). I reply in other channel.

lambda7xx

Reviewable status: 20 of 36 files reviewed, 52 unresolved discussions (waiting on @lockshaw)

lib/utils/include/utils/graph/digraph.h line 63 at r21 (raw file):

Previously, lockshaw (Colin Unger) wrote…

In that case it should probably be removed

Done.

lib/utils/include/utils/graph/node.h line 57 at r29 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Move into a new file called utils/internal_only_tag.h

Done.

lib/utils/src/graph/algorithms.cc line 509 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

I would do the following

// in containers.h
template <typename T>
std::unordered_set<T> intersection(std::vector<std::unordered_set<T>> const &v) {
  if (v.empty()) {
    throw mk_runtime_error("cannot take intersection of no sets");
  }
  
  std::unordered_set<T> result = v[0];
  for (int i = 1; i < v.size(); i++) {
    result = intersection(result, v);
  }
  return result;
}

// here
std::unordered_set<Node> commonDoms = intersection(values(restrict_keys(get_post_dominators(g), nodes)));

return ...; // see next comment

Done.

lib/utils/src/graph/node.cc line 21 at r21 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Yes, but these should be defined using fmt, not operator<<

I use your sugg

lambda7xx · 2023-08-05T02:24:33Z

lib/utils/test/src/test_algorithms.cc line 16 at r10 (raw file):

Previously, lockshaw (Colin Unger) wrote…

MultiDiGraph::add_nodes and MultiDiGraph::add_edges should not exist

I reply in other channel

lambda7xx · 2023-08-05T02:26:51Z

lib/utils/test/src/test_algorithms.cc line 17 at r29 (raw file):

  MultiDiGraph g = MultiDiGraph::create<AdjacencyMultiDiGraph>();
  std::vector<Node> n = g.add_nodes(4);
  std::vector<NodePort> p = g.add_node_ports(4);

if we have a function add_node_ports, it definition is like?

Code snippet:

std::vector<NodePort> add_node_ports(DiGraph &, size_t);

std::vector<NodePort> add_node_ports(MultiDiGraph &, size_t);

lambda7xx · 2023-08-05T02:35:33Z

lib/utils/test/src/test_algorithms.cc line 102 at r10 (raw file):

Previously, lockshaw (Colin Unger) wrote…

No, I mean you deleted the subcase for get_neighbors and now it's untested. You should add that subcase back in

still have some bug. I am debuging.

lambda7xx · 2023-08-05T02:37:05Z

lib/utils/test/src/test_algorithms.cc line 163 at r10 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Yeah, I agree there is a bug in is_acyclic, but I don't think modifying unchecked_dfs_iterator is the right approach. I think the correct approach would be to allow the is_acyclic to access the stack in unchecked_dfs_iterator and use that rather than seen

I modify the unecked_dfs_iterator. but the time complexity is o(n) because std::find, I try to optimize it.

Code snippet:

udi &udi::operator++() {
  Node const last = this->operator*();
  this->stack.pop_back();

  std::unordered_set<DirectedEdge> outgoing = get_outgoing_edges(graph, {last});
  for (DirectedEdge const &e : outgoing) {
    auto it = std::find(stack.begin(), stack.end(), e.dst);
    if (it == stack.end()) {
      stack.push_back(e.dst);
    } else {
      stack.erase(it);
      stack.push_back(e.dst);
    }
  }
  return *this;
}

lambda7xx · 2023-08-05T02:37:29Z

lib/utils/include/utils/fmt.h line 17 at r20 (raw file):

Previously, lockshaw (Colin Unger) wrote…

See https://reviewable.io/reviews/flexflow/FlexFlow/842#-Na9fFQc81sTLIR7dUpD

still struggle to implement this.

lambda7xx · 2023-08-08T02:50:39Z

lib/utils/test/src/test_adjacency_multidigraph.cc line 43 at r21 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Yes, but query_set<Node> can be implicitly constructed from a Node, so this should work

this shouldn't work. query_set<Node> can't be implicitly constructed from a Node. Unless we add a constructor method for query_set.

query_set(T const & node):{
query.insert(node);
}

Code snippet:

template <typename T>
struct query_set {
  query_set() = delete;
  query_set(T const &query) : query({query}) {}

  query_set(std::unordered_set<T> const &query) : query(query) {}

  query_set(optional<std::unordered_set<T>> const &query) : query(query) {}

  friend bool operator==(query_set const &lhs, query_set const &rhs) {
    return lhs.value == rhs.value;
  }

  friend bool operator!=(query_set const &lhs, query_set const &rhs) {
    return lhs.value != rhs.value;
  }

  friend bool operator<(query_set const &lhs, query_set const &rhs) {
    return lhs.value < rhs.value;
  }

  friend bool is_matchall(query_set const &q) {
    return !q.query.has_value();
  }

  friend std::unordered_set<T> allowed_values(query_set const &q) {
    assert(!is_matchall(q));
    return q.query.value();
  }

  static query_set<T> matchall() {
    return {nullopt};
  }

private:
  optional<std::unordered_set<T>> query;
};

lambda7xx · 2023-08-08T03:15:32Z

lib/utils/src/graph/node.cc line 21 at r21 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

I use your sugg

I think there are still problems when we use fmt not operator<<

lambda7xx · 2023-08-08T03:15:41Z

lib/utils/src/graph/node.cc line 13 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

so how to check the NodeQuery ?

fix it

lambda7xx

Reviewable status: 19 of 36 files reviewed, 52 unresolved discussions (waiting on @lockshaw)

lib/utils/include/utils/graph/digraph.h line 74 at r29 (raw file):

  }

  static DiGraphView unsafe_create(IDiGraphView const &graphView);

Done.

lib/utils/test/src/test_algorithms.cc line 49 at r21 (raw file):

Previously, lockshaw (Colin Unger) wrote…

I still don't see it: https://github.com/lambda7xx/FlexFlow/blob/2a8f8a6bbc7e75c4c9b1396bbd79ef64e48039ff/lib/utils/test/src/test_algorithms.cc#L41

Done.

lib/utils/test/src/test_algorithms.cc line 26 at r29 (raw file):

  };

  g.add_edges(e);

Done.

lockshaw · 2023-08-09T22:50:05Z

Moving to #940

lambda7xx added 3 commits July 6, 2023 11:21

fix the inplace_sorted_by in container.h

0fc19a0

start to implement the contract revelant

cbb6d12

finish other method except unsafe related

c20e1a6

lambda7xx added the repo-refactor Topics related to the repo and search refactors label Jul 7, 2023

lambda7xx added 3 commits July 7, 2023 12:02

add implement for the unsafe_create

b9ba680

add test for algorithm

881ecee

add test for algorithm

1525999

lockshaw requested changes Jul 10, 2023

View reviewed changes

lambda7xx added 9 commits July 10, 2023 13:46

refine the code according to the PR

b138d36

refine the PR

7368510

fix the doctest

666cf82

refine the code

2ac1e11

fix the bug

2b82bfc

modify the get_imm_post_dominator

1ccf5c4

fix the lib/CMakeLists.txt

e83b24c

refine the get_weakly_connected_components

375053e

modfiy the lib/CMakeLists.txt

1c8395b

lockshaw self-requested a review July 11, 2023 12:30

lockshaw requested changes Jul 11, 2023

View reviewed changes

refine the utils

f2ef339

lambda7xx requested a review from lockshaw July 13, 2023 11:26

lambda7xx added 8 commits July 13, 2023 12:52

fix the get_mutable

380d1df

implement the add_nodes(DiGraph)

20e6835

use DiGraph::create<AdjacencyDiGraph>() to replacee AdjacncyDiGraph

dad3729

use DiGraphView to implement the DiGraph::query_nodes

19b8df1

use MultiDiGraph in test

179d7c3

remove std::hash for JoinNodeKey

9a372af

use the fmt

8b73684

use the optional to replace tl::optional in algorithm.cc

79237b1

refine the node

9f0c44e

lambda7xx commented Aug 5, 2023

View reviewed changes

refine the query_intersection

f1b06d9

refine the query_intersection

efe5b84

refine the glgorith,s

3429a78

lambda7xx commented Aug 8, 2023

View reviewed changes

lockshaw changed the title ~~Add code for the repo-refactor utils~~ Add unit tests for lib/utils/graph Aug 8, 2023

lockshaw assigned lambda7xx Aug 8, 2023

lambda7xx requested a review from lockshaw August 8, 2023 11:57

lockshaw mentioned this pull request Aug 9, 2023

Add unit testing to utils #940

Merged

1 task

lockshaw closed this Aug 9, 2023


		std::vector<Node> ordering = bfs_ordering(g, {n[0]});
		for(DirectedEdge edge: edges) {

	for(DirectedEdge edge: edges) {
	for(DirectedEdge const &edge: edges) {

	AdjacencyMultiDiGraph(std::size_t next_node_idx, ContentsType const & adjacency, std::size_t next_node_port=0)
	AdjacencyMultiDiGraph(std::size_t next_node_idx, std::size_t next_node_port_idx, ContentsType const & adjacency)

	/*
	// Set the shared_ptr's destructor to a nop so that effectively there is no ownership


		TEST_CASE("AdjacencyMultiDiGraph:remove_node") {
		AdjacencyMultiDiGraph g;

Add unit tests for lib/utils/graph #842

Add unit tests for lib/utils/graph #842

Conversation

lambda7xx commented Jul 7, 2023 • edited by lockshaw Loading

lockshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lockshaw left a comment

Choose a reason for hiding this comment

lockshaw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx left a comment

Choose a reason for hiding this comment

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 5, 2023

lambda7xx commented Aug 8, 2023

lambda7xx commented Aug 8, 2023

lambda7xx commented Aug 8, 2023

lambda7xx left a comment

Choose a reason for hiding this comment

lockshaw commented Aug 9, 2023

Add unit tests for `lib/utils/graph` #842

Add unit tests for `lib/utils/graph` #842

lambda7xx commented Jul 7, 2023 •

edited by lockshaw

Loading

lockshaw left a comment •

edited

Loading