Merge substitution-builder

[victorli2002]

Merging the substitution-builder branch

• adding top-level function to fetch all substitutions

Related Issues:

Linked Issues:

• Issue Add ability to get vector of all enum values to proj dtgen #1478

Issues closed by this PR:

• Closes #

---

This change is 

[lockshaw]

Patch coverage isn't great, that probably should get improved before merging (or at least investigated)

Reviewed 62 of 64 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @lockshaw and @victorli2002)

---

lib/substitutions/include/substitutions/operator_pattern/operator_attribute_constraint.h line 13 at r2 (raw file):

                                               OperatorAttributeValue const &);
OperatorAttributeConstraint op_attr_key_divisible_by(OperatorAttributeKey,
                                                     int denominator);

Suggestion:

                                                     nonnegative_int denominator);

---

lib/substitutions/src/substitutions/sub_parallel_computation_graph.cc line 191 at r2 (raw file):

std::string as_dot(SubParallelComputationGraph const &spcg) {
  NOT_IMPLEMENTED();

TODO

---

lib/substitutions/include/substitutions/tensor_pattern/tensor_attribute_pattern.h line 9 at r2 (raw file):

TensorAttributePattern tensor_attribute_pattern_match_all();
TensorAttributePattern tensor_attr_pattern_require_num_dims(int num_dims);

Suggestion:

TensorAttributePattern tensor_attr_pattern_require_num_dims(nonnegative_int num_dims);

---

lib/utils/include/utils/containers.h line 0 at r2 (raw file):
Delete this file

---

lib/substitutions/src/substitutions/substitution_builder.cc line 110 at r2 (raw file):

                            "OutputGraphExprValue holding a "
                            "OutputGraphExprNodeOutput, but received {}",
                            maybe_pattern_output));

Suggestion:

maybe_output_graph_expr_output

---

lib/substitutions/src/substitutions/substitution_builder.cc line 115 at r2 (raw file):

  assert(!this->output_mapping.contains_l(pattern_output));
  assert(!this->output_mapping.contains_r(output_graph_expr_output));

Throw an exception with an actual error message rather than using assert

Code quote:

  assert(!this->output_mapping.contains_l(pattern_output));
  assert(!this->output_mapping.contains_r(output_graph_expr_output));

---

lib/utils/src/utils/graph/open_dataflow_graph/algorithms/view_inputs_as_nodes.cc line 5 at r2 (raw file):

namespace FlexFlow {

ViewInputsAsNodes::ViewInputsAsNodes(

Is ViewInputsAsNodes actually used anywhere? If not it should be removed

---

lib/substitutions/src/substitutions/unity_substitution_set.cc line 14 at r2 (raw file):

  std::vector<Substitution> substitutions;
  for (int num_dims : range(MAX_TENSOR_DIM)) {
    for (int degree = 1; degree <= resources.num_nodes; degree *= 2) {

Suggestion:

    for (int degree = 1; degree <= get_num_gpus(resources); degree *= 2) {

---

lib/utils/src/utils/graph/dataflow_graph/algorithms/as_dot.cc line 0 at r2 (raw file):
What happened to the old dot rendering code? Does it need to be deleted?

---

lib/utils/include/utils/render_dot.h line 1 at r2 (raw file):

#ifndef _FLEXFLOW_LIB_UTILS_INCLUDE_UTILS_RENDER_DOT_H

This should probably be in utils/graph

---

lib/utils/include/utils/containers/replicate.h line 11 at r2 (raw file):

template <typename T>
std::vector<T> replicate(int n, T const &element) {

Suggestion:

std::vector<T> replicate(nonnegative_int n, T const &element) {

---

lib/utils/include/utils/containers/replicate.h line 14 at r2 (raw file):

  if (n < 0) {
    throw mk_runtime_error(
        fmt::format("replicate expected n > 0, but received n = {}", n));

Suggestion:

        fmt::format("replicate expected n >= 0, but received n = {}", n));

---

lib/substitutions/src/substitutions/substitution.cc line 32 at r2 (raw file):

  };

  auto l_from_r_output_attrs_assignment =

This code needs to get cleaned up

---

lib/utils/include/utils/containers/merge_maps.h line 18 at r2 (raw file):

    merge_maps(std::unordered_map<K, V> const &lhs,
               std::unordered_map<K, V> const &rhs,
               MergeMethod merge_method = MergeMethod::REQUIRE_DISJOINT) {

Does this actually need to be dynamically choosable, or can we just create three functions (merge_disjoint_maps, merge_maps_left_dominates, merge_maps_right_dominates)?

Code quote:

               MergeMethod merge_method = MergeMethod::REQUIRE_DISJOINT

---

lib/substitutions/test/src/substitutions/substitution.cc line 81 at r2 (raw file):

  }

  // TEST_CASE("is_valid_substitution") {

TODO

[victorli2002]

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

---

lib/substitutions/src/substitutions/sub_parallel_computation_graph.cc line 191 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

TODO

Can you take this?

---

lib/substitutions/src/substitutions/substitution.cc line 32 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

This code needs to get cleaned up

Can you take this?

---

lib/utils/include/utils/containers/merge_maps.h line 18 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Does this actually need to be dynamically choosable, or can we just create three functions (merge_disjoint_maps, merge_maps_left_dominates, merge_maps_right_dominates)?

I think it should be fine to create three separate functions. Will do that.

---

lib/utils/src/utils/graph/dataflow_graph/algorithms/as_dot.cc line at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

What happened to the old dot rendering code? Does it need to be deleted?

If I'm not mistaken then this is the old code?
flexflow-train/lib/utils/src/utils/graph/dataflow_graph/algorithms/as_dot.cc at master · flexflow/flexflow-train

The version from substitution-builder seemed more robust and nicer to me.

---

lib/utils/src/utils/graph/open_dataflow_graph/algorithms/view_inputs_as_nodes.cc line 5 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Is ViewInputsAsNodes actually used anywhere? If not it should be removed

It's not used anywhere atm. Will remove

[lockshaw]

lib/substitutions/src/substitutions/substitution.cc line 32 at r2 (raw file):

Previously, victorli2002 (Victor Li) wrote…

Can you take this?

Sure

[lockshaw]

lib/substitutions/src/substitutions/sub_parallel_computation_graph.cc line 191 at r2 (raw file):

Previously, victorli2002 (Victor Li) wrote…

Can you take this?

Sure

[victorli2002]

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

---

lib/substitutions/src/substitutions/substitution_builder.cc line 115 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Throw an exception with an actual error message rather than using assert

Done.

---

lib/utils/include/utils/containers.h line at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Delete this file

Done.

---

lib/substitutions/include/substitutions/operator_pattern/operator_attribute_constraint.h line 13 at r2 (raw file):

                                               OperatorAttributeValue const &);
OperatorAttributeConstraint op_attr_key_divisible_by(OperatorAttributeKey,
                                                     int denominator);

Done.

---

lib/substitutions/src/substitutions/substitution_builder.cc line 110 at r2 (raw file):

                            "OutputGraphExprValue holding a "
                            "OutputGraphExprNodeOutput, but received {}",
                            maybe_pattern_output));

Done.

---

lib/substitutions/include/substitutions/tensor_pattern/tensor_attribute_pattern.h line 9 at r2 (raw file):

TensorAttributePattern tensor_attribute_pattern_match_all();
TensorAttributePattern tensor_attr_pattern_require_num_dims(int num_dims);

Done.

---

lib/substitutions/src/substitutions/unity_substitution_set.cc line 14 at r2 (raw file):

  std::vector<Substitution> substitutions;
  for (int num_dims : range(MAX_TENSOR_DIM)) {
    for (int degree = 1; degree <= resources.num_nodes; degree *= 2) {

Done.

[lockshaw]

Reviewed 30 of 31 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @victorli2002)

[victorli2002]

Reviewable status: 66 of 75 files reviewed, 3 unresolved discussions (waiting on @lockshaw)

---

lib/utils/include/utils/containers/merge_maps.h line 18 at r2 (raw file):

Previously, victorli2002 (Victor Li) wrote…

I think it should be fine to create three separate functions. Will do that.

the left_dominates form is currently unusued, but it feels weird removing it while keeping right_dominates (which is used in one place)?

---

lib/utils/include/utils/render_dot.h line 1 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

This should probably be in utils/graph

Done.

---

lib/utils/include/utils/containers/replicate.h line 11 at r2 (raw file):

template <typename T>
std::vector<T> replicate(int n, T const &element) {

Done.

---

lib/utils/include/utils/containers/replicate.h line 14 at r2 (raw file):

  if (n < 0) {
    throw mk_runtime_error(
        fmt::format("replicate expected n > 0, but received n = {}", n));

Done.