Merge `attrs` and `attr_protos` in `IRFunction` | chore(irbuilder) #625

justinchuby · 2023-04-12T17:53:20Z

Stack from ghstack (oldest at bottom):

Merge the two list in IRFunction by changing its type to IRAttributeParameter. This way we retain type information for all attributes. It is useful for creating correct OpSchemas and ParamSchemas in the next PR.

Also Include typeinfo in add_attr_parameter.

Move version_utils to `_internal` so that it can be used my onnxscript [ghstack-poisoned]

[ghstack-poisoned]

ghstack-source-id: 3582671 Pull Request resolved: #625

Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at bottom): * #626 * #625 * __->__ #624 Move version_utils to `_internal` so that it can be used my onnxscript

titaiwangms · 2023-04-12T18:15:54Z

The changes aren't stacked? It seems being piled up to the next commit?

justinchuby · 2023-04-12T18:17:19Z

The changes aren't stacked? It seems being piled up to the next commit?

That's the thing I talked about with ghstack and how we use github to merge which is annoying. I changed the base branch back. Try now?

titaiwangms · 2023-04-12T18:21:47Z

The changes aren't stacked? It seems being piled up to the next commit?

That's the thing I talked about with ghstack and how we use github to merge which is annoying. I changed the base branch back. Try now?

Working!

…tion` | chore(irbuilder)" Merge the two list in `IRFunction` by adding a `has_default` field in `IRAttributeValue`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…builder)" Merge the two list in `IRFunction` by adding a `has_default` field in `IRAttributeValue`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

codecov · 2023-04-12T21:02:55Z

Codecov Report

Merging #625 (6c54f07) into main (9bde82d) will decrease coverage by 0.15%.
The diff coverage is 77.35%.

@@            Coverage Diff             @@
##             main     #625      +/-   ##
==========================================
- Coverage   73.95%   73.81%   -0.15%     
==========================================
  Files         107      107              
  Lines       11389    11390       +1     
  Branches     1185     1185              
==========================================
- Hits         8423     8407      -16     
- Misses       2645     2657      +12     
- Partials      321      326       +5

Impacted Files	Coverage Δ
onnxscript/converter.py	`81.74% <ø> (-1.20%)`	⬇️
onnxscript/type_annotation.py	`71.62% <0.00%> (-1.36%)`	⬇️
onnxscript/values.py	`91.35% <60.00%> (-0.36%)`	⬇️
onnxscript/irbuilder.py	`75.17% <80.00%> (-1.78%)`	⬇️
onnxscript/converter_test.py	`84.39% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

…tion` | chore(irbuilder)" Merge the two list in `IRFunction` by adding a `has_default` field in `IRAttributeValue`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…builder)" Merge the two list in `IRFunction` by adding a `has_default` field in `IRAttributeValue`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…tion` | chore(irbuilder)" Merge the two list in `IRFunction` by adding a `has_default` field in `IRAttributeValue`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…builder)" Merge the two list in `IRFunction` by adding a `has_default` field in `IRAttributeValue`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

gramalingam · 2023-04-12T23:05:57Z

onnxscript/irbuilder.py

@@ -108,10 +108,16 @@ def _opt_var_to_str(x):


 class IRAttributeValue:
-    """An attribute value (representing an actual parameter)."""
+    """An attribute value (representing an actual parameter).


With the current changes, this class is being used to cover both actual parameters (which are values) as well as formal parameters (with or without a default-value). I am not 100% sure if we should unify all of these into a single type/class. But if we do, may be the name should be changed to reflect this? May be IRAttributeParameter ? And, of course, the above documentation line should be changed to actual+formal parameter.

For example, should we add an assertion/check in the IRStmt init method that all attrs have a default-value?

Thanks! I added IRAttributeParameter. I wonder if it should be a subclass of IRAttributeValue or be its own class? For now I subclassed but semantically should it be that?

I made an update on IRAttributeParameter. PTAL

Yes, semantically it should not be a subclass. The current form is preferable.

…tion` | chore(irbuilder)" Merge the two list in `IRFunction` by adding a `has_default` field in `IRAttributeValue`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…builder)" Merge the two list in `IRFunction` by adding a `has_default` field in `IRAttributeValue`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…tion` | chore(irbuilder)" Merge the two list in `IRFunction` by adding a `has_default` field in `IRAttributeValue`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…builder)" Merge the two list in `IRFunction` by adding a `has_default` field in `IRAttributeValue`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…tion` | chore(irbuilder)" Merge the two list in `IRFunction` by changing its type to `IRAttributeParameter`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…builder)" Merge the two list in `IRFunction` by changing its type to `IRAttributeParameter`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…tion` | chore(irbuilder)" Merge the two list in `IRFunction` by changing its type to `IRAttributeParameter`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…builder)" Merge the two list in `IRFunction` by changing its type to `IRAttributeParameter`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…tion` | chore(irbuilder)" Merge the two list in `IRFunction` by changing its type to `IRAttributeParameter`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…builder)" Merge the two list in `IRFunction` by changing its type to `IRAttributeParameter`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

gramalingam · 2023-04-13T23:05:55Z

onnxscript/irbuilder.py

+
+    It may or may not carry a default value.
+
+    Attributes:


Is this out-dated, or is this the accepted convention? That is, should this list be "name, type, default_value" or as below?

I should add the other attributes. thanks for catching

I don't see the changes. Not sure if something's wrong with what I am seeing, or the change was not pushed.

Is this the same view you saw?

Wait so was this good or did you mean something else?

I guess I am trying to understand the conventions here. The current form makes sense as the "public interface" exposed. Technically, however, default_value is also an attribute. Is the idea that it is "private"? Would renaming default_value to _default_value be appropriate?

Added default_value. Thanks!

onnxscript/irbuilder.py

…tion` | chore(irbuilder)" Merge the two list in `IRFunction` by changing its type to `IRAttributeParameter`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…builder)" Merge the two list in `IRFunction` by changing its type to `IRAttributeParameter`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…tion` | chore(irbuilder)" Merge the two list in `IRFunction` by changing its type to `IRAttributeParameter`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

…builder)" Merge the two list in `IRFunction` by changing its type to `IRAttributeParameter`. This way we retain type information for all attributes. It is useful for creating correct `OpSchema`s and `ParamSchema`s in the next PR. Also Include `typeinfo` in `add_attr_parameter`. [ghstack-poisoned]

ghstack-source-id: 24c2201 Pull Request resolved: microsoft/onnxscript#625 Signed-off-by: Justin Chu <[email protected]>

justinchuby added 2 commits April 12, 2023 17:30

Move version_utils to _internal | chore

c6505cf

Move version_utils to `_internal` so that it can be used my onnxscript [ghstack-poisoned]

Merge attrs and attr_protos in IRFunction | chore(irbuilder)

268ad8f

[ghstack-poisoned]

justinchuby mentioned this pull request Apr 12, 2023

Move version_utils to _internal | chore #624

Merged

justinchuby added a commit that referenced this pull request Apr 12, 2023

Merge attrs and attr_protos in IRFunction | chore(irbuilder)

e8eb966

ghstack-source-id: 3582671 Pull Request resolved: #625

justinchuby requested review from titaiwangms and gramalingam April 12, 2023 17:55

justinchuby mentioned this pull request Apr 12, 2023

Auto generate OpSchema for functions | feat(op_schema) #626

Merged

justinchuby changed the base branch from gh/justinchuby/15/base to main April 12, 2023 18:11

justinchuby changed the base branch from main to gh/justinchuby/15/base April 12, 2023 18:16

This comment was marked as resolved.

Sign in to view

justinchuby added the change base before merge Remember to change the merge base to main when the PR is ready to merge label Apr 12, 2023

justinchuby added 2 commits April 12, 2023 20:55

justinchuby changed the base branch from gh/justinchuby/15/base to main April 12, 2023 20:55

justinchuby removed the change base before merge Remember to change the merge base to main when the PR is ready to merge label Apr 12, 2023

justinchuby added 4 commits April 12, 2023 22:08

gramalingam reviewed Apr 12, 2023

View reviewed changes

justinchuby added 5 commits April 13, 2023 04:56

justinchuby added 5 commits April 13, 2023 15:46

gramalingam reviewed Apr 13, 2023

View reviewed changes

onnxscript/irbuilder.py Outdated Show resolved Hide resolved

justinchuby added 2 commits April 13, 2023 23:37

justinchuby mentioned this pull request Apr 14, 2023

Analyze type annotation for input types | feat(op_schema) #631

Merged

gramalingam approved these changes Apr 14, 2023

View reviewed changes

justinchuby added 2 commits April 14, 2023 21:22

justinchuby merged commit 5701cbf into main Apr 14, 2023

justinchuby deleted the gh/justinchuby/15/head branch April 14, 2023 21:39

Indie365 pushed a commit to Indie365/onnxscript that referenced this pull request Oct 26, 2023

Merge attrs and attr_protos in IRFunction | chore(irbuilder)

c2dfaef

ghstack-source-id: 24c2201 Pull Request resolved: microsoft/onnxscript#625 Signed-off-by: Justin Chu <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge `attrs` and `attr_protos` in `IRFunction` | chore(irbuilder) #625

Merge `attrs` and `attr_protos` in `IRFunction` | chore(irbuilder) #625

justinchuby commented Apr 12, 2023 •

edited

Loading

titaiwangms commented Apr 12, 2023

justinchuby commented Apr 12, 2023

titaiwangms commented Apr 12, 2023

This comment was marked as resolved.

codecov bot commented Apr 12, 2023 •

edited

Loading

gramalingam Apr 12, 2023

gramalingam Apr 12, 2023

justinchuby Apr 13, 2023

justinchuby Apr 13, 2023

gramalingam Apr 13, 2023

gramalingam Apr 13, 2023

justinchuby Apr 13, 2023

justinchuby Apr 13, 2023

gramalingam Apr 14, 2023

justinchuby Apr 14, 2023

gramalingam Apr 14, 2023

justinchuby Apr 14, 2023

gramalingam Apr 14, 2023

justinchuby Apr 14, 2023

Merge attrs and attr_protos in IRFunction | chore(irbuilder) #625

Merge attrs and attr_protos in IRFunction | chore(irbuilder) #625

Conversation

justinchuby commented Apr 12, 2023 • edited Loading

titaiwangms commented Apr 12, 2023

justinchuby commented Apr 12, 2023

titaiwangms commented Apr 12, 2023

This comment was marked as resolved.

codecov bot commented Apr 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Merge `attrs` and `attr_protos` in `IRFunction` | chore(irbuilder) #625

Merge `attrs` and `attr_protos` in `IRFunction` | chore(irbuilder) #625

justinchuby commented Apr 12, 2023 •

edited

Loading

codecov bot commented Apr 12, 2023 •

edited

Loading