New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
op function body not to allow default opset version #5908
base: main
Are you sure you want to change the base?
op function body not to allow default opset version #5908
Conversation
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5908 +/- ##
=======================================
Coverage 56.81% 56.81%
=======================================
Files 506 506
Lines 30370 30370
Branches 4592 4592
=======================================
Hits 17256 17256
Misses 12285 12285
Partials 829 829 ☔ View full report in Codecov by Sentry. |
|
||
bool BuildContextDependentFunction( | ||
const FunctionBodyBuildContext& ctx, | ||
FunctionProto& function_proto, | ||
int requested_opset_version = OpSchema::kUninitializedSinceVersion) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering: is OpSchema::kUninitializedSinceVersion
used anywhere else, or can it be removed?
@@ -699,21 +699,14 @@ void OpSchema::ParseAndSetTypes( | |||
OpSchema& OpSchema::SetContextDependentFunctionBodyBuilder( | |||
ContextDependentFunctionBodyBuilder functionBuilder, | |||
int opset_version) { | |||
if (opset_version == OpSchema::kUninitializedSinceVersion && since_version_ != OpSchema::kUninitializedSinceVersion) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I wonder if we should have an assert to ensure that opset_verion is > 0 (that is, not equal to OpSchema::kUninitializedSinceVersion
)? Just to catch it if user passes that value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question - we had a similar discussion in #5906 (comment) as well.
Description
OpSchema::FunctionBody, OpSchema::SetContextDependentFunctionBodyBuilder, and OpSchema::BuildContextDependentFunction used to have since_version as the default opset_version. This poses an issue for custom ops. Make opset version a required argument so that there is no ambiguity.
Motivation and Context
to enhance validity of function bodies. Need to validate with Ort before merge. It PR will be merged after 1.16.0 release.