Add Inception-v3 model by Marsella8 · Pull Request #1495 · flexflow/flexflow-train

Marsella8 · 2024-09-10T23:43:23Z

Description of changes:

Related Issues:

Linked Issues:

Issue #

Issues closed by this PR:

Closes #

This change is

codecov · 2024-09-10T23:54:33Z

Codecov Report

Attention: Patch coverage is 91.20743% with 142 lines in your changes missing coverage. Please review.

Project coverage is 72.63%. Comparing base (6455415) to head (8a062aa).
Report is 1 commits behind head on repo-refactor.

Files with missing lines	Patch %	Lines
lib/op-attrs/src/op-attrs/ops/concat.cc	37.83%	46 Missing ⚠️
lib/op-attrs/src/op-attrs/ops/batch_norm.cc	83.33%	23 Missing ⚠️
...tation_graph/parallel_computation_graph_builder.cc	0.00%	20 Missing ⚠️
lib/op-attrs/src/op-attrs/ops/pool_2d.cc	87.50%	10 Missing ⚠️
...rc/substitutions/operator_pattern/get_attribute.cc	0.00%	10 Missing ⚠️
lib/models/src/models/inception_v3/inception_v3.cc	97.09%	7 Missing ⚠️
lib/op-attrs/src/op-attrs/get_output_shapes.cc	0.00%	5 Missing ⚠️
lib/op-attrs/test/src/op-attrs/ops/flat.cc	97.04%	5 Missing ⚠️
bin/export-model-arch/src/export_model_arch.cc	0.00%	4 Missing ⚠️
lib/op-attrs/test/src/op-attrs/ops/pool_2d.cc	98.49%	4 Missing ⚠️
... and 4 more

Additional details and impacted files

@@                Coverage Diff                @@
##           repo-refactor    #1495      +/-   ##
=================================================
+ Coverage          71.03%   72.63%   +1.60%     
=================================================
  Files                710      725      +15     
  Lines              21388    22967    +1579     
  Branches             615      676      +61     
=================================================
+ Hits               15192    16682    +1490     
- Misses              6196     6285      +89

Flag	Coverage Δ
unittests	`72.63% <91.20%> (+1.60%)`	⬆️

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

Files with missing lines	Coverage Δ
...odels/test/src/models/inception_v3/inception_v3.cc	`100.00% <100.00%> (ø)`
.../models/test/src/models/transformer/transformer.cc	`100.00% <ø> (ø)`
lib/op-attrs/include/op-attrs/dim_ordered/concat.h	`100.00% <100.00%> (ø)`
...p-attrs/include/op-attrs/dim_ordered/dim_ordered.h	`82.35% <100.00%> (+0.81%)`	⬆️
...include/op-attrs/dim_ordered/ff_ordered_from_map.h	`100.00% <100.00%> (ø)`
lib/op-attrs/include/op-attrs/dim_ordered/slice.h	`100.00% <ø> (ø)`
...op-attrs/src/op-attrs/get_incoming_tensor_roles.cc	`74.19% <100.00%> (+8.06%)`	⬆️
lib/op-attrs/src/op-attrs/ops/conv_2d.cc	`98.75% <100.00%> (-0.04%)`	⬇️
lib/op-attrs/src/op-attrs/ops/layer_norm.cc	`90.74% <ø> (ø)`
lib/op-attrs/src/op-attrs/parallel_tensor_dims.cc	`72.50% <100.00%> (+4.85%)`	⬆️
... and 32 more

... and 5 files with indirect coverage changes

lockshaw · 2024-09-11T00:20:58Z

@reyna-abhyankar Can you take care of the "implementation vs paper" checks for this PR? I'll work on getting the additional operators implemented

lockshaw

Reviewed 5 of 7 files at r1, 34 of 39 files at r2, 17 of 17 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Marsella8 and @reyna-abhyankar)

lib/models/include/models/inceptionv3.h line 5 at r1 (raw file):

#include "models/inceptionv3_config.dtg.h"
#include "pcg/computation_graph.h"

Nothing in the .h needs the functions declared in computation_graph.h

Suggestion:

#include "pcg/computation_graph.dtg.h"

lib/models/include/models/inceptionv3.h line 6 at r1 (raw file):

#include "models/inceptionv3_config.dtg.h"
#include "pcg/computation_graph.h"
#include "pcg/computation_graph_builder.h"

Move to .cc, nothing in the .h mentions ComputationGraphBuilder

lib/models/include/models/inceptionv3_config.struct.toml line 15 at r1 (raw file):

[[fields]]
name = "input_height"
type = "size_t"

Prefer ints for all of these

lib/models/src/models/inceptionv3.cc line 35 at r1 (raw file):

                                  std::nullopt,
                                  1,
                                  use_bias);

Argument name comments might be nice for some of these (like "1" and "std::nullopt")

Code quote:

                                  std::nullopt,
                                  1,
                                  use_bias);

lib/models/src/models/inceptionv3.cc line 130 at r1 (raw file):

  branch3x3dbl = cgb.concat(2, {branch3x3dbl_1, branch3x3dbl_2}, 3);

  tensor_guid_t branch_pool = cgb.pool2d(input, 3, 3, 1, 1, 1, 1, PoolOp::AVG);

Honestly might not be the worst idea to add argument comments for all of these raw number parameters for readability. Right now even if I had the order memorized there are so many that it's visually difficult to pick out which is which

lib/models/test/src/models/inception_v3/inception_v3.cc line 15 at r2 (raw file):

    SUBCASE("num layers") {
      // int result_num_layers = get_layers(result).size();
      int correct_num_layers = -1;

Checks should be added before merge

reyna-abhyankar

Reviewed all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Marsella8)

lib/models/src/models/inception_v3/inception_v3.cc line 154 at r3 (raw file):

                                  tensor_guid_t const &input,
                                  size_t num_classes) {
  tensor_guid_t x = cgb.pool2d(input, 8, 8, 1, 1, 0, 0, PoolOp::AVG);

So technically the module is adaptive avg pool, which I think means it reduces any input to the specified output size. The point of this is to deal with variable size inputs, which only happens if the inception_aux = true. I think you can probably just handle this manually (i.e. if (aux_logits) { ...__size A__...} else { ...__size B__...})

lib/models/src/models/inception_v3/inception_v3.cc line 175 at r3 (raw file):

  x = create_inception_module_c(cgb, x, 160);
  x = create_inception_module_c(cgb, x, 192);

Missing inception_aux: can add aux_logits in the config as such
https://github.com/pytorch/vision/blob/00e7fa164bfdfd302f0b471c18ce2fd6bd1a50bc/torchvision/models/inception.py#L78

https://pytorch.org/vision/0.8/_modules/torchvision/models/inception.html - is the code for the module

…n-v3-model

lockshaw

Reviewed 12 of 22 files at r4, 27 of 38 files at r6, 14 of 14 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @reyna-abhyankar)

lib/models/include/models/inceptionv3.h line 5 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Nothing in the .h needs the functions declared in computation_graph.h

Done.

lib/models/include/models/inceptionv3.h line 6 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Move to .cc, nothing in the .h mentions ComputationGraphBuilder

Done.

lib/models/include/models/inceptionv3_config.struct.toml line 15 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Prefer ints for all of these

Done.

lib/models/src/models/inceptionv3.cc line 35 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Argument name comments might be nice for some of these (like "1" and "std::nullopt")

Done.

lib/models/src/models/inceptionv3.cc line 130 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Honestly might not be the worst idea to add argument comments for all of these raw number parameters for readability. Right now even if I had the order memorized there are so many that it's visually difficult to pick out which is which

Done.

lib/models/src/models/inception_v3/inception_v3.cc line 154 at r3 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

So technically the module is adaptive avg pool, which I think means it reduces any input to the specified output size. The point of this is to deal with variable size inputs, which only happens if the inception_aux = true. I think you can probably just handle this manually (i.e. if (aux_logits) { ...__size A__...} else { ...__size B__...})

Added (partial) adaptive avg pool support

lib/models/src/models/inception_v3/inception_v3.cc line 175 at r3 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Missing inception_aux: can add aux_logits in the config as such
https://github.com/pytorch/vision/blob/00e7fa164bfdfd302f0b471c18ce2fd6bd1a50bc/torchvision/models/inception.py#L78

https://pytorch.org/vision/0.8/_modules/torchvision/models/inception.html - is the code for the module

Done.

lib/models/test/src/models/inception_v3/inception_v3.cc line 15 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Checks should be added before merge

Done.

lockshaw

Reviewed 23 of 23 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @reyna-abhyankar)

reyna-abhyankar

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Marsella8)

lib/op-attrs/src/op-attrs/ops/pool_2d.cc line 41 at r7 (raw file):

  int stride_h = kernel_h;
  int stride_w = kernel_w;

On the stack overflow post, I'm seeing the following formula:

stride = ind // outd
kernel_size = ind - (outd-1)*stride
padding = 0

lockshaw

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @reyna-abhyankar)

lib/op-attrs/src/op-attrs/ops/pool_2d.cc line 41 at r7 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

On the stack overflow post, I'm seeing the following formula:
stride = ind // outd
kernel_size = ind - (outd-1)*stride
padding = 0

ind - (outd - 1)*stride = ind - (outd - 1) * (ind / outd) = ind - ind + (ind /outd) = ind / outd = stride

reyna-abhyankar

Reviewable status: 68 of 69 files reviewed, all discussions resolved (waiting on @lockshaw)

lib/op-attrs/src/op-attrs/ops/pool_2d.cc line 41 at r7 (raw file):

Previously, lockshaw (Colin Unger) wrote…

ind - (outd - 1)*stride = ind - (outd - 1) * (ind / outd) = ind - ind + (ind /outd) = ind / outd = stride

Nice.

lockshaw

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Marsella8)

lib/op-attrs/src/op-attrs/ops/pool_2d.cc line 41 at r7 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Nice.

Added a comment documenting this.

inception v3 initial implementation

ede66f7

Marsella8 requested a review from lockshaw September 10, 2024 23:43

lockshaw requested a review from reyna-abhyankar September 11, 2024 00:19

lockshaw added 2 commits September 10, 2024 19:57

Add parallel shape inference for concat and pool2d

d724185

Format

cc16481

lockshaw requested changes Sep 11, 2024

View reviewed changes

reyna-abhyankar requested changes Sep 11, 2024

View reviewed changes

jiazhihao added the repo-refactor label Sep 12, 2024

lockshaw added 5 commits September 16, 2024 15:38

Respond to PR comments

97b19d8

Fix model bugs

de67ca4

Merge remote-tracking branch 'origin/repo-refactor' into add-inceptio…

610c331

…n-v3-model

Update batch norm to match pytorch interface for inception v3

fe31d8e

Finishing touches for inception, re-add relu flag for batchnorm

51fa6a2

lockshaw requested a review from reyna-abhyankar September 17, 2024 17:30

lockshaw reviewed Sep 17, 2024

View reviewed changes

lockshaw changed the title ~~inception v3 initial implementation~~ Add Inception-v3 model Sep 17, 2024

lockshaw mentioned this pull request Sep 17, 2024

Pool2D parallel shape inference lockshaw/FlexFlow#9

Closed

1 task

Format

e501aea

lockshaw reviewed Sep 17, 2024

View reviewed changes

reyna-abhyankar requested changes Sep 17, 2024

View reviewed changes

lockshaw requested a review from reyna-abhyankar September 17, 2024 17:55

lockshaw reviewed Sep 17, 2024

View reviewed changes

Document adaptive pool2d formula simplification

eabd822

reyna-abhyankar previously approved these changes Sep 17, 2024

View reviewed changes

lockshaw previously approved these changes Sep 17, 2024

View reviewed changes

lockshaw enabled auto-merge (squash) September 17, 2024 18:11

Format

8a062aa

lockshaw dismissed their stale review via 8a062aa September 17, 2024 18:37

lockshaw dismissed reyna-abhyankar’s stale review via 8a062aa September 17, 2024 18:37

lockshaw approved these changes Sep 17, 2024

View reviewed changes

lockshaw merged commit 6475ec7 into flexflow:repo-refactor Sep 17, 2024

lockshaw mentioned this pull request Sep 18, 2024

Add kernels/local-execution support for new BatchNormAttrs fields #1505

Open

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 Inception-v3 model#1495

Add Inception-v3 model#1495
lockshaw merged 11 commits intoflexflow:repo-refactorfrom
Marsella8:inceptionv3

Marsella8 commented Sep 10, 2024 •

edited by wmdi

Loading

Uh oh!

codecov bot commented Sep 10, 2024 •

edited

Loading

Uh oh!

lockshaw commented Sep 11, 2024

Uh oh!

lockshaw left a comment

Uh oh!

reyna-abhyankar left a comment

Uh oh!

lockshaw left a comment

Uh oh!

lockshaw left a comment

Uh oh!

reyna-abhyankar left a comment

Uh oh!

lockshaw left a comment

Uh oh!

reyna-abhyankar left a comment

Uh oh!

lockshaw left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Marsella8 commented Sep 10, 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 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lockshaw commented Sep 11, 2024

Uh oh!

lockshaw 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!

lockshaw 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Marsella8 commented Sep 10, 2024 •

edited by wmdi

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading