Graph Documentation

[Marsella8]

Description of changes:

Added graph inheritance diagram, updated README.md

Related Issues:

Linked Issues:

• Issue Documentation for graph library #1383

Issues closed by this PR:

• Closes #

Issues closed by this PR:

• Closes Documentation for graph library #1383

---

This change is 

[lockshaw]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Marsella8 and @wmdi)

---

lib/utils/include/utils/graph/README.md line 190 at r1 (raw file):

![Open graphs inheritance diagram](docs/open.svg)

Arrows with pointed tips indicate inheritance, while arrows with square tips indicate that the pointing class has a 'cow_ptr' of the type of the pointed class. (for more info on `cow_ptr`, see below).

FYI I think you can actually do section references in markdown, e.g., see below or something like that

Code quote:

see below).

---

lib/utils/include/utils/graph/README.md line 244 at r1 (raw file):

### Virtual Inheritance (Possibly superflous)

No this is useful, I would just add an example of the diamond inheritance that shows up rather than just claiming that "diamond-style inheritance patterns emerge"--if they emerge it should be easy to add a quick example

Code quote:

 (Possibly superflous)

---

lib/utils/include/utils/graph/README.md line 248 at r1 (raw file):

In the case of a diamond inheritance pattern C++ will instantiate multiple copies of the base class whenever we instantiate a derived class.
To address this issue, we employ [Virtual Inheritance](https://en.wikipedia.org/wiki/Virtual_inheritance), which removes the ambiguity associated with the multiple copies.
Furthermore, the use of virtual functions allows for runtime polymorphism, allowing for a single function defined on some superclass to also work correctly on it's subclasses.

Suggestion:

its 

---

lib/utils/include/utils/graph/README.md line 252 at r1 (raw file):

### strong_typedef
`Node` inherits from `strong_typedef`: this is in order to ensure that distinct types that alias the same type are still considered distinct (and thus using one in place of the other will result in a compiler error).  
For more info, see https://www.foonathan.net/2016/10/strong-typedefs/.

Probably delete this, we're going to remove strong_typedef soon

Code quote:

### strong_typedef
`Node` inherits from `strong_typedef`: this is in order to ensure that distinct types that alias the same type are still considered distinct (and thus using one in place of the other will result in a compiler error).
For more info, see https://www.foonathan.net/2016/10/strong-typedefs/.

---

lib/utils/include/utils/graph/docs/generate_diagram.py line 79 at r1 (raw file):

if __name__=='__main__':
    cmd = 'hpp2plantuml -i "../labelled/*.h"'

I don't think hpp2plantuml is in the nix environment. It should be added to the flake.nix alongside a brief explanation of how to run this script

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.32%. Comparing base (411bc12) to head (cffbff3).

Additional details and impacted files

@@              Coverage Diff               @@
##           repo-refactor    #1391   +/-   ##
==============================================
  Coverage          41.32%   41.32%           
==============================================
  Files                181      181           
  Lines               5265     5265           
  Branches             271      271           
==============================================
  Hits                2176     2176           
  Misses              3089     3089           

Flag	Coverage Δ
unittests	41.32% <ø> (ø)

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

[Marsella8]

Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @lockshaw and @wmdi)

---

lib/utils/include/utils/graph/README.md line 244 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

No this is useful, I would just add an example of the diamond inheritance that shows up rather than just claiming that "diamond-style inheritance patterns emerge"--if they emerge it should be easy to add a quick example

Done.

---

lib/utils/include/utils/graph/README.md line 248 at r1 (raw file):

In the case of a diamond inheritance pattern C++ will instantiate multiple copies of the base class whenever we instantiate a derived class.
To address this issue, we employ [Virtual Inheritance](https://en.wikipedia.org/wiki/Virtual_inheritance), which removes the ambiguity associated with the multiple copies.
Furthermore, the use of virtual functions allows for runtime polymorphism, allowing for a single function defined on some superclass to also work correctly on it's subclasses.

Done.

---

lib/utils/include/utils/graph/README.md line 252 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Probably delete this, we're going to remove strong_typedef soon

Done.

---

lib/utils/include/utils/graph/docs/generate_diagram.py line 79 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

I don't think hpp2plantuml is in the nix environment. It should be added to the flake.nix alongside a brief explanation of how to run this script

I've ended up retooling the script to make it use hpp2plantuml as a python module instead of calling it through the command line so that it's easier to setup within nix, but I was unable to set it up (it's not present in nixpkgs, and had tried some other methods but was unsuccesful), let me know how to proceed.

[Marsella8]

Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @lockshaw and @wmdi)

---

lib/utils/include/utils/graph/README.md line 244 at r1 (raw file):

Previously, Marsella8 wrote…

Done.

I've looked around the Graph Library and was surprisingly there are actually no diamond-style inheritance patterns present (at least in the strict sense, so without considering a given class having a cow_ptr to another class as inheritance).
The reason virtual inheritance is still needed however is because we still want to keep a single copy of the superclasses, since while we don't access them using inheritance we still access them using cow_ptrs right? I've currently added this as an explanation to the graph docs, but I'm unsure on whether it is correct, lmk if I'm wrong and if so what is the purpose of virtual inheritance within the library.

[lockshaw]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Bob-Chen222, @Marsella8, and @wmdi)

---

lib/utils/include/utils/graph/README.md line 244 at r1 (raw file):

Previously, Marsella8 wrote…

I've looked around the Graph Library and was surprisingly there are actually no diamond-style inheritance patterns present (at least in the strict sense, so without considering a given class having a cow_ptr to another class as inheritance).
The reason virtual inheritance is still needed however is because we still want to keep a single copy of the superclasses, since while we don't access them using inheritance we still access them using cow_ptrs right? I've currently added this as an explanation to the graph docs, but I'm unsure on whether it is correct, lmk if I'm wrong and if so what is the purpose of virtual inheritance within the library.

I believe there are some, e.g., https://github.com/flexflow/FlexFlow/blob/411bc12dd47a8fe6fff3c434061cfae28f88dd88/lib/utils/include/utils/graph/labelled/output_labelled_open.h#L11-L12 (which both inherit eventually from NodeLabelledMultiDiGraphView, among others)

Also, I'm not sure I'd describe holding a cow_ptr as inheritance--describing the non-I classes as handles might be better?

---

lib/utils/include/utils/graph/docs/generate_diagram.py line 79 at r1 (raw file):

Previously, Marsella8 wrote…

I've ended up retooling the script to make it use hpp2plantuml as a python module instead of calling it through the command line so that it's easier to setup within nix, but I was unable to set it up (it's not present in nixpkgs, and had tried some other methods but was unsuccesful), let me know how to proceed.

@Bob-Chen222 Can you add a nix derivation for https://github.com/thibaultmarin/hpp2plantuml to the flake as part of this PR, similar to what we do with legion since legion isn't in nixpkgs? It's installable through pypi, so this should be pretty trivial--you can find examples in https://github.com/lockshaw/sp/blob/790e63952a06080ce9c4a888c5a5080dbd50f429/flake.nix#L20-L50

[Marsella8]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Bob-Chen222, @lockshaw, and @wmdi)

---

lib/utils/include/utils/graph/README.md line 244 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

I believe there are some, e.g., https://github.com/flexflow/FlexFlow/blob/411bc12dd47a8fe6fff3c434061cfae28f88dd88/lib/utils/include/utils/graph/labelled/output_labelled_open.h#L11-L12 (which both inherit eventually from NodeLabelledMultiDiGraphView, among others)

Also, I'm not sure I'd describe holding a cow_ptr as inheritance--describing the non-I classes as handles might be better?

Ah yes, thanks for the catch, I'll update the docs acccordingly

[lockshaw]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Bob-Chen222 and @wmdi)

[Bob-Chen222]

Reviewable status: 6 of 9 files reviewed, 1 unresolved discussion (waiting on @lockshaw and @wmdi)

---

lib/utils/include/utils/graph/docs/generate_diagram.py line 79 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

@Bob-Chen222 Can you add a nix derivation for https://github.com/thibaultmarin/hpp2plantuml to the flake as part of this PR, similar to what we do with legion since legion isn't in nixpkgs? It's installable through pypi, so this should be pretty trivial--you can find examples in https://github.com/lockshaw/sp/blob/790e63952a06080ce9c4a888c5a5080dbd50f429/flake.nix#L20-L50

Done. Turns out that hpp2plantuml only has wheel file on pypi, which took some time

[lockshaw]

Reviewed 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wmdi)