Fix issue with schema lookup where there are custom ops using the ONNX domain #14492
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
…X domain. Update testing infrastructure to use an explicit domain for custom ops. Using an empty string clashes with the ONNX domain and can cause unexpected issues. It's also a bad example for external users as our docs point to the unit tests. Fix a couple of places using exact matching of the node since version to be slightly more flexible and use a range (which aligns with how the kernel lookup works).
RandySheriffH
approved these changes
Feb 2, 2023
skottmckay
deleted the
skottmckay/FixIssuesWithCustomOpsUsingOnnxDomain
branch
skottmckay
added a commit
that referenced
this pull request
Mar 1, 2023
### Description <!-- Describe your changes. --> Changes to support standalone custom ops in a minimal build. Also incorporates changes from #14492 (needed to test builds prior to that being checked in). We first need to save the schema info from the operators used by the standalone op invoker in the ORT format model. Add mechanism for that. Merge the kernel lookup logic so the same is used in full and minimal build. NOTE: the version matching is now consistent with all other kernel lookups, and the call to CreateOp MUST use the exact version for the operator. Previously matching wasn't as strict, but this can lead to the incorrect kernel being chosen. Add tests. NOTE: There is currently no way to detect the ops/types/opsets used inside these custom ops as they don't exist until we create kernels, which is after model loading completes (which is the point the ORT format model is saved). Due to that they have to be manually added to the configuration used to do the reduced ops build. That shouldn't be too hard for the custom op author to add given the custom op implementation is specifying the op, opset and type constraints (i.e. they have the info and it's just a case of capturing/formatting it correctly). ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> Enable usage of the standalone op invoker by custom ops in a minimal build. --------- Co-authored-by: Edward Chen <18449977+edgchen1@users.noreply.github.com>
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.
Description
Fix issue with schema lookup where there are custom ops using the ONNX domain.
Update testing infrastructure to use an explicit domain for custom ops. Using an empty string clashes with the ONNX domain and can cause unexpected issues. It's also a bad example for external users as our docs point to the unit tests.
Fix a couple of places using exact matching of the node since version to be slightly more flexible and use a range (which aligns with how the kernel lookup works).
Motivation and Context
Fixes a problem that came up when adding support for standalone custom ops in an ORT format model. Separating these changes out to simplify review.