Retire get_schema in Op | chore!(api)
#698
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ghstack-poisoned]
This was referenced Apr 28, 2023
justinchuby
added
topic: api
topic: breaking changes
change base before merge
|
The cleanup is a good idea. However, I wonder if it is incomplete. Perhaps the lookup in the opset should be moved into |
I think it is done in the generated code: onnxscript/onnxscript/onnx_opset/_impl/opset18.py Lines 66 to 70 in 08d1162 Although this will mean if anything is not yet generated (e.g. opset20), it will no longer find the opschema after this change, which I think is ok.
Is this it, in onnxscript/onnxscript/values.py Lines 74 to 78 in 08d1162 |
No, that just returns the schema. (That should probably be cleaned up too, at some point.) I mean, a method to get the |
|
Since the code is auto generated I think that's ok? The op can be obtained by |
|
With this change I am getting the stack below. I can't seem to locate the error. Does this stack make sense to you? Debug info: |
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. [ghstack-poisoned]
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. [ghstack-poisoned]
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. [ghstack-poisoned]
justinchuby
added a commit
that referenced
this pull request
Apr 28, 2023
ghstack-source-id: 771b415 Pull Request resolved: #698 Signed-off-by: Justin Chu <[email protected]>
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. [ghstack-poisoned]
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. [ghstack-poisoned]
justinchuby
added a commit
that referenced
this pull request
Apr 28, 2023
ghstack-source-id: 771b415 Pull Request resolved: #698 Signed-off-by: Justin Chu <[email protected]>
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. [ghstack-poisoned]
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. [ghstack-poisoned]
justinchuby
added a commit
that referenced
this pull request
Apr 28, 2023
ghstack-source-id: e894b25 Pull Request resolved: #698 Signed-off-by: Justin Chu <[email protected]>
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. [ghstack-poisoned]
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. [ghstack-poisoned]
justinchuby
added a commit
that referenced
this pull request
Apr 28, 2023
ghstack-source-id: 1178555 Pull Request resolved: #698 Signed-off-by: Justin Chu <[email protected]>
justinchuby
added a commit
that referenced
this pull request
Apr 28, 2023
Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at bottom): * #698 * __->__ #674 This PR implements auto OpSchema generation for trace_only function as well. It leverages onnxscript's converter to create the same function IR as we do in `OnnxFunction`, but without translating the body. We created an `TracedOnnxFunction` class to expose a similar interface as `OnnxFunction`, and an `OpLike` protocol to standardize them. - Creates the `OpLike` protocol that defines common attributes and methods for `Op`, `OnnxFunction` and `TracedOnnxFunction` so we can assume a common interface. - Implement `param_schemas` for `TracedOnnxFunction` - Refactor `param_schemas` to extract common logic. - Removes `is_single_op` from `Op` because it is unused. - Refactor ast logic from `main.py` to `onnxscript/_internal/ast_utils.py` - The change is tested on all the existing trace_only functions. Fixes #630
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. [ghstack-poisoned]
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. [ghstack-poisoned]
justinchuby
added a commit
that referenced
this pull request
Apr 28, 2023
ghstack-source-id: 1bc5d2c Pull Request resolved: #698 Signed-off-by: Justin Chu <[email protected]>
justinchuby
commented
Apr 28, 2023
| @@ -135,7 +135,7 @@ def __init__(self, name: Optional[Union[str, List[str]]], kind: ConverterExpress | |||
| def is_const(self): | |||
| return self.kind == ConverterExpressionKind.CONST | |||
|
|
|||
| def __str__(self): | |||
| def __str__(self) -> str: | |||
| return self.name | |||
There was a problem hiding this comment.
@gramalingam name is Optional[Union[str, List[str]]]. Should we change this or the type?
There was a problem hiding this comment.
Change the type to optional[str] ... that's what translation of None seems to generate
There was a problem hiding this comment.
Do you mean changing the name's type to optional[str]? __str__ always needs to return a string. should we do something to self.name when is it None in this function?
There was a problem hiding this comment.
I guess we could do "" if self.name is None else self.name. Not sure if and where this is used.
There was a problem hiding this comment.
I will add an assert for now since it looks like we don't expect it to be None or anything other than str.
There was a problem hiding this comment.
I have a recollection that this may be used in the translation of None ... in other words, I suspect we might be creating a ConverterExpression that has no name (that is, self.name is None). However, we likely don't invoke the str method on it. So, it's fine in that sense. (Anyway, I suspect a bigger cleanup of ConverterExpression is due at some point, so that's another reason not to bother much for now.)
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. Rename `opschema` to `op_schema` for naming consistency. [ghstack-poisoned]
Remove `get_schema` in Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema. Rename `opschema` to `op_schema` for naming consistency. [ghstack-poisoned]
justinchuby
added a commit
that referenced
this pull request
Apr 28, 2023
ghstack-source-id: ef95fd1 Pull Request resolved: #698 Signed-off-by: Justin Chu <[email protected]>
gramalingam
reviewed
May 1, 2023
onnxscript/values.py
Outdated
| if op_schema is None: | ||
| self._op_schema = opset[opname] | ||
| else: | ||
| self._op_schema = op_schema |
There was a problem hiding this comment.
Nit: I believe self._op_schema = op_schema or opset[opname] does the same thing more compactly.
gramalingam
approved these changes
May 1, 2023
justinchuby
added a commit
that referenced
this pull request
May 2, 2023
…700) This requires ONNX 1.14. I am skipping the op test for ONNX<1.14 altogether. I tested locally to make sure no float32 test is skipped with this mechanism, and that it skips properly according to the type annotation. Currently no new dtypes are enabled because there are too many failures. Requires - #701 - #698
Indie365
pushed a commit
to Indie365/onnxscript
that referenced
this pull request
Oct 26, 2023
ghstack-source-id: a249990 Pull Request resolved: microsoft/onnxscript#698
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
get_schemain Op | chore!(api) #698Remove
get_schemain Op because (1) if an op does not have schema, it will not be in its opset either. (2) If an op does have schema, we don't need to consult with the opset. This change ensures a single correct way of accessing OpSchema.Rename
opschematoop_schemafor naming consistency.