SP-ization

[Marsella8]

Description of changes:

Related Issues:

Linked Issues:

• Issue #

Issues closed by this PR:

• Closes #

Before merging:

• Did you update the flexflow-third-party repo, if modifying any of the Cmake files, the build configs, or the submodules?

---

This change is 

[Marsella8]

Reviewable status: 0 of 78 files reviewed, 1 unresolved discussion (waiting on @lockshaw)

---

lib/utils/test/src/utils/graph/series_parallel/sp_ization/spanish_algo.cc line 23 at r1 (raw file):

TEST_SUITE(FF_TEST_SUITE) {
  TEST_CASE("spanish_algo - subcomponents") {
    SUBCASE("add_dummy_nodes") {

how should I test graphs where nodes are added? Here I check for some properties but it's obviously non-exhaustive

[Marsella8]

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

---

lib/utils/include/utils/graph/series_parallel/sp_ization/work_preserving_sp_ization.h line 64 at r1 (raw file):

 *the SP-ized graph.
 **/
SeriesParallelDecomposition cost_aware_stratum_sync_sp_ization(

old cost-aware algo, will leave it in for benchmarking purposes though we'll probably remove it eventually

---

bin/sp_ization_benchmarking/nasnet_bench_graph_generator.h line 1 at r1 (raw file):

// For context, see https://arxiv.org/abs/1902.09635 &&

Benchmarking code is a bit sloppy but it's onyl temporary, should be fine(?)

[codecov]

Codecov Report

Attention: Patch coverage is 95.75758% with 28 lines in your changes missing coverage. Please review.

Project coverage is 62.67%. Comparing base (2b71235) to head (94afb3c).

Files with missing lines	Patch %	Lines
...s/graph/series_parallel/series_parallel_metrics.cc	81.25%	12 Missing ⚠️
.../utils/graph/series_parallel/digraph_generation.cc	90.62%	6 Missing ⚠️
..._parallel/sp_ization/work_preserving_sp_ization.cc	96.96%	3 Missing ⚠️
...h/series_parallel/series_parallel_decomposition.cc	95.00%	2 Missing ⚠️
...s/graph/series_parallel/sp_ization/spanish_algo.cc	98.55%	2 Missing ⚠️
.../utils/graph/digraph/algorithms/get_bottlenecks.cc	93.33%	1 Missing ⚠️
.../digraph/algorithms/get_lowest_common_ancestors.cc	94.44%	1 Missing ⚠️
...s/src/utils/graph/series_parallel/get_ancestors.cc	97.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1562      +/-   ##
==========================================
+ Coverage   60.90%   62.67%   +1.77%     
==========================================
  Files         620      636      +16     
  Lines       14541    15191     +650     
==========================================
+ Hits         8856     9521     +665     
+ Misses       5685     5670      -15     

Flag	Coverage Δ
unittests	62.67% <95.75%> (+1.77%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../utils/include/utils/containers/unordered_set_of.h	100.00% <ø> (ø)
lib/utils/src/utils/graph/algorithms.cc	27.52% <ø> (+8.25%)	⬆️
lib/utils/src/utils/graph/digraph/algorithms.cc	100.00% <100.00%> (ø)
...rc/utils/graph/digraph/algorithms/get_ancestors.cc	100.00% <100.00%> (ø)
.../utils/graph/digraph/algorithms/get_descendants.cc	100.00% <100.00%> (ø)
...ils/graph/digraph/algorithms/get_dominators_map.cc	100.00% <ø> (ø)
...h/algorithms/get_longest_path_lengths_from_root.cc	100.00% <100.00%> (ø)
...aph/digraph/algorithms/get_topological_ordering.cc	100.00% <ø> (ø)
...hms/get_topological_ordering_from_starting_node.cc	100.00% <100.00%> (ø)
...tils/graph/digraph/algorithms/is_2_terminal_dag.cc	100.00% <100.00%> (ø)
... and 15 more

... and 10 files with indirect coverage changes

[lockshaw]

Reviewed 44 of 78 files at r1, 10 of 19 files at r2, 1 of 1 files at r3, 2 of 3 files at r4, all commit messages.
Reviewable status: 56 of 78 files reviewed, 34 unresolved discussions (waiting on @Marsella8)

---

lib/utils/include/utils/graph/series_parallel/sp_ization/work_preserving_sp_ization.h line 64 at r1 (raw file):

Previously, Marsella8 wrote…

old cost-aware algo, will leave it in for benchmarking purposes though we'll probably remove it eventually

Sounds good, can you include this information in the actual comment itself? It's useful to have information like this (i.e., how all the various algorithms relate to each other and what our future plans are for them) in the code itself

---

lib/utils/test/src/utils/graph/series_parallel/sp_ization/spanish_algo.cc line 23 at r1 (raw file):

Previously, Marsella8 wrote…

how should I test graphs where nodes are added? Here I check for some properties but it's obviously non-exhaustive

I'd recommend isomorphism checking, similar to what we do with dataflow graphs

---

lib/utils/include/utils/graph/digraph/algorithms/get_bottlenecks.h line 19 at r4 (raw file):

 bottleneck if and only if it's the unique source / sink of the graph.
 */
std::unordered_set<Node> get_bottlenecks(DiGraphView const &g);

What is the difference between bottlenecks and dominators?

---

lib/utils/include/utils/graph/digraph/algorithms/get_ancestors.h line 14 at r2 (raw file):

 * @note `n` is not considered to be its own descendant, and is thus not
 *included in the returned set.
 **/

Suggestion:

/**
 * @brief Computes the set of all ancestors of a given node `n` in a directed
 * graph, which is the set of all nodes `m` for which a directed path from `n` to
 * `m` exists.
 *
 * @note `n` is not considered to be its own ancestor, and is thus not
 * included in the returned set.
 */

---

lib/utils/include/utils/graph/digraph/algorithms/get_descendants.h line 10 at r4 (raw file):

/**
 * @brief Computes the set of all descendants of a given node in a directed
 *graph.

Suggestion:

 * graph.

---

lib/utils/include/utils/graph/digraph/algorithms/get_descendants.h line 13 at r4 (raw file):

 *
 * @note `starting_node` is not considered to be its own descendant, and is thus
 *not included in the returned set.

Suggestion:

 * not included in the returned set.

---

lib/utils/include/utils/graph/digraph/algorithms/get_lowest_common_ancestors.h line 35 at r4 (raw file):

 * LCA, or a set of nodes as LCA.
 */
std::optional<std::unordered_set<Node>>

Why does this return a std::optional? What does returning std::nullopt signify?

Code quote:

std::optional<std::unordered_set<Node>>

---

lib/utils/include/utils/graph/digraph/algorithms/get_longest_path_lengths_from_root.h line 18 at r4 (raw file):

 * @note The root has a path length of 1. g must be acyclic.
 */
std::unordered_map<Node, int>

Suggestion:

std::unordered_map<Node, nonnegative_int>

---

lib/utils/include/utils/graph/digraph/algorithms/get_longest_path_lengths_from_root.h line 32 at r4 (raw file):

 */
std::unordered_map<Node, float> get_weighted_longest_path_lengths_from_root(
    DiGraphView const &g, std::unordered_map<Node, float> const &node_costs);

Are negative node costs allowed by this function?

Code quote:

    DiGraphView const &g, std::unordered_map<Node, float> const &node_costs);

---

lib/utils/include/utils/graph/series_parallel/series_parallel_decomposition.h line 23 at r4 (raw file):

bool is_empty(SeriesSplit const &serial);
bool is_empty(ParallelSplit const &parallel);
bool is_empty(SeriesParallelDecomposition const &sp);

Should be changed to be defined for NonNormalSeriesParallelDecomposition

Code quote:

bool is_empty(SeriesParallelDecomposition const &sp);

---

lib/utils/include/utils/graph/series_parallel/series_parallel_decomposition.h line 32 at r4 (raw file):

 * multiple times
 */
size_t num_nodes(SeriesParallelDecomposition const &sp);

Suggestion:

nonnegative_int num_nodes(SeriesParallelDecomposition const &sp);

---

lib/utils/include/utils/graph/series_parallel/sp_ization/node_role.enum.toml line 2 at r4 (raw file):

namespace = "FlexFlow"
name = "NodeRole"

What is this? A docstring would be quite helpful here to add some context for what this enum represents and what it's used for

---

lib/utils/include/utils/graph/series_parallel/series_parallel_metrics.h line 20 at r4 (raw file):

    get_node_counter_map(ParallelSplit const &parallel);
std::unordered_map<Node, size_t>
    get_node_counter_map(SeriesParallelDecomposition const &sp);

Seems weird having all but the last of these as public functions--what about making the first three lambdas/static?

---

lib/utils/include/utils/graph/series_parallel/series_parallel_metrics.h line 20 at r4 (raw file):

    get_node_counter_map(ParallelSplit const &parallel);
std::unordered_map<Node, size_t>
    get_node_counter_map(SeriesParallelDecomposition const &sp);

Suggestion:

std::unordered_map<Node, nonnegative_int>
    get_node_counter_map(SeriesParallelDecomposition const &sp);

---

lib/utils/include/utils/graph/series_parallel/series_parallel_metrics.h line 20 at r4 (raw file):

    get_node_counter_map(ParallelSplit const &parallel);
std::unordered_map<Node, size_t>
    get_node_counter_map(SeriesParallelDecomposition const &sp);

Suggestion:

    get_node_num_occurences_map(SeriesParallelDecomposition const &sp);

---

lib/utils/include/utils/graph/series_parallel/series_parallel_metrics.h line 38 at r4 (raw file):

 *
 */
int num_dependencies(SeriesParallelDecomposition const &sp);

Suggestion:

nonnegative_int num_dependencies(SeriesParallelDecomposition const &sp);

---

lib/utils/include/utils/graph/series_parallel/sp_ization/spanish_algo.h line 23 at r4 (raw file):

                  std::unordered_map<Node, NodeRole> const &node_roles);

SeriesParallelDecomposition spanish_strata_sync(DiGraph g);

Can we get some documentation of the functions in this header, ideally accompanied by some corresponding links to the paper?

---

lib/utils/test/src/utils/graph/digraph/algorithms/is_acyclic.cc line 55 at r4 (raw file):

      CHECK(is_acyclic(g));
    }
    SUBCASE("traversal with root") {

Why "traversal"? I'm not sure what it means in this context

Code quote:

  SUBCASE("traversal with root") {

---

lib/utils/test/src/utils/graph/digraph/algorithms/get_longest_path_lengths_from_root.cc line 14 at r4 (raw file):

    std::vector<Node> n = add_nodes(g, 5);
    std::vector<DirectedEdge> edges = {
        DirectedEdge{n[0], n[1]},

Prefer .at for bounds-checking

Suggestion:

       DirectedEdge{n.at(0), n[1]},

---

lib/utils/test/src/utils/graph/digraph/algorithms/get_longest_path_lengths_from_root.cc line 33 at r4 (raw file):

  }

  TEST_CASE("get_longest_path_lengths_from_root - more complex graph") {

Prefer TEST_CASE with nested SUBCASEs

Code quote:

  TEST_CASE("get_longest_path_lengths_from_root - more complex graph") {

---

lib/utils/test/src/utils/graph/series_parallel/digraph_generation.cc line 54 at r4 (raw file):

    }

    SUBCASE("Mixed Serial-Parallel") {

Suggestion:

    SUBCASE("Mixed Series-Parallel") {

---

lib/utils/test/src/utils/graph/series_parallel/digraph_generation.cc line 65 at r4 (raw file):

    }

    SUBCASE("Mixed Parallel-Serial") {

Suggestion:

    SUBCASE("Mixed Parallel-Series") {

---

lib/utils/test/src/utils/graph/series_parallel/digraph_generation.cc line 93 at r4 (raw file):

      CHECK(num_edges(result) == 4);
      CHECK(get_sources(result).size() == 1);
      CHECK(get_sinks(result).size() == 1);

Probably better to check the full graph structure using isomorphism checking

Code quote:

      CHECK(num_nodes(result) == 4);
      CHECK(num_edges(result) == 4);
      CHECK(get_sources(result).size() == 1);
      CHECK(get_sinks(result).size() == 1);

---

lib/utils/test/src/utils/graph/digraph/algorithms/get_lowest_common_ancestors.cc line 11 at r4 (raw file):

  TEST_CASE("get_lowest_common_ancestors") {
    DiGraph g = DiGraph::create<AdjacencyDiGraph>();

Add a check for the case where get_lowest_common_ancestors returns std::nullopt?

---

lib/utils/test/src/utils/graph/digraph/algorithms/get_lowest_common_ancestors.cc line 18 at r4 (raw file):

        std::unordered_set<Node> result =
            get_lowest_common_ancestors(g, {n.at(0)}).value();
        CHECK(correct == result);

Suggestion:

        std::optional<std::unordered_set<Node>> correct = {n.at(0)};
        std::optional<std::unordered_set<Node>> result =
            get_lowest_common_ancestors(g, {n.at(0)});
        CHECK(correct == result);

---

lib/utils/test/src/utils/graph/digraph/algorithms/get_lowest_common_ancestors.cc line 38 at r4 (raw file):

        correct = {n.at(2)};
        result = get_lowest_common_ancestors(g, {n.at(2)}).value();
        CHECK(correct == result);

Ideally justify these examples with SUBCASEs. Ideally every correct/result should be declared once in its own SUBCASE (doesn't have to always hold, but is a good heuristic)

Code quote:

        std::unordered_set<Node> correct = {n.at(0)};
        std::unordered_set<Node> result =
            get_lowest_common_ancestors(g, {n.at(1), n.at(2)}).value();
        CHECK(correct == result);

        correct = {n.at(1)};
        result = get_lowest_common_ancestors(g, {n.at(1)}).value();
        CHECK(correct == result);

        correct = {n.at(2)};
        result = get_lowest_common_ancestors(g, {n.at(2)}).value();
        CHECK(correct == result);

---

lib/utils/test/src/utils/graph/series_parallel/sp_ization/critical_path_preserving_sp_ization.cc line 15 at r4 (raw file):

TEST_SUITE(FF_TEST_SUITE) {

  TEST_CASE("critical_path_preserving_sp_ization") {

Honestly some rapidcheck tests might be nice here in addition to the manual tests, as there's a clear property that you can check that you would like to hold for all acyclic dags

---

lib/utils/test/src/utils/graph/series_parallel/sp_ization/critical_path_preserving_sp_ization.cc line 17 at r4 (raw file):

  TEST_CASE("critical_path_preserving_sp_ization") {

    SUBCASE("Sample Graph #1") {

It would be nice to have stronger justification for this testcase than "Sample Graph #1"--what is this subcase checking? Why did you choose this graph? How is it different from "Sample Graph #2"?

---

lib/utils/test/src/utils/graph/series_parallel/sp_ization/critical_path_preserving_sp_ization.cc line 55 at r4 (raw file):

        CHECK(correct == result);
      }
      SUBCASE("work cost") {

This doesn't seem to check any properties of critical_path_preserving_sp_ization beyond what the "structure" subcase checks?

---

lib/utils/test/src/utils/graph/series_parallel/sp_ization/critical_path_preserving_sp_ization.cc line 286 at r4 (raw file):

      }
    }
    SUBCASE("Transitive Reduction") {

What does htis mean as a testcase? It doesn't seem like it's testing transitive reduction, so it's a bit oddly named.

---

lib/utils/include/utils/graph/series_parallel/normalize_sp_decomposition.h line 11 at r4 (raw file):

 * @brief Recursively normalizes a SeriesParallelDecomposition.
 *
 * @details This function performs the following semantic substitutions:

I would rather this be a structural property of SeriesParallelDecomposition. Can you create some additional type (e.g., NonNormalSPDecomposition) and then make this function be SeriesParallelDecomposition normalize_sp_decomposition(NonNormalSPDecomposition const &)?

---

lib/utils/test/src/utils/graph/series_parallel/normalize_sp_decomposition.cc line 9 at r4 (raw file):

TEST_SUITE(FF_TEST_SUITE) {
  TEST_CASE("normalize_sp_decomposition") {
    Node n1 = Node(1);

Prefer bracket initialization

Suggestion:

    Node n1 = Node{1};

---

lib/utils/src/utils/containers/invert_map.cc line 1 at r2 (raw file):

#include "utils/containers/invert_map.h"

Add an explicit specialization using value_type

---

lib/utils/test/src/utils/graph/series_parallel/get_ancestors.cc line 11 at r4 (raw file):

  TEST_CASE("get_ancestors") {
    std::vector<Node> n = {
        Node(0), Node(1), Node(2), Node(3), Node(4), Node(5), Node(6), Node(7)};

Prefer bracket initialization

Suggestion:

        Node{0}, Node(1), Node(2), Node(3), Node(4), Node(5), Node(6), Node(7)};