Finish implementing compiler

[wmdi]

Description of changes:

• finish machine mapping (dp algorithm)
• finish cost estimator
• finish unity search

Related Issues:

Linked Issues:

• Issue #

Issues closed by this PR:

• Closes Add compiler unit tests to CI #1280 Add substitutions unit tests to CI #1279

---

This change is 

[wmdi]

Reviewed 26 of 76 files at r3, 123 of 123 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lambda7xx and @lockshaw)

[wmdi]

Reviewed 86 of 86 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lambda7xx and @lockshaw)

[wmdi]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lambda7xx and @lockshaw)

[wmdi]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lambda7xx and @lockshaw)

[lockshaw]

Reviewed 5 of 76 files at r3, 57 of 123 files at r4, 85 of 86 files at r5, 2 of 2 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 80 unresolved discussions (waiting on @lambda7xx and @wmdi)

---

lib/compiler/include/compiler/machine_mapping.h line 27 at r7 (raw file):

  SerialParallelDecomposition subgraph;
  req<MachineSpecification> resource;
  // req<std::unordered_map<Node, MachineView>> given_machine_views;

What are these for? Why are they commented out?

---

lib/compiler/include/compiler/unity_algorithm.h line 39 at r7 (raw file):

} // namespace FlexFlow

VISITABLE_STRUCT(FlexFlow::Strategy, pcg, machine_mapping, runtime);

Why here instead of using FF_VISITABLE_STRUCT?

---

lib/compiler/src/graph_utils.cc line 15 at r7 (raw file):

SubParallelComputationGraph pcg_to_subpcg(ParallelComputationGraph const &pcg) {
  return materialize_output_labelled_open_multidigraph_view<

I don't think doing this should be this complicated. If it is we probably need to make some changes to the graph library. Also, why is explicit materialization required here? Viewing sub-PCGs is the whole point of the view abstraction

---

lib/compiler/src/machine_mapping.cc line 0 at r7 (raw file):
Overall the readability of the algorithm here (while much better than the old version) still needs a bit of work. The key thing is all the argument passing obscures what's actually going on

---

lib/compiler/src/machine_mapping.cc line 48 at r7 (raw file):

optional<OptimalCostResult>
    OptimalCostCache::load(OptimalCostState const &state) const {
  auto it = cache.find(state);

This looks unfinished?

---

lib/compiler/src/machine_mapping.cc line 99 at r7 (raw file):

                    std::unordered_map<OpenMultiDiEdge, MachineView> const
                        &frontier_machine_views) {
  return 0.1;

This hardcoding the cost should probably be moved to the specific implementation of ICostEstimator that backs estimator, instead of here

---

lib/compiler/src/machine_mapping.cc line 137 at r7 (raw file):

  OptimalCostResult operator()(T const &t) const {
    OptimalCostState state{
        t, resource /*, given_machine_views, frontier_machine_views*/};

Remove commented-out arguments?

---

lib/compiler/src/machine_mapping.cc line 157 at r7 (raw file):

    GraphSplit graph_split = get_graph_split(pre_decompn, post_decompn);
    SubParallelComputationGraphView pre_graph =
        get_subgraph<OpenMultiDiSubgraphView>(g, graph_split.first);

Why are explicit template parameters necessary here?

---

lib/compiler/src/machine_mapping.cc line 193 at r7 (raw file):

                                             resource,
                                             new_given_machine_views,
                                             frontier_machine_views,

Instead of using a single implementation to do both unity and the newer-memory-optimization algorithms, I'd rather have two different algorithm implementations and factor out the parts in common where necessary/useful. Otherwise this is quickly going to grow unmaintainably large

---

lib/compiler/src/machine_mapping.cc line 215 at r7 (raw file):

    OptimalCostResult optimal_result = OptimalCostResult::sequential_combine(
        visit(OptimalCost(g1,
                          cost_estimator,

I appreciate the stateless design here (i.e., passing all of the algorithm state in the form of resource, allowed_machine_views, cached_subgraph_costs, etc. but here I think it's making things hard to read/understand. Can we separate the implementation here into a stateless layer (where most of the individual code is) and then add a stateful layer on top for readability?

Also, since this is all stateless I'm a little confused why there is a class here (i.e., OptimalCost)--if this is just to handle visit let's move the visit invocation into a separate function so the details of how all variant dispatch are separate from the actual algorithm.

---

lib/compiler/src/unity_algorithm.cc line 13 at r7 (raw file):

std::unordered_set<Substitution>
    get_all_substitutions(ParallelComputationGraph const &pcg) {

What is this function intended to do? Is it to get all applicable substitutions? Or if not why does it require a ParallelComputationGraph as input?

Especially for these NOT_IMPLEMENTED functions a quick docstring would be quite helpful--we have doxygen support 🙂

---

lib/compiler/src/unity_algorithm.cc line 46 at r7 (raw file):

                                                      cached_subgraph_costs);
  Strategy initial_result{
      pcg, initial_pcg_result.machine_mapping, initial_pcg_result.runtime};

Why does optimal_cost not just return a Strategy directly, or why does an API that just does that translation not exist?

---

lib/compiler/src/unity_algorithm.cc line 51 at r7 (raw file):

  candidates.push(initial_result);

  for (int iteration = 0; !candidates.empty() && iteration < opt_config.budget;

Refactoring needs to be done here to reduce the amount of nesting--if we're hitting tripluy-nested for loops something has gone wrong. Overall the functions in this PR could do with some dividing-up.

---

lib/compiler/src/unity_algorithm.cc line 56 at r7 (raw file):

    candidates.pop();

    if (StrategyRuntimeCmp{}(current_result, best_result)) {

Just compare on runtime directly unless there's some abstraction that's being enabled here

---

lib/compiler/src/unity_algorithm.cc line 59 at r7 (raw file):

      best_result = current_result;
    } else if (current_result.runtime >
               best_result.runtime * opt_config.alpha) {

Would it be possible to pull the MCMC part of this out into a separate MCMC function that does the acceptance/rejection logic?

---

lib/compiler/src/unity_algorithm.cc line 86 at r7 (raw file):

namespace std {

size_t hash<FlexFlow::Strategy>::operator()(FlexFlow::Strategy const &s) const {

Why not use visitable for the hash function?

---

lib/compiler/src/old/basic_graph.h line 0 at r7 (raw file):
If we're not using anything in old/ let's just remove it?

---

lib/compiler/test/CMakeLists.txt line 5 at r7 (raw file):

    compiler-test
  SRC_PATTERNS
    src/test_labelled_open_graph.cc

Why is explicitly listing the files here necessary?

---

lib/compiler/test/src/test_labelled_open_graph.cc line 7 at r7 (raw file):

using namespace FlexFlow;

TEST_CASE("get_subgraph_open_graph") {

Suggestion:

TEST_CASE("get_subgraph(OpenMultiDiGraphView)") {

---

lib/compiler/test/src/test_labelled_open_graph.cc line 59 at r7 (raw file):

  std::unordered_set<OutputMultiDiEdge> output_set{e5};

  CHECK(bool(get_open_inputs(subgraph0) == input_set));

Why is there a bool here?

---

lib/compiler/test/src/test_open_graph.cc line 10 at r7 (raw file):

  OpenMultiDiGraph g = OpenMultiDiGraph::create<AdjacencyOpenMultiDiGraph>();

  int s0 = 100000;

What is s0 here for?

---

lib/compiler/test/src/test_open_graph.cc line 28 at r7 (raw file):

  OpenMultiDiGraph g = OpenMultiDiGraph::create<AdjacencyOpenMultiDiGraph>();

  int s0 = 100000;

What are s0 and t0 here for?

---

lib/compiler/test/src/test_open_graph.cc line 59 at r7 (raw file):

  std::vector<Node> ns;
  for (int i = 0; i < 5; ++i) {
    ns.push_back(g.add_node());

Use add_nodes?

---

lib/compiler/test/src/test_open_graph.cc line 66 at r7 (raw file):

  MultiDiEdge e2{ns[3], g.add_node_port(), ns[1], g.add_node_port()};
  MultiDiEdge e3{ns[4], g.add_node_port(), ns[2], g.add_node_port()};
  MultiDiEdge e4{ns[4], g.add_node_port(), ns[3], g.add_node_port()};

Eventually it would be good to add infrastructure to include dot descriptions of these graphs in these testcases for readability accompanied with checks to make sure they don't get out of date. It might even be worth writing a quick parser and DSL for specifying them as otherwise it's hard to tell what these testcases are actually doing (at least for people who didn't write the testcases themselves 🙂 )

---

lib/compiler/test/src/test_optimal_cost.cc line 11 at r7 (raw file):

allowed machine views, trivial cost estimator and random machine specification.
*/
// TEST_CASE("optimal_cost") {

Why is this commented out?

---

lib/op-attrs/src/get_output_shapes.cc line 11 at r7 (raw file):

    PCGOperatorAttrs const &op_params,
    std::vector<ParallelTensorShape> const &input_tensor_shapes) {
  NOT_IMPLEMENTED();

Ideally add a brief docstring (or even better a couple unit tests) specifying the intended functionality of these NOT_IMPLEMENTED functions

---

lib/pcg/include/pcg/machine_view.h line 16 at r7 (raw file):

struct MachineView {
  // MachineView() = delete;

Why not remove?

---

lib/pcg/include/pcg/operator.h line 20 at r7 (raw file):

public:
  PCGOperatorAttrs attrs;
  optional<std::string> name;

Since we're now on C++17 let's start changing to use std::optional instead of optional. In the long term I'll probably remove the current alias

---

lib/pcg/include/pcg/parallel_computation_graph.h line 18 at r7 (raw file):

CHECK_WELL_BEHAVED_VALUE_TYPE_NO_HASH(ParallelComputationGraph);

bool operator==(ParallelComputationGraph const &,

Is this not done by strong_typedef?

---

lib/pcg/include/pcg/parallel_computation_graph.h line 23 at r7 (raw file):

} // namespace FlexFlow

namespace std {

MAKE_TYPEDEF_HASHABLE?

---

lib/pcg/include/pcg/strided_rectangle.h line 44 at r7 (raw file):

struct StridedRectangle {
public:
  // StridedRectangle() = delete;

Remove?

---

lib/pcg/include/pcg/strided_rectangle.h line 65 at r7 (raw file):

MAKE_TYPEDEF_PRINTABLE(::FlexFlow::side_size_t, "side_size");

// VISITABLE_STRUCT(::FlexFlow::StridedRectangleSide, num_points, stride);

Remove?

---

lib/pcg/src/machine_view.cc line 6 at r7 (raw file):

namespace FlexFlow {

// MachineView::MachineView(device_id_t const &start, StridedRectangle const

Remove?

---

lib/pcg/src/operator.cc line 5 at r7 (raw file):

namespace FlexFlow {

Operator::Operator(PCGOperatorAttrs const &attrs,

Isn't this provided by visitable?

---

lib/pcg/src/parallel_computation_graph.cc line 6 at r7 (raw file):

namespace FlexFlow {

bool operator==(ParallelComputationGraph const &lhs,

Can't you have hash collisions? If this isn't actually a true == operator it shouldn't just be provided as the default == operator

---

lib/pcg/src/parallel_computation_graph.cc line 17 at r7 (raw file):

size_t hash<FlexFlow::ParallelComputationGraph>::operator()(
    FlexFlow::ParallelComputationGraph const &g) const {

It might be cleaner to pull this out into a separate ParallelComputationGraphHashKey object which is visitable rather then using hash_combine which is harder to read and easier to make mistakes in

---

lib/pcg/src/strided_rectangle.cc line 33 at r7 (raw file):

}

// StridedRectangle::StridedRectangle(

Remove?

---

lib/utils/include/utils/graph/algorithms.h line 110 at r7 (raw file):

std::unordered_set<OutputMultiDiEdge>
    get_open_outputs(OpenMultiDiGraphView const &);

Why not just get_outputs and get_inputs?

---

lib/utils/include/utils/graph/labelled/node_labelled.h line 57 at r7 (raw file):

private:
  Interface const &get_ptr() const {
    return *std::dynamic_pointer_cast<Interface const>(GraphView::ptr.get());

dynamic_casts shouldn't be necessary here--there's no needed runtime checking so reinterpret should be fine

---

lib/utils/include/utils/graph/labelled/node_labelled_open.h line 58 at r7 (raw file):

private:
  Interface const &get_ptr() const {
    return *std::dynamic_pointer_cast<Interface const>(GraphView::ptr.get());

dynamic_cast shouldn't be necessary here

---

lib/utils/include/utils/graph/labelled/open_views.h line 55 at r7 (raw file):

};

// CHECK_NOT_ABSTRACT(OutputLabelledOpenMultiDiSubgraphView);

Why comment this out? Either remove or uncomment

---

lib/utils/include/utils/graph/labelled/output_labelled.h line 34 at r7 (raw file):

      operator=(OutputLabelledMultiDiGraphView const &) = default;

  virtual NodeLabel const &at(Node const &n) const {

Non I classes should not have virtual methods

---

lib/utils/include/utils/graph/labelled/output_labelled_open.h line 66 at r7 (raw file):

private:
  Interface const &get_ptr() const {
    return *std::dynamic_pointer_cast<Interface const>(GraphView::ptr.get());

dynamic_cast not necessary

---

lib/utils/include/utils/graph/labelled/output_labelled_open.h line 94 at r7 (raw file):

  void add_node_unsafe(Node const &n, NodeLabel const &l) {
    get_ptr().add_node_unsafe(n);
    nl.get_mutable()->add_label(n, l);

Suggestion:

    this->get_ptr().add_node_unsafe(n);
    this->node_labelling.get_mutable()->add_label(n, l);

---

lib/utils/include/utils/graph/labelled/output_labelled_open.h line 176 at r7 (raw file):

  Interface &get_ptr() {
    return *std::dynamic_pointer_cast<Interface>(GraphView::ptr.get_mutable());

No dynamic_cast

---

lib/utils/include/utils/graph/labelled/output_labelled_open.h line 185 at r7 (raw file):

  cow_ptr_t<INodeLabel> nl;
  cow_ptr_t<IInputLabel> il;
  cow_ptr_t<IOutputLabel> ol;

Suggestion:

  cow_ptr_t<INodeLabel> node_labelling;
  cow_ptr_t<IInputLabel> input_labelling;
  cow_ptr_t<IOutputLabel> output_labelling;

---

lib/utils/include/utils/graph/labelled/standard_labelled.h line 63 at r7 (raw file):

  Interface const &get_ptr() const {
    return *std::dynamic_pointer_cast<Interface const>(GraphView::ptr.get());

reinterpret

---

lib/utils/include/utils/graph/labelled/standard_labelled.h line 134 at r7 (raw file):

  Interface &get_ptr() {
    return *std::dynamic_pointer_cast<Interface>(GraphView::ptr.get_mutable());

reinterpret

---

lib/utils/include/utils/graph/labelled/standard_labelled.h line 138 at r7 (raw file):

  Interface const &get_ptr() const {
    return *std::dynamic_pointer_cast<Interface const>(GraphView::ptr.get());

reinterpret

---

lib/utils/include/utils/graph/labelled/unordered_label.h line 22 at r7 (raw file):

  void add_label(Elem const &e, Label const &l) {
    auto p = std::make_pair(e, l);

Why the extra line?

---

lib/utils/include/utils/graph/labelled/views.h line 99 at r7 (raw file):

          typename OutputLabel>
OutputLabelledOpenMultiDiGraph<NodeLabel, OutputLabel>
    materialize_output_labelled_open_multidigraph_view(

Can this not just be an overload of materialize?

---

lib/utils/include/utils/graph/labelled/views.h line 116 at r7 (raw file):

      result.add_label(input_edge, g.at(input_edge));
    } else {
      MultiDiOutput output =

The planned edge inheritance will fix this, right?

---

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

    Node from = kv.first;
    Node into = kv.second;
    if (from != into) {

Should this even be allowed? Why should this not raise an exception or fail an assertion?

---

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

std::unordered_set<OutputMultiDiEdge>
    get_open_outputs(OpenMultiDiGraphView const &g) {
  return transform(

Add this to variant.h along the lines of https://github.com/flexflow/FlexFlow/blob/0c45f61f114414215c5eff7d39006f07735e2fe7/lib/utils/include/utils/variant.h#L190-L200

---

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

std::unordered_set<Node> get_open_sources(OpenMultiDiGraphView const &g) {
  return filter(get_nodes(g), [&](Node const &n) {
    return !g.query_edges(InputMultiDiEdgeQuery::all().with_dst_nodes({n}))

get_incoming_edges?

---

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

std::unordered_set<Node> get_open_sinks(OpenMultiDiGraphView const &g) {
  return filter(get_nodes(g), [&](Node const &n) {
    return !g.query_edges(OutputMultiDiEdgeQuery::all().with_src_nodes({n}))

get_outgoing_edges?

---

lib/utils/src/graph/digraph.cc line 17 at r7 (raw file):

IDiGraphView const &DiGraphView::get_ptr() const {
  return *std::dynamic_pointer_cast<IDiGraphView const>(GraphView::ptr.get());

reinterpret

---

lib/utils/src/graph/digraph.cc line 50 at r7 (raw file):

IDiGraph &DiGraph::get_ptr() {
  return *std::dynamic_pointer_cast<IDiGraph>(GraphView::ptr.get_mutable());

reinterpet

---

lib/utils/src/graph/digraph.cc line 54 at r7 (raw file):

IDiGraph const &DiGraph::get_ptr() const {
  return *std::dynamic_pointer_cast<IDiGraph const>(

reinterpet

---

lib/utils/src/graph/multidigraph.cc line 27 at r7 (raw file):

IMultiDiGraphView const &MultiDiGraphView::get_ptr() const {
  return *std::dynamic_pointer_cast<IMultiDiGraphView const>(

reinterpet

---

lib/utils/src/graph/multidigraph.cc line 69 at r7 (raw file):

IMultiDiGraph const &MultiDiGraph::get_ptr() const {
  return *std::dynamic_pointer_cast<IMultiDiGraph const>(GraphView::ptr.get());

reinterpet

---

lib/utils/src/graph/multidigraph.cc line 73 at r7 (raw file):

IMultiDiGraph &MultiDiGraph::get_ptr() {
  return *std::dynamic_pointer_cast<IMultiDiGraph>(

reinterpet

---

lib/utils/src/graph/node.cc line 56 at r7 (raw file):

IGraph const &Graph::get_ptr() const {
  return *std::dynamic_pointer_cast<IGraph const>(GraphView::ptr.get());

reinterpet

---

lib/utils/src/graph/node.cc line 60 at r7 (raw file):

IGraph &Graph::get_ptr() {
  return *std::dynamic_pointer_cast<IGraph>(GraphView::ptr.get_mutable());

reinterpet

---

lib/utils/src/graph/open_graphs.cc line 25 at r7 (raw file):

IOpenMultiDiGraphView const &OpenMultiDiGraphView::get_ptr() const {
  return *std::dynamic_pointer_cast<IOpenMultiDiGraphView const>(

reinterpet

---

lib/utils/src/graph/open_graphs.cc line 55 at r7 (raw file):

NodePort OpenMultiDiGraph::add_node_port() {
  return get_ptr().add_node_port();

Suggestion:

  return this->get_ptr().add_node_port();

---

lib/utils/src/graph/open_graphs.cc line 59 at r7 (raw file):

IOpenMultiDiGraph &OpenMultiDiGraph::get_ptr() {
  return *std::dynamic_pointer_cast<IOpenMultiDiGraph>(

reinterpet

---

lib/utils/src/graph/open_graphs.cc line 80 at r7 (raw file):

IUpwardOpenMultiDiGraphView const &UpwardOpenMultiDiGraphView::get_ptr() const {
  return *std::dynamic_pointer_cast<IUpwardOpenMultiDiGraphView const>(

reinterpet

---

lib/utils/src/graph/open_graphs.cc line 110 at r7 (raw file):

IUpwardOpenMultiDiGraph const &UpwardOpenMultiDiGraph::get_ptr() const {
  return *std::dynamic_pointer_cast<IUpwardOpenMultiDiGraph const>(

reinterpet

---

lib/utils/src/graph/open_graphs.cc line 115 at r7 (raw file):

IUpwardOpenMultiDiGraph &UpwardOpenMultiDiGraph::get_ptr() {
  return *std::dynamic_pointer_cast<IUpwardOpenMultiDiGraph>(

reinterpet

---

lib/utils/src/graph/open_graphs.cc line 132 at r7 (raw file):

IDownwardOpenMultiDiGraphView const &
    DownwardOpenMultiDiGraphView::get_ptr() const {
  return *std::dynamic_pointer_cast<IDownwardOpenMultiDiGraphView const>(

reinterpet

---

lib/utils/src/graph/open_graphs.cc line 168 at r7 (raw file):

IDownwardOpenMultiDiGraph &DownwardOpenMultiDiGraph::get_ptr() {
  return *std::dynamic_pointer_cast<IDownwardOpenMultiDiGraph>(

reinterpet

---

lib/utils/src/graph/undirected.cc line 29 at r7 (raw file):

IUndirectedGraph const &UndirectedGraph::get_ptr() const {
  return *std::dynamic_pointer_cast<IUndirectedGraph const>(

reinterpet

---

lib/utils/src/graph/undirected.cc line 34 at r7 (raw file):

IUndirectedGraph &UndirectedGraph::get_ptr() {
  return *std::dynamic_pointer_cast<IUndirectedGraph>(

reinterpet

---

lib/utils/src/graph/undirected.cc line 59 at r7 (raw file):

IUndirectedGraphView const &UndirectedGraphView::get_ptr() const {
  return *std::dynamic_pointer_cast<IUndirectedGraphView const>(

reinterpet

---

lib/utils/src/graph/views.cc line 473 at r7 (raw file):

    OpenMultiDiGraphView const &g, std::unordered_set<Node> const &nodes)
    : g(g), nodes(nodes),
      inputs(transform(get_cut_set(g, nodes), to_inputmultidiedge)) {}

Doing this much logic in the member initializer list is not great--let's move this into the actual body of the constructor

---

lib/utils/src/graph/views.cc line 486 at r7 (raw file):

      OutputMultiDiEdgeQuery::none());
  std::unordered_set<OpenMultiDiEdge> result = g.query_edges(subgraph_query);
  extend(result, query_edge(inputs, q.input_edge_query.with_dst_nodes(nodes)));

Use set union instead of extend

---

lib/utils/src/graph/views.cc line 498 at r7 (raw file):

    OpenMultiDiGraphView const &g, std::unordered_set<Node> const &nodes)
    : g(g), nodes(nodes),
      outputs(transform(get_cut_set(g, nodes), to_outputmultidiedge)) {}

Move into constructor body

---

lib/utils/src/graph/views.cc line 508 at r7 (raw file):

      q.output_edge_query.with_src_nodes(nodes));
  std::unordered_set<OpenMultiDiEdge> result = g.query_edges(subgraph_query);
  extend(result,

Use set union instead of extend

---

deps/fmt line 1 at r4 (raw file):

Subproject commit a33701196adfad74917046096bf5a2aa0ab0bb50

Revert this change?

[wmdi]

Reviewable status: all files reviewed, 80 unresolved discussions (waiting on @lockshaw)

---

lib/compiler/include/compiler/machine_mapping.h line 27 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

What are these for? Why are they commented out?

This is related to an issue I forgot to report. We need these attributes, but FF_VISITABLE_STRUCT would fail with them.

---

lib/compiler/include/compiler/unity_algorithm.h line 39 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why here instead of using FF_VISITABLE_STRUCT?

I don't want automatically generated operator== for them.

---

lib/compiler/src/graph_utils.cc line 15 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

I don't think doing this should be this complicated. If it is we probably need to make some changes to the graph library. Also, why is explicit materialization required here? Viewing sub-PCGs is the whole point of the view abstraction

The template arguments make it look so but I don't think it too complicated. An explicit materialization is required here because we need a graph instead of a graph view, which is because we need to change the graph in the algorithm.

---

lib/compiler/src/machine_mapping.cc line 48 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

This looks unfinished?

Yes, due to the visitable issue of OptimalCostState.

---

lib/compiler/src/machine_mapping.cc line 99 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

This hardcoding the cost should probably be moved to the specific implementation of ICostEstimator that backs estimator, instead of here

Done.

---

lib/compiler/src/machine_mapping.cc line 137 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Remove commented-out arguments?

This should be fixed with the visitable issue related to OptimalCostState.

---

lib/compiler/src/machine_mapping.cc line 157 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why are explicit template parameters necessary here?

We use template parameters to specify the open type of the subgraph now. It should be unnecessary after we change the open subgraph algorithm.

---

lib/compiler/src/machine_mapping.cc line 193 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Instead of using a single implementation to do both unity and the newer-memory-optimization algorithms, I'd rather have two different algorithm implementations and factor out the parts in common where necessary/useful. Otherwise this is quickly going to grow unmaintainably large

I agree. Will keep it in mind when implementing the new algorithm.

---

lib/compiler/src/unity_algorithm.cc line 13 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

What is this function intended to do? Is it to get all applicable substitutions? Or if not why does it require a ParallelComputationGraph as input?

Especially for these NOT_IMPLEMENTED functions a quick docstring would be quite helpful--we have doxygen support 🙂

Done.

---

lib/compiler/src/unity_algorithm.cc line 46 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why does optimal_cost not just return a Strategy directly, or why does an API that just does that translation not exist?

I treat the PCG as the input of optimal_cost, which means optimal_cost searches for the best device mapping for a fixed PCG. On the other hand, the upper-level search graph_optimize searches for a PCG. Thus it makes more sense that PCG is not part of the output of optimal_cost.

---

lib/compiler/src/unity_algorithm.cc line 56 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Just compare on runtime directly unless there's some abstraction that's being enabled here

Done.

---

lib/compiler/src/unity_algorithm.cc line 86 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why not use visitable for the hash function?

The hash function for MachineMapping does not work and I use manual hash function to get around temporarily. Will get back to visitable after the visitable issues are solved.

---

lib/compiler/src/old/basic_graph.h line at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

If we're not using anything in old/ let's just remove it?

Done.

---

lib/compiler/test/src/test_labelled_open_graph.cc line 7 at r7 (raw file):

using namespace FlexFlow;

TEST_CASE("get_subgraph_open_graph") {

Done.

---

lib/compiler/test/src/test_labelled_open_graph.cc line 59 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why is there a bool here?

To get around the fmt issue.

---

lib/compiler/test/src/test_open_graph.cc line 10 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

What is s0 here for?

Done.

---

lib/compiler/test/src/test_open_graph.cc line 28 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

What are s0 and t0 here for?

Done.

---

lib/compiler/test/src/test_open_graph.cc line 59 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Use add_nodes?

Done.

---

lib/compiler/test/src/test_optimal_cost.cc line 11 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why is this commented out?

They use rapidcheck which does not work for now.

---

lib/op-attrs/src/get_output_shapes.cc line 11 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Ideally add a brief docstring (or even better a couple unit tests) specifying the intended functionality of these NOT_IMPLEMENTED functions

Done.

---

lib/pcg/include/pcg/machine_view.h line 16 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why not remove?

Done.

---

lib/pcg/include/pcg/operator.h line 20 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Since we're now on C++17 let's start changing to use std::optional instead of optional. In the long term I'll probably remove the current alias

Done.

---

lib/pcg/include/pcg/parallel_computation_graph.h line 18 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Is this not done by strong_typedef?

We don't have a hash function for OutputLabelledOpenMultiDiGraph (and I think we should not have one), so we cannot directly use MAKE_TYPEDEF_HASHABLE for ParallelComputationGraph and thus have a separate hash function. For this reason, since operator== should be consistent with the hash function, we have a separate operator== as well

---

lib/pcg/include/pcg/parallel_computation_graph.h line 23 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

MAKE_TYPEDEF_HASHABLE?

We don't have a hash function for OutputLabelledOpenMultiDiGraph (and I think we should not have one), so we cannot directly use MAKE_TYPEDEF_HASHABLE for ParallelComputationGraph and thus have a separate hash function.

---

lib/pcg/include/pcg/strided_rectangle.h line 44 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Remove?

Done.

---

lib/pcg/include/pcg/strided_rectangle.h line 65 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Remove?

Done.

---

lib/pcg/src/machine_view.cc line 6 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Remove?

Done.

---

lib/pcg/src/operator.cc line 5 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Isn't this provided by visitable?

Done.

---

lib/pcg/src/strided_rectangle.cc line 33 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Remove?

Done.

---

lib/utils/include/utils/graph/algorithms.h line 110 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why not just get_outputs and get_inputs?

I expect they have different semantics?

---

lib/utils/include/utils/graph/labelled/open_views.h line 55 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why comment this out? Either remove or uncomment

Done.

---

lib/utils/include/utils/graph/labelled/output_labelled_open.h line 185 at r7 (raw file):

  cow_ptr_t<INodeLabel> nl;
  cow_ptr_t<IInputLabel> il;
  cow_ptr_t<IOutputLabel> ol;

Done.

---

lib/utils/include/utils/graph/labelled/unordered_label.h line 22 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why the extra line?

Done.

---

lib/utils/include/utils/graph/labelled/views.h line 116 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

The planned edge inheritance will fix this, right?

I don't think so. Although MultiDiEdge and OutputMultiDiEdge can be converted to MultiDiOutput, variant<MultiDiEdge, OutputMultiDiEdge, InputMultiDiEdge> cannot.

---

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

Previously, lockshaw (Colin Unger) wrote…

Should this even be allowed? Why should this not raise an exception or fail an assertion?

Let's allow this to make it as general as possible. If we need such a constraint, we should write another check function.

---

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

Previously, lockshaw (Colin Unger) wrote…

Add this to variant.h along the lines of https://github.com/flexflow/FlexFlow/blob/0c45f61f114414215c5eff7d39006f07735e2fe7/lib/utils/include/utils/variant.h#L190-L200

Done.

---

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

Previously, lockshaw (Colin Unger) wrote…

get_incoming_edges?

Done.

---

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

Previously, lockshaw (Colin Unger) wrote…

get_outgoing_edges?

Done.

---

lib/utils/src/graph/open_graphs.cc line 55 at r7 (raw file):

NodePort OpenMultiDiGraph::add_node_port() {
  return get_ptr().add_node_port();

Done.

---

lib/utils/src/graph/views.cc line 473 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Doing this much logic in the member initializer list is not great--let's move this into the actual body of the constructor

Done.

---

lib/utils/src/graph/views.cc line 486 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Use set union instead of extend

The element type of result (OpenMultiDiEdge) and query_edge (OutputMultiDiEdge) do not match, so we cannot just use set union.

---

lib/utils/src/graph/views.cc line 498 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Move into constructor body

Done.

---

lib/utils/src/graph/views.cc line 508 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Use set union instead of extend

The element type of result (OpenMultiDiEdge) and query_edge (OutputMultiDiEdge) do not match, so we cannot just use set union.

[lockshaw]

Reviewed 22 of 23 files at r8, all commit messages.
Reviewable status: 152 of 153 files reviewed, 60 unresolved discussions (waiting on @wmdi)

---

lib/compiler/include/compiler/machine_mapping.h line 27 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

This is related to an issue I forgot to report. We need these attributes, but FF_VISITABLE_STRUCT would fail with them.

What is the issue?

---

lib/compiler/include/compiler/unity_algorithm.h line 39 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

I don't want automatically generated operator== for them.

Why not? Usually that's an indicator that you shouldn't actually be using visitable

---

lib/compiler/include/compiler/unity_algorithm.h line 14 at r8 (raw file):

  ParallelComputationGraph pcg;
  MachineMapping machine_mapping;
  req<float> runtime;

Why put runtime as an object on Strategy rather than having an unordered_map somewhere from pairs of ParallelComputationGraph and MachineMapping to runtime?

---

lib/compiler/src/graph_utils.cc line 15 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

The template arguments make it look so but I don't think it too complicated. An explicit materialization is required here because we need a graph instead of a graph view, which is because we need to change the graph in the algorithm.

Why are we changing the graph in the algorithm? If you're referring to applying substitutions, then it seems like materializing the graph should instead be handled there and not here?

Also, the need to pass all those template arguments to me suggests that the interface actually is to complex, as there generally should not be like 5 different template parameters that all have effects that don't have reasonable defaults that can be inferred

---

lib/compiler/src/machine_mapping.cc line 48 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

Yes, due to the visitable issue of OptimalCostState.

What is this issue?

---

lib/compiler/src/machine_mapping.cc line 157 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

We use template parameters to specify the open type of the subgraph now. It should be unnecessary after we change the open subgraph algorithm.

Why can't this just be inferred from the type of g?

---

lib/compiler/src/machine_mapping.cc line 100 at r8 (raw file):

                        &frontier_machine_views) {
  float cost = 0;
  for (Node const &node : get_nodes(g)) {

It's not entirely clear to me what this function is doing. Why is the cost just the sum of the cost of the pieces? Where is parallelism taken into account?

---

lib/compiler/src/unity_algorithm.cc line 13 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

Done.

In that case, maybe rename to get_all_applicable_substitutions? I think the clarity is worth the extra typing

---

lib/compiler/src/unity_algorithm.cc line 46 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

I treat the PCG as the input of optimal_cost, which means optimal_cost searches for the best device mapping for a fixed PCG. On the other hand, the upper-level search graph_optimize searches for a PCG. Thus it makes more sense that PCG is not part of the output of optimal_cost.

In that case why is the PCG part of Strategy?

---

lib/compiler/test/src/test_labelled_open_graph.cc line 59 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

To get around the fmt issue.

What fmt issue?

---

lib/compiler/test/src/test_labelled_open_graph.cc line 10 at r8 (raw file):

  auto g = OpenMultiDiGraph::create<AdjacencyOpenMultiDiGraph>();

  int t0 = 100000;

What is t0 here for?

---

lib/compiler/test/src/test_optimal_cost.cc line 11 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

They use rapidcheck which does not work for now.

In that case an issue should be created and a link should be placed in the code so people reading this know why it's commented out and we don't lose track of the fact that it needs to be fixed.

---

lib/pcg/include/pcg/operator.h line 23 at r8 (raw file):

FF_VISITABLE_STRUCT(Operator, attrs, name);

static_assert(is_well_behaved_value_type<Operator>::value, "");

Suggestion:

static_assert(is_well_behaved_value_type<Operator>::value)

---

lib/pcg/include/pcg/parallel_computation_graph.h line 18 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

We don't have a hash function for OutputLabelledOpenMultiDiGraph (and I think we should not have one), so we cannot directly use MAKE_TYPEDEF_HASHABLE for ParallelComputationGraph and thus have a separate hash function. For this reason, since operator== should be consistent with the hash function, we have a separate operator== as well

Why should we not have a hash function for graphs? We have hash functions for sets and maps, which should be sufficient for hashing graphs. In either case the hash function here looks problematic--it seems to rely on topological ordering, which is not necessarily going to return a consistent ordering for anything other than straight-line graphs

---

lib/pcg/src/operator.cc line 5 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

Done.

Remove?

---

lib/utils/include/utils/graph/algorithms.h line 110 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

I expect they have different semantics?

Should probably be renamed to get_output_edges

---

lib/utils/include/utils/graph/labelled/views.h line 116 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

I don't think so. Although MultiDiEdge and OutputMultiDiEdge can be converted to MultiDiOutput, variant<MultiDiEdge, OutputMultiDiEdge, InputMultiDiEdge> cannot.

But the case here is not that we have a variant<MultiDiEdge, OutputMultiDiEdge, InputMultiDiEdge>, it's a variant<MultiDiEdge, OutputMultiDiEdge>, as otherwise this function wouldn't make any sense. Either way we need to figure out a way to better encapsulate the complexity of interacting with the variants (wrapping them in classes, more generic functions in variant.h, etc.), as this current code is much more difficult to read than it should be considering the simplicity of the underlying operation.

---

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

Previously, wmdi (Mengdi Wu) wrote…

Let's allow this to make it as general as possible. If we need such a constraint, we should write another check function.

In that case let's write that function now

---

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

Previously, wmdi (Mengdi Wu) wrote…

Done.

And then refactor these cases to use the new function

---

lib/utils/src/graph/views.cc line 486 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

The element type of result (OpenMultiDiEdge) and query_edge (OutputMultiDiEdge) do not match, so we cannot just use set union.

See https://reviewable.io/reviews/flexflow/FlexFlow/1229#-NpXIBGU11sbI_OAUfTb

---

lib/utils/src/graph/views.cc line 508 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

The element type of result (OpenMultiDiEdge) and query_edge (OutputMultiDiEdge) do not match, so we cannot just use set union.

Makes sense. In that case let's instead do an explicit cast on the right-hand side or modify set_union to allow it to take arguments of different types that can be made the same via a static cast

---

lib/utils/src/graph/views.cc line 473 at r8 (raw file):

    OpenMultiDiGraphView const &g, std::unordered_set<Node> const &nodes)
    : g(g), nodes(nodes) {
  inputs = transform(get_cut_set(g, nodes), to_inputmultidiedge);

Suggestion:

 this->inputs = transform(get_cut_set(g, nodes), to_inputmultidiedge);

---

lib/utils/src/graph/views.cc line 499 at r8 (raw file):

    OpenMultiDiGraphView const &g, std::unordered_set<Node> const &nodes)
    : g(g), nodes(nodes) {
  outputs = transform(get_cut_set(g, nodes), to_outputmultidiedge);

Suggestion:

  this->outputs

[wmdi]

Reviewable status: 152 of 153 files reviewed, 60 unresolved discussions (waiting on @lockshaw)

---

lib/compiler/include/compiler/machine_mapping.h line 27 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

What is the issue?

In file included from /home/mengdiwu/wmdi/FlexFlow/lib/op-attrs/include/op-attrs/tensor_shape.h:8,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/op-attrs/include/op-attrs/parallel_tensor_shape.h:5,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/op-attrs/include/op-attrs/ops/attention.h:5,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/op-attrs/include/op-attrs/operator_attrs.h:5,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/compiler/include/compiler/cost_estimate.h:5,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/compiler/include/compiler/unity_algorithm.h:4,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/compiler/src/graph_utils.h:4,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/compiler/src/graph_utils.cc:1:
/home/mengdiwu/wmdi/FlexFlow/lib/utils/include/utils/visitable.h:320:56: error: static assertion failed: OptimalCostState should be fully visitable
  320 |   static_assert(sizeof(visit_as_tuple_raw_t<TYPENAME>) == sizeof(TYPENAME),    \
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
/home/mengdiwu/wmdi/FlexFlow/lib/utils/include/utils/visitable.h:313:3: note: in expansion of macro ‘CHECK_WELL_BEHAVED_VISIT_TYPE_NONSTANDARD_CONSTRUCTION_NO_EQ’
  313 |   CHECK_WELL_BEHAVED_VISIT_TYPE_NONSTANDARD_CONSTRUCTION_NO_EQ(TYPENAME);      \

Is it because std::unordered_map is not visitable?

[lockshaw]

Reviewable status: 152 of 153 files reviewed, 60 unresolved discussions (waiting on @wmdi)

---

lib/compiler/include/compiler/machine_mapping.h line 27 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

In file included from /home/mengdiwu/wmdi/FlexFlow/lib/op-attrs/include/op-attrs/tensor_shape.h:8,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/op-attrs/include/op-attrs/parallel_tensor_shape.h:5,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/op-attrs/include/op-attrs/ops/attention.h:5,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/op-attrs/include/op-attrs/operator_attrs.h:5,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/compiler/include/compiler/cost_estimate.h:5,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/compiler/include/compiler/unity_algorithm.h:4,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/compiler/src/graph_utils.h:4,
                 from /home/mengdiwu/wmdi/FlexFlow/lib/compiler/src/graph_utils.cc:1:
/home/mengdiwu/wmdi/FlexFlow/lib/utils/include/utils/visitable.h:320:56: error: static assertion failed: OptimalCostState should be fully visitable
  320 |   static_assert(sizeof(visit_as_tuple_raw_t<TYPENAME>) == sizeof(TYPENAME),    \
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
/home/mengdiwu/wmdi/FlexFlow/lib/utils/include/utils/visitable.h:313:3: note: in expansion of macro ‘CHECK_WELL_BEHAVED_VISIT_TYPE_NONSTANDARD_CONSTRUCTION_NO_EQ’
  313 |   CHECK_WELL_BEHAVED_VISIT_TYPE_NONSTANDARD_CONSTRUCTION_NO_EQ(TYPENAME);      \

Is it because std::unordered_map is not visitable?

It's complaining that not all of the fields of OptimalCostState are listed under the FF_VISITABLE_STRUCT declaration. I'm not entirely sure what code you tried to compile, but the following should be correct:

Suggestion:

struct OptimalCostState {
  SerialParallelDecomposition subgraph;
  MachineSpecification resource;
  std::unordered_map<Node, MachineView> given_machine_views;
  req<std::unordered_map<OpenMultiDiEdge, MachineView>> frontier_machine_views;
};
FF_VISITABLE_STRUCT(OptimalCostState, subgraph, resource, given_machine_views, frontier_machine_views);

[wmdi]

Reviewable status: 152 of 153 files reviewed, 60 unresolved discussions (waiting on @lockshaw)

---

lib/compiler/include/compiler/machine_mapping.h line 27 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

It's complaining that not all of the fields of OptimalCostState are listed under the FF_VISITABLE_STRUCT declaration. I'm not entirely sure what code you tried to compile, but the following should be correct:

I changed to this version and got a long error information which starts with

/home/mengdiwu/wmdi/FlexFlow/lib/compiler/src/machine_mapping.cc:168:22: error: use of deleted function ‘FlexFlow::required<std::unordered_map<mpark::variant<FlexFlow::InputMultiDiEdge, FlexFlow::OutputMultiDiEdge, FlexFlow::MultiDiEdge>, FlexFlow::MachineView>, void>::required()’

[lockshaw]

lib/compiler/include/compiler/machine_mapping.h line 27 at r7 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

I changed to this version and got a long error information which starts with

/home/mengdiwu/wmdi/FlexFlow/lib/compiler/src/machine_mapping.cc:168:22: error: use of deleted function ‘FlexFlow::required<std::unordered_map<mpark::variant<FlexFlow::InputMultiDiEdge, FlexFlow::OutputMultiDiEdge, FlexFlow::MultiDiEdge>, FlexFlow::MachineView>, void>::required()’

That's because you're not passing in sufficient arguments at that line: https://github.com/flexflow/FlexFlow/blob/95fa427b8680e68acf33cccf96bbc66bb37cd1fa/lib/compiler/src/machine_mapping.cc#L168-L169

[lockshaw]

Reviewed 1 of 23 files at r8.
Reviewable status: all files reviewed, 60 unresolved discussions (waiting on @lockshaw and @wmdi)

[wmdi]

Reviewed 68 of 68 files at r18, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lockshaw)

[wmdi]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lockshaw)

---

lib/pcg/include/pcg/parallel_computation_graph.h line 18 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

But the equality operation here is just falling back to the hash function, so clearly there's an implicit usage here. I think we should define the strictest hash function by default here, and then other hash functions can be used as desired but are not the default.

Tracked in #1329

[wmdi]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lockshaw)

---

lib/utils/src/graph/open_graphs.cc line 25 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Still seems to be dynamic?

Done.

[wmdi]

Reviewed 10 of 10 files at r19, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lockshaw)

[wmdi]

Reviewed 23 of 23 files at r20, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lockshaw)

[lockshaw]

Reviewed 10 of 22 files at r17, 38 of 68 files at r18, 10 of 10 files at r19, 20 of 23 files at r20, 27 of 27 files at r21, 4 of 4 files at r22, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wmdi)

[lockshaw]

Reviewed 12 of 12 files at r23, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wmdi)