Add first draft of docs for visitable by lockshaw · Pull Request #890 · flexflow/flexflow-train

lockshaw · 2023-07-27T09:06:10Z

Description of changes:

Add high-level documentation of visitable with lots of examples and motivation.

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

lockshaw · 2023-07-27T09:11:10Z

https://reviewable.io/reviews/flexflow/FlexFlow/890#-

KateUnger · 2023-07-29T04:49:29Z

lib/utils/README.md

-FlexFlow's codebase makes heavy use of "plain old data" types[^1] such as the following:
+### Motivation
+
+FlexFlow's codebase makes heavy use of "plain old data"[^2] types[^1] (referred to these as _product types_ in the rest of this document) such as the following:


"referred to as product types in the rest of this document" - delete 'these'

KateUnger · 2023-07-29T04:52:02Z

lib/utils/README.md

+  int age;
+};
+```
+However, this standard implementation defines a set of behaviors that we (i.e., the FlexFlow developers) find undesirable.


"the FlexFlow developers) find undesirable:

Default..."

Remove `these are as follows"

KateUnger · 2023-07-29T04:55:20Z

lib/utils/README.md

+  return p;
+}
+```
+If the `if` branch is not taken, we will return a `Person` with nonsensical values, and there do not exist any values that naturally form a default.


I like the explanation below more than the function (to me it's more straight-forward). I would recommend moving the if example below the struct example.

KateUnger · 2023-07-29T05:04:25Z

lib/utils/README.md

+
+Person p{"donald", "knuth", 85};
+```
+would compile without any errors, but is (as of writing this) incorrect.


I like these examples (the ones for 2.)

KateUnger · 2023-07-29T05:07:24Z

lib/utils/README.md

+  int age;
+};
 ```
+If we take the previous example of adding an additional `is_male` field to `Person`, it can be easy to miss a location, leading to incorrectness. 


Good examples, but maybe reword to " it can be easy to forget to update the operators."

KateUnger · 2023-07-29T05:11:42Z

lib/utils/README.md

+```
+and for product types with more fields this grows increasingly tedious to write and maintain. 
+
+4. Hashing: hashing for product types is also relatively trivial, as long as each of the fields is hashable. But again, we have to do a bunch of extra work to specify this, and this work has to be done for each product type in the codebase.


Reword to "Hashing product types is relatively trivial, long as each of the fields..."

KateUnger · 2023-07-29T05:15:45Z

lib/utils/README.md

+
+}
+```
+and if we also want to support, say, `std::set<Person>`, we also have to add `operator<`


I might be missing smt, but what does this operator example have to do with hashing?

KateUnger · 2023-07-29T05:28:53Z

lib/utils/README.md

+Don't worry if you forget to add a `req`: `FF_VISITABLE_STRUCT` will check that your type properly disables default and partial construction.
+Combined with [aggregate initialization](https://en.cppreference.com/w/cpp/language/aggregate_initialization), we are able to construct a Person as follows:
+```cpp
+Person p = { "donald", "knuth", 85, true };


Don't feel like this adds much, prob doesnt hurt to leave in tho

KateUnger · 2023-07-29T05:33:46Z

lib/utils/README.md

+
+using MyInts = MyLists<int>;
+
+FF_VISITABLE_STRUCT(MyInts, list1, list2); // CORRECT


An additional ex would help here

KateUnger · 2023-07-29T05:37:06Z

lib/utils/README.md

+
+}
+```
+which is tedious and bug-prone.


this is a pretty long example just to say its tedious, that said it gets the point across if you cant think of another way to do so

wmdi

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @lockshaw and @tarunprabhu)

lib/utils/README.md line 261 at r1 (raw file):

Conceptually, `req` is simple: it removes default constructibility of the type it wraps (if the last field in the struct is already not default-constructible, no `req` is needed).
Don't worry if you forget to add a `req`: `FF_VISITABLE_STRUCT` will check that your type properly disables default and partial construction.
Combined with [aggregate initialization](https://en.cppreference.com/w/cpp/language/aggregate_initialization), we are able to construct a Person as follows:

Suggestion:

Combined with [aggregate initialization](https://en.cppreference.com/w/cpp/language/aggregate_initialization), we are able to construct a `Person` as follows:

lib/utils/README.md line 265 at r1 (raw file):

Person p = { "donald", "knuth", 85, true };
and any subset of the fields would raise an error at compile time.

Should also mention hashing here.

tarunprabhu

It might be obvious, and it is certainly not relevant to the documentation, but if there are ways to disable default construction of an object in C++, why go down this route? The cases where one needs to have an object be default constructible usually arise with deserialization where one often needs to construct a default object and then initialize the fields with the deserialized values. If we will only ever serialize types over which we have complete control, that same effect can be obtained using a static create method and a protected default constructor. Of course, you have probably thought a lot longer about this, so consider this my <<2 cents.

Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @KateUnger and @lockshaw)

lib/utils/README.md line 15 at r1 (raw file):

Previously, KateUnger (Kate Unger) wrote…

"the FlexFlow developers) find undesirable:

Default..."

Remove `these are as follows"

Replace parentheses and i.e. with commas.

... a set of behaviors that we, the FlexFlow developers, find undesirable.

lib/utils/README.md line 28 at r1 (raw file):

Previously, KateUnger (Kate Unger) wrote…

I like the explanation below more than the function (to me it's more straight-forward). I would recommend moving the if example below the struct example.

I agree. Just explaining why leaving the fields at their default values may be semantically invalid is sufficient.

lib/utils/README.md line 36 at r1 (raw file):

  int age = 0;
}

Would it be possible to have an example more relevant to FlexFlow users or developers? Just a thought. This example does well enough to explain why we may want objects to be default constructible. A more relevant example may be more compelling.

Code quote:

```cpp
struct Person {
  std::string first_name; // initializes to ""
  std::string last_name; // initializes to ""
  int age = 0;
}

___
*[`lib/utils/README.md` line 121 at r1](https://reviewable.io/reviews/flexflow/FlexFlow/890#-Nah_7UH2k65ft6a98Y-:-Nah_7UH2k65ft6a98Y0:b-5k1eyz) ([raw file](https://github.com/flexflow/FlexFlow/blob/85ca60bef191e36ef7975e7d42217f5f6108dc91/lib/utils/README.md#L121)):*
> ```Markdown
>       || lhs.last_name != rhs.last_name
>       || lhs.age != rhs.age;
>       // oops, forgot to update here. Have fun debugging :P
> ```

Perhaps mention explicitly that `is_male` was not updated.



_Code quote:_
```Markdown
  // oops, forgot to update here. Have fun debugging :P

lib/utils/README.md line 237 at r1 (raw file):

}
For a datatype that conceptually is as simple as three independent fields, we now have an rather significant amount of code that must be written and maintained.

replace with "a significant amount"

Code quote:

an rather significant amount

lib/utils/README.md line 237 at r1 (raw file):

}
For a datatype that conceptually is as simple as three independent fields, we now have an rather significant amount of code that must be written and maintained.

Consider rewording. E.g. "Even for such a simple datatype, we have a significant amount of code ..."

Code quote:

For a datatype that conceptually is as simple as three independent fields

lib/utils/README.md line 257 at r1 (raw file):

The key addition here is the calling the FF_VISITABLE_STRUCT macro.
In addition to defining all of the above functions, this macro also performs a series of static_asserts to check that the product type is implemented correctly (for example, it will check that the type is not default constructible).

Consider replacing with "compile-time checks".
... a series of compile-time checks to ensure that the product type ...

Code quote:

`static_assert`s

lib/utils/README.md line 257 at r1 (raw file):

The key addition here is the calling the FF_VISITABLE_STRUCT macro.
In addition to defining all of the above functions, this macro also performs a series of static_asserts to check that the product type is implemented correctly (for example, it will check that the type is not default constructible).

Does it do more than just check if the type is not default constructible? If so, would it be worth listing all the properties that the type is expected to have? I am adding review comments from the bottom up, so this probably belongs earlier in the document, but if there is a set of properties that all product types must possess, it should be documented somewhere. If it is, then perhaps there should be a link to the list of properties here so the reader can be clear about what properties the macro checks.

Code quote:

is implemented correctly (for example, it will check that the type is not default constructible).

lib/utils/README.md line 259 at r1 (raw file):

In addition to defining all of the above functions, this macro also performs a series of `static_assert`s to check that the product type is implemented correctly (for example, it will check that the type is not default constructible).
The only additional change is the addition of the `req` (which stands for `required`) wrapper on the last field. 
Conceptually, `req` is simple: it removes default constructibility of the type it wraps (if the last field in the struct is already not default-constructible, no `req` is needed).

It feels as if req is a workaround to force a type to not be default constructable. Does req also act as a marker to indicate that some field of a type may not be semantically valid if initialized with a default value? If so, would it be reasonable advice to require it on all such fields?

Code quote:

Conceptually, `req` is simple: it removes default constructibility of the type it wraps (if the last field in the struct is already not default-constructible, no `req` is needed).

lib/utils/README.md line 351 at r1 (raw file):

Previously, KateUnger (Kate Unger) wrote…

this is a pretty long example just to say its tedious, that said it gets the point across if you cant think of another way to do so

I agree with this comment. You could just list the operators that would need to be implemented, and provide a reference to the previous Person example where that was done.

lib/utils/README.md line 352 at r1 (raw file):

which is tedious and bug-prone.
To remove the constructibility checks performed by FF_VISITABLE_STRUCT, we simply use FF_VISITABLE_STRUCT_NONSTANDARD_CONSTRUCTION instead:

You may want to state that this is also necessary for cases where the product type requires a constructor with a specific signature i.e. it is not a copy or move constructor.

lockshaw

I'm not sure I 100% get what you're suggesting, so let me know if the response below leaves something out:

Overall, I'm not against other setups that use static methods, private/protected constructors, etc. The goal of visitable here is really just to make the most desirable behavior (at least in the flexflow context) the easiest to implement. As the example show below, it is possible to get this behavior without using visitable, but it requires 10s-100s of lines of code per struct as opposed to only a couple lines of code, and so in practice the correct behavior will rarely be implemented (even if everyone agrees it should be). In addition, FF_VISITABLE_STRUCT bundles not only the correct behaviors, but checks to make sure nothing has been missed, whereas manually implementing things makes it very easy to introduce bugs that fail silently at runtime.

Reviewable status: 0 of 1 files reviewed, 21 unresolved discussions (waiting on @KateUnger, @tarunprabhu, and @wmdi)

lib/utils/README.md line 7 at r1 (raw file):

Previously, KateUnger (Kate Unger) wrote…

"referred to as product types in the rest of this document" - delete 'these'

Done.

lib/utils/README.md line 15 at r1 (raw file):

Previously, tarunprabhu (Tarun Prabhu) wrote…

Replace parentheses and i.e. with commas.

... a set of behaviors that we, the FlexFlow developers, find undesirable.

Done.

lib/utils/README.md line 28 at r1 (raw file):

Previously, tarunprabhu (Tarun Prabhu) wrote…

I agree. Just explaining why leaving the fields at their default values may be semantically invalid is sufficient.

Done.

lib/utils/README.md line 36 at r1 (raw file):

Previously, tarunprabhu (Tarun Prabhu) wrote…

Would it be possible to have an example more relevant to FlexFlow users or developers? Just a thought. This example does well enough to explain why we may want objects to be default constructible. A more relevant example may be more compelling.

Maybe, but I wanted to keep the examples as approachable as possible--if people want FlexFlow-relevant examples, they can always just read the FlexFlow code, and adding more complexity just prevents more people from understanding it (and visitable has nothing to do with FlexFlow except for the fact that we use it--it has not fundamental link to machine learning or distributed systems, etc.)

lib/utils/README.md line 64 at r1 (raw file):

Previously, KateUnger (Kate Unger) wrote…

I like these examples (the ones for 2.)

Done.

lib/utils/README.md line 95 at r1 (raw file):

Previously, KateUnger (Kate Unger) wrote…

Good examples, but maybe reword to " it can be easy to forget to update the operators."

Done.

lib/utils/README.md line 121 at r1 (raw file):

Previously, tarunprabhu (Tarun Prabhu) wrote…

Perhaps mention explicitly that is_male was not updated.

Done.

lib/utils/README.md line 121 at r1 (raw file):

Previously, KateUnger (Kate Unger) wrote…

Helpful comment

Done.

lib/utils/README.md line 132 at r1 (raw file):

Previously, KateUnger (Kate Unger) wrote…

Reword to "Hashing product types is relatively trivial, long as each of the fields..."

Done.

lib/utils/README.md line 181 at r1 (raw file):

Previously, KateUnger (Kate Unger) wrote…

I might be missing smt, but what does this operator example have to do with hashing?

std::unordered_set uses hashing, and std:;set uses ordering (i.e., operator<), so if we want to support both types of set we need more than just hashing

lib/utils/README.md line 237 at r1 (raw file):

Previously, tarunprabhu (Tarun Prabhu) wrote…

replace with "a significant amount"

Done.

lib/utils/README.md line 237 at r1 (raw file):

Previously, tarunprabhu (Tarun Prabhu) wrote…

Consider rewording. E.g. "Even for such a simple datatype, we have a significant amount of code ..."

Done.

lib/utils/README.md line 257 at r1 (raw file):

Previously, tarunprabhu (Tarun Prabhu) wrote…

Consider replacing with "compile-time checks".
... a series of compile-time checks to ensure that the product type ...

Done.

lib/utils/README.md line 257 at r1 (raw file):

Previously, tarunprabhu (Tarun Prabhu) wrote…

Does it do more than just check if the type is not default constructible? If so, would it be worth listing all the properties that the type is expected to have? I am adding review comments from the bottom up, so this probably belongs earlier in the document, but if there is a set of properties that all product types must possess, it should be documented somewhere. If it is, then perhaps there should be a link to the list of properties here so the reader can be clear about what properties the macro checks.

Done.

lib/utils/README.md line 259 at r1 (raw file):

It feels as if req is a workaround to force a type to not be default constructable.

Yes, this is exactly correct.

If so, would it be reasonable advice to require it on all such fields?

It's simply not necessary--since only aggregate initialization is allowed in constructing visitable types, as long as the last field is not default constructible the correct behavior is achieved. (also, req complicates things such as error messages, and why make error messages worse if you don't have to)

lib/utils/README.md line 261 at r1 (raw file):

Conceptually, `req` is simple: it removes default constructibility of the type it wraps (if the last field in the struct is already not default-constructible, no `req` is needed).
Don't worry if you forget to add a `req`: `FF_VISITABLE_STRUCT` will check that your type properly disables default and partial construction.
Combined with [aggregate initialization](https://en.cppreference.com/w/cpp/language/aggregate_initialization), we are able to construct a Person as follows:

Done.

lib/utils/README.md line 263 at r1 (raw file):

Previously, KateUnger (Kate Unger) wrote…

Don't feel like this adds much, prob doesnt hurt to leave in tho

I think it's best to leave in--it's nice to have an example of what to do, rather than just a bunch of examples of what not to do

lib/utils/README.md line 265 at r1 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

Should also mention hashing here.

What specifically do you think I should mention here? Just that Person is hashable? I've added in some additional text, let me know if you think it's sufficient

lib/utils/README.md line 285 at r1 (raw file):

Previously, KateUnger (Kate Unger) wrote…

An additional ex would help here

What aspect of this limitation do you think is insufficiently covered by the existing example? Happy to add more, but I'm not sure what would be good to add

lib/utils/README.md line 351 at r1 (raw file):

Previously, tarunprabhu (Tarun Prabhu) wrote…

I agree with this comment. You could just list the operators that would need to be implemented, and provide a reference to the previous Person example where that was done.

Added in a disclaimer so people can just skim the code if they want

lib/utils/README.md line 352 at r1 (raw file):

Previously, tarunprabhu (Tarun Prabhu) wrote…

You may want to state that this is also necessary for cases where the product type requires a constructor with a specific signature i.e. it is not a copy or move constructor.

Done.

tarunprabhu

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @KateUnger)

wmdi

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @KateUnger)

KateUnger

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lockshaw)

lib/utils/README.md line 285 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

What aspect of this limitation do you think is insufficiently covered by the existing example? Happy to add more, but I'm not sure what would be good to add

Rereading it, I think you're good

Add first draft of docs for visitable

85ca60b

lockshaw added the repo-refactor label Jul 27, 2023

lockshaw requested review from KateUnger and wmdi July 27, 2023 09:09

lockshaw requested a review from tarunprabhu July 27, 2023 21:36

KateUnger reviewed Jul 29, 2023

View reviewed changes

wmdi requested changes Jul 29, 2023

View reviewed changes

tarunprabhu reviewed Jul 31, 2023

View reviewed changes

lockshaw self-assigned this Aug 8, 2023

lockshaw added 2 commits August 17, 2023 03:40

Address PR comments

b1b212f

Merge remote-tracking branch 'origin/repo-refactor' into visitable-docs

6a1ea68

lockshaw commented Aug 17, 2023

View reviewed changes

tarunprabhu approved these changes Aug 18, 2023

View reviewed changes

wmdi approved these changes Aug 18, 2023

View reviewed changes

KateUnger reviewed Aug 19, 2023

View reviewed changes

lockshaw merged commit 47b2a7f into flexflow:repo-refactor Aug 20, 2023

lockshaw deleted the visitable-docs branch August 20, 2023 01:45

This was referenced Jan 15, 2025

Merge substitution-builder #1575

Merged

Change OpCostMetrics.memory to be a nonnegative_int (Issue 1584) #1585

Closed

Change OpCostMetrics.memory to be a nonnegative_int #1586

Merged

victorli2002 mentioned this pull request Feb 7, 2025

Adding to unity substitution set #1594

Closed

victorli2002 mentioned this pull request Mar 12, 2025

MCMC #1601

Closed

victorli2002 mentioned this pull request Apr 21, 2025

fix substitution #1606

Closed


		using MyInts = MyLists<int>;

		FF_VISITABLE_STRUCT(MyInts, list1, list2); // CORRECT

Conversation

lockshaw commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lockshaw commented Jul 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmdi left a comment

Choose a reason for hiding this comment

Uh oh!

tarunprabhu left a comment

Choose a reason for hiding this comment

Uh oh!

lockshaw left a comment

Choose a reason for hiding this comment

Uh oh!

tarunprabhu left a comment

Choose a reason for hiding this comment

Uh oh!

wmdi left a comment

Choose a reason for hiding this comment

Uh oh!

KateUnger left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lockshaw commented Jul 27, 2023 •

edited

Loading