Add interface for differentiating inputs and weights in CG & PCG by lockshaw · Pull Request #1493 · flexflow/flexflow-train

lockshaw · 2024-09-09T19:19:38Z

Description of changes:

Also do some PCG and op-attrs test suite organizing
Fix a bug in the transformer implementation (MHAs should not use bias)

Related Issues:

Linked Issues:

Issue #

Issues closed by this PR:

Closes #

This change is

codecov · 2024-09-09T19:41:45Z

Codecov Report

Attention: Patch coverage is 89.58904% with 76 lines in your changes missing coverage. Please review.

Project coverage is 71.03%. Comparing base (ae8bbf9) to head (5355085).
Report is 1 commits behind head on repo-refactor.

Files with missing lines	Patch %	Lines
...op-attrs/src/op-attrs/get_incoming_tensor_roles.cc	66.12%	21 Missing ⚠️
lib/pcg/src/pcg/computation_graph_builder.cc	80.00%	17 Missing ⚠️
...p-attrs/src/op-attrs/computation_graph_op_attrs.cc	0.00%	10 Missing ⚠️
...rc/substitutions/operator_pattern/get_attribute.cc	0.00%	8 Missing ⚠️
lib/pcg/test/src/pcg/computation_graph.cc	94.95%	6 Missing ⚠️
lib/local-execution/src/local_cost_estimator.cc	0.00%	3 Missing ⚠️
...tation_graph/parallel_computation_graph_builder.cc	76.92%	3 Missing ⚠️
lib/op-attrs/src/op-attrs/ops/topk.cc	0.00%	2 Missing ⚠️
lib/op-attrs/src/op-attrs/ops/attention.cc	87.50%	1 Missing ⚠️
lib/op-attrs/src/op-attrs/ops/conv_2d.cc	85.71%	1 Missing ⚠️
... and 4 more

Additional details and impacted files

@@                Coverage Diff                @@
##           repo-refactor    #1493      +/-   ##
=================================================
+ Coverage          70.45%   71.03%   +0.57%     
=================================================
  Files                705      710       +5     
  Lines              20861    21388     +527     
  Branches             607      615       +8     
=================================================
+ Hits               14698    15192     +494     
- Misses              6163     6196      +33

Flag	Coverage Δ
unittests	`71.03% <89.58%> (+0.57%)`	⬆️

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

Files with missing lines	Coverage Δ
lib/kernels/include/kernels/legion_dim.h	`28.57% <ø> (ø)`
lib/models/src/models/transformer/transformer.cc	`98.88% <100.00%> (ø)`
...p-attrs/include/op-attrs/dim_ordered/dim_ordered.h	`81.53% <ø> (ø)`
.../op-attrs/include/op-attrs/dim_ordered/enumerate.h	`100.00% <ø> (ø)`
...attrs/include/op-attrs/dim_ordered/ff_ordered_of.h	`100.00% <ø> (ø)`
...b/op-attrs/include/op-attrs/dim_ordered/get_idxs.h	`100.00% <ø> (ø)`
lib/op-attrs/include/op-attrs/dim_ordered/slice.h	`100.00% <ø> (ø)`
.../op-attrs/include/op-attrs/dim_ordered/transform.h	`100.00% <ø> (ø)`
lib/op-attrs/include/op-attrs/dim_ordered/zip.h	`100.00% <ø> (ø)`
lib/op-attrs/src/op-attrs/pcg_operator_attrs.cc	`36.84% <100.00%> (+31.07%)`	⬆️
... and 41 more

reyna-abhyankar

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

lib/models/src/models/transformer.cc line 93 at r1 (raw file):

                                                         vdim,
                                                         config.dropout,
                                                         /*bias=*/false);

Should we make bias false by default?

lib/op-attrs/include/op-attrs/ops/batch_matmul.h line 12 at r1 (raw file):

namespace FlexFlow {

CHECK_VALID_OP_ATTR(BatchMatmulAttrs);

What does this do / was there any reason it wasn't there before?

lib/pcg/include/pcg/computation_graph_builder.h line 166 at r1 (raw file):

      std::optional<InitializerAttrs> const &bias_initializer = std::nullopt,
      std::optional<std::string> const &name = std::nullopt,
      std::optional<std::string> const &projection_name = std::nullopt,

why are names necessary?

lib/pcg/src/pcg/computation_graph.cc line 25 at r1 (raw file):

}

LayerAddedResult add_layer(ComputationGraph &computation_graph,

Why a new interface?

lib/op-attrs/test/src/op-attrs/ops/conv_2d.cc line 23 at r1 (raw file):

    SUBCASE("with bias") {
      Conv2DAttrs attrs = make_attrs(true);

Suggestion:

make_attrs(/*use_bias=*/true)

lib/op-attrs/test/src/op-attrs/ops/conv_2d.cc line 37 at r1 (raw file):

    SUBCASE("without bias") {
      Conv2DAttrs attrs = make_attrs(false);

Suggestion:

make_attrs(/*use_bias=*/false)

lockshaw

Reviewable status: 74 of 75 files reviewed, 6 unresolved discussions (waiting on @reyna-abhyankar and @wmdi)

lib/models/src/models/transformer.cc line 93 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Should we make bias false by default?

Probably not, since bias=true is the pytorch default: https://pytorch.org/docs/stable/generated/torch.nn.MultiheadAttention.html

lib/op-attrs/include/op-attrs/ops/batch_matmul.h line 12 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

What does this do / was there any reason it wasn't there before?

It just checks a couple basic properties (mainly just value/copy semantics) that honestly probably aren't all that necessary anymore now that we have an actual testing framework in place. I'll leave it in for now for consistency with the other operators, but I've created an issue to eventually go and remove them: #1498

lib/pcg/include/pcg/computation_graph_builder.h line 166 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

why are names necessary?

For testing--currently names are the way to retrieve layer_guid_ts from a ComputationGraph built by ComputationGraphBuilder--see https://github.com/lockshaw/FlexFlow/blob/3440b3e669943b69320b092fdd1e2975dbbf5496/lib/pcg/test/src/pcg/computation_graph.cc#L155-L204 for an example.

Currently there's a bit of awkwardness in that ComputationGraphBuilder is a bit high-level for a lot of tests (has the ability to auto-insert cast and broadcast operators, auto-create weights, etc. which makes getting layer_guid_ts and tensor_guid_ts a bit difficult and makes reasoning about what a test actually does a bit more challenging than it should be), but the low-level graph interface is often way too verbose. The solution is likely to add an interface in the middle (between ComputationGraphBuilder and the raw graph interface--tracked in #1499 and somewhat related to #1474), but I'm not yet ready to implement it, so for now we're using a balance of the two interfaces while I figure out a better long-term solution

lib/pcg/src/pcg/computation_graph.cc line 25 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Why a new interface?

This interface actually has been there for a while, it just wasn't implemented. That said, there is good reason for having interfaces between the raw graph interface and ComputationGraphBuilder, as outlined in https://reviewable.io/reviews/flexflow/FlexFlow/1493#-O6UVfTk8CqARIr-WYgQ

lib/op-attrs/test/src/op-attrs/ops/conv_2d.cc line 23 at r1 (raw file):

    SUBCASE("with bias") {
      Conv2DAttrs attrs = make_attrs(true);

Done.

lib/op-attrs/test/src/op-attrs/ops/conv_2d.cc line 37 at r1 (raw file):

    SUBCASE("without bias") {
      Conv2DAttrs attrs = make_attrs(false);

Done.

wmdi

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @reyna-abhyankar)

reyna-abhyankar

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

lib/pcg/include/pcg/computation_graph_builder.h line 166 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

For testing--currently names are the way to retrieve layer_guid_ts from a ComputationGraph built by ComputationGraphBuilder--see https://github.com/lockshaw/FlexFlow/blob/3440b3e669943b69320b092fdd1e2975dbbf5496/lib/pcg/test/src/pcg/computation_graph.cc#L155-L204 for an example.

Currently there's a bit of awkwardness in that ComputationGraphBuilder is a bit high-level for a lot of tests (has the ability to auto-insert cast and broadcast operators, auto-create weights, etc. which makes getting layer_guid_ts and tensor_guid_ts a bit difficult and makes reasoning about what a test actually does a bit more challenging than it should be), but the low-level graph interface is often way too verbose. The solution is likely to add an interface in the middle (between ComputationGraphBuilder and the raw graph interface--tracked in #1499 and somewhat related to #1474), but I'm not yet ready to implement it, so for now we're using a balance of the two interfaces while I figure out a better long-term solution

Yeah, that's fair.

…sor-roles

Add interface for differentiating inputs and weights in CG/PCG

a32030f

lockshaw added the repo-refactor label Sep 9, 2024

Format

3440b3e

lockshaw requested review from reyna-abhyankar and wmdi September 9, 2024 20:24

lockshaw marked this pull request as ready for review September 9, 2024 20:30

lockshaw mentioned this pull request Sep 11, 2024

Add num_inputs check to get_output_shapes(PCGOperatorAttrs, std::vector<ParallelTensorShape>) #1496

Open

reyna-abhyankar requested changes Sep 11, 2024

View reviewed changes

Address Reyna PR comments

b8d5ec3

lockshaw commented Sep 12, 2024

View reviewed changes

lockshaw mentioned this pull request Sep 12, 2024

Unity device mapping algorithm #1459

Merged

wmdi previously approved these changes Sep 14, 2024

View reviewed changes

reyna-abhyankar previously approved these changes Sep 15, 2024

View reviewed changes

lockshaw added 3 commits September 16, 2024 09:57

Merge remote-tracking branch 'origin/repo-refactor' into incoming-ten…

aa8e4fc

…sor-roles

fix bugs from merge

5cf7a40

Format

5355085

lockshaw dismissed stale reviews from reyna-abhyankar and wmdi via 5355085 September 16, 2024 17:24

lockshaw merged commit 6455415 into flexflow:repo-refactor Sep 16, 2024

lockshaw deleted the incoming-tensor-roles branch September 16, 2024 18:17

lockshaw mentioned this pull request Oct 9, 2024

Add function to divide list of PCG inputs into inputs and weights in op-attrs #1469

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add interface for differentiating inputs and weights in CG & PCG#1493

Add interface for differentiating inputs and weights in CG & PCG#1493
lockshaw merged 6 commits intoflexflow:repo-refactorfrom
lockshaw:incoming-tensor-roles

lockshaw commented Sep 9, 2024 •

edited by wmdi

Loading

Uh oh!

codecov bot commented Sep 9, 2024 •

edited

Loading

Uh oh!

reyna-abhyankar left a comment

Uh oh!

lockshaw left a comment

Uh oh!

wmdi left a comment

Uh oh!

reyna-abhyankar left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lockshaw commented Sep 9, 2024 • edited by wmdi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

reyna-abhyankar 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!

wmdi left a comment

Choose a reason for hiding this comment

Uh oh!

reyna-abhyankar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lockshaw commented Sep 9, 2024 •

edited by wmdi

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading