Skip to content

Rename intermediate values to _val and _const | feat(torchlib)#881

Merged
justinchuby merged 1 commit intomainfrom
gh/justinchuby/31/head
Jul 17, 2023
Merged

Rename intermediate values to _val and _const | feat(torchlib)#881
justinchuby merged 1 commit intomainfrom
gh/justinchuby/31/head

Conversation

@justinchuby
Copy link
Collaborator

@justinchuby justinchuby commented Jul 17, 2023 •
edited
Loading

Stack from ghstack (oldest at bottom):

The torchscript ONNX graph generator creates numeric value names by default (0, 1). These are not legal ONNX tensor names, since ONNX requires the names to be valid C variable names. This change updates the names by prepending a prefix _val_ or _const_ to make them valid ONNX names. It also improves readability by making the names less likely to be confused with shape values.

I decided to use the _ prefix to reduce the chance of name collision with FX names.

After:

<
   ir_version: 8,
   opset_import: ["" : 18],
   producer_name: "pytorch",
   producer_version: "2.1.0"
>
torch_jit (float[5,5,5,5] input_0, int64[2] input_1_3) => (float[5,5,5,2] _val_10) {
   _val_2 = Transpose <perm = [0, 1, 2, 3]> (input_0)
   _val_3 = Max (input_1_3)
   _val_4 = Shape <start = 0> (_val_3)
   _val_5 = Expand (input_1_3, _val_4)
   _const_6 = Constant <value = int64 {-1}> ()
   _val_7 = Unsqueeze (_val_5, _const_6)
   _val_8 = Concat <axis = -1> (_val_7)
   _val_9 = GatherND <batch_dims = 0> (_val_2, _val_8)
   _val_10 = Transpose <perm = [0, 1, 2, 3]> (_val_9)
}

Before:

<
   ir_version: 8,
   opset_import: ["" : 18],
   producer_name: "pytorch",
   producer_version: "2.1.0"
>
torch_jit (float[5,5,5,5] input_0, int64[2] input_1_3) => (float[5,5,5,2] 10) {
   2 = Transpose <perm = [0, 1, 2, 3]> (input_0)
   3 = Max (input_1_3)
   4 = Shape <start = 0> (3)
   5 = Expand (input_1_3, 4)
   6 = Constant <value = int64 {-1}> ()
   7 = Unsqueeze (5, 6)
   8 = Concat <axis = -1> (7)
   9 = GatherND <batch_dims = 0> (2, 8)
   10 = Transpose <perm = [0, 1, 2, 3]> (9)
}

@justinchuby justinchuby mentioned this pull request Jul 17, 2023
Merged
@justinchuby justinchuby requested a review from BowenBao July 17, 2023 17:09
justinchuby added a commit that referenced this pull request Jul 17, 2023
@justinchuby
Rename intermediate values to _val and _const | feat(torchlib)
5da12ed 
ghstack-source-id: 54bb9d9
Pull Request resolved: #881
@justinchuby justinchuby mentioned this pull request Jul 17, 2023
Merged
@BowenBao
Copy link
Contributor

BowenBao commented Jul 17, 2023

Could you describe motivation in description?

@codecov
Copy link

codecov bot commented Jul 17, 2023 •
edited
Loading

Codecov Report

Merging #881 (5bde197) into main (06a0a5c) will increase coverage by 0.01%.
The diff coverage is 76.19%.

@@            Coverage Diff             @@
##             main     #881      +/-   ##
==========================================
+ Coverage   76.56%   76.57%   +0.01%     
==========================================
  Files         112      112              
  Lines       13396    13413      +17     
  Branches     1346     1349       +3     
==========================================
+ Hits        10256    10271      +15     
+ Misses       2806     2805       -1     
- Partials      334      337       +3
Impacted Files Coverage Δ
...nxscript/function_libs/torch_lib/graph_building.py 82.24% <76.19%> (+0.31%) ⬆️

@justinchuby justinchuby changed the base branch from gh/justinchuby/31/base to main July 17, 2023 17:29
@justinchuby
Copy link
Collaborator Author

justinchuby commented Jul 17, 2023

Could you describe motivation in description?

Done

@justinchuby justinchuby added the module: torchlib Related to the torch/aten function lib in development label Jul 17, 2023
@justinchuby justinchuby merged commit 9c7de8a into main Jul 17, 2023
@justinchuby justinchuby deleted the gh/justinchuby/31/head branch July 17, 2023 18:10
justinchuby added a commit that referenced this pull request Jul 17, 2023
@justinchuby
Simplify model error message in test | test(torchlib) (#882)
bbdbf1e 
Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at
bottom):
* #883
* __->__ #882
* #881

Remove the duplicate check_model message when displaying
thiagocrepaldi
thiagocrepaldi reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@thiagocrepaldi thiagocrepaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change.

Would the distinction between _val and _const be ambiguous for the scenario where a _val node has a constant input, which would make the node have a constant output, but with the name _val that makes it look like a non-constant?

e.g. _val_1 = Max(_const_1) mas gets a _val_1 name, but max of constant is a constant, and thus the name could actually be _const_2?

If that is the case, all nodes could be called _val_ and prevent the semantic meaning of dynamic/constant

@justinchuby
Copy link
Collaborator Author

justinchuby commented Jul 17, 2023

That works too! I will update to val if no one thinks otherwise

@justinchuby justinchuby mentioned this pull request Jul 17, 2023
Merged
justinchuby added a commit that referenced this pull request Jul 22, 2023
@justinchuby @BowenBao
Change _const prefix to _val | feat(torchlib) (#886)
457383d 
Use `_val` for all intermediate values to avoid ambiguity based on
comment:
#881 (review)

Co-authored-by: Bowen Bao <bowbao@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@thiagocrepaldi thiagocrepaldi thiagocrepaldi left review comments

@BowenBao BowenBao BowenBao approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module: torchlib Related to the torch/aten function lib in development

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@justinchuby @BowenBao @thiagocrepaldi