Skip to content
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

Support register custom OpSchema by python #5906

Merged
merged 41 commits into from Feb 23, 2024

Conversation

OYCN
Copy link
Contributor

@OYCN OYCN commented Feb 5, 2024

Description

  • Add a python interface to register the OpSchema.
  • Add a python interface to deregister the OpSchema.
  • Avoid the value error when executing OpSchema::Finalize multiple times
  • Add flag to enable check node for custom domain
  • overload onnx.defs.has with version parameter

example for register OpSchema:

op_schema = defs.OpSchema(
    "CustomOp",
    "",
    1,
)
onnx.defs.register_schema(op_schema)

example for deregister OpSchema:

onnx.defs.deregister_schema('CustomOp', 1)

Motivation and Context

Follow up of #5019

@OYCN OYCN requested review from a team as code owners February 5, 2024 10:35
@xadupre
Copy link
Contributor

xadupre commented Feb 5, 2024

Why reg_schema and not register_schema which seems easier to understand? Should we check that has_schema returns True as well? Should we add a method to unregister a schema?

onnx/test/schema_test.py Fixed Show fixed Hide fixed
onnx/defs/__init__.py Fixed Show resolved Hide resolved
onnx/test/schema_test.py Fixed Show fixed Hide fixed
Copy link

codecov bot commented Feb 5, 2024

Codecov Report

Attention: Patch coverage is 98.75000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 56.79%. Comparing base (06548e4) to head (ce63fbc).

Files Patch % Lines
onnx/test/schema_test.py 98.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5906      +/-   ##
==========================================
+ Coverage   56.68%   56.79%   +0.11%     
==========================================
  Files         506      506              
  Lines       30231    30308      +77     
  Branches     4566     4580      +14     
==========================================
+ Hits        17137    17214      +77     
+ Misses      12268    12267       -1     
- Partials      826      827       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@OYCN
Copy link
Contributor Author

OYCN commented Feb 5, 2024

Why reg_schema and not register_schema which seems easier to understand?

You're right, register_schema is indeed clearer than reg_schema. I will update the naming to register_schema

Should we check that has_schema returns True as well ?

has_schema function will checks if the domain::name exist, and we need also check the version .

Should we add a method to unregister a schema ?

This is a good suggestion. I plan to implement this in the upcoming commit.

This is my first time submitting a PR. I'll do my best to learn and get better at this! 😃

Signed-off-by: oPluss <opluss@qq.com>
@justinchuby
Copy link
Contributor

Copy link
Contributor

@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! This is much needed.

onnx/cpp2py_export.cc Outdated Show resolved Hide resolved
onnx/defs/__init__.py Outdated Show resolved Hide resolved
onnx/defs/__init__.py Outdated Show resolved Hide resolved
onnx/defs/__init__.py Outdated Show resolved Hide resolved
onnx/defs/__init__.py Outdated Show resolved Hide resolved
onnx/defs/__init__.py Outdated Show resolved Hide resolved
onnx/test/schema_test.py Show resolved Hide resolved
onnx/test/schema_test.py Outdated Show resolved Hide resolved
onnx/test/schema_test.py Outdated Show resolved Hide resolved
onnx/test/schema_test.py Outdated Show resolved Hide resolved
@justinchuby justinchuby added this to the 1.16 milestone Feb 6, 2024
onnx/defs/__init__.py Outdated Show resolved Hide resolved
Signed-off-by: oPluss <opluss@qq.com>
Signed-off-by: oPluss <opluss@qq.com>
Signed-off-by: oPluss <opluss@qq.com>
Signed-off-by: oPluss <opluss@qq.com>
Signed-off-by: oPluss <opluss@qq.com>
Signed-off-by: oPluss <opluss@qq.com>
onnx/cpp2py_export.cc Outdated Show resolved Hide resolved
onnx/cpp2py_export.cc Outdated Show resolved Hide resolved
onnx/cpp2py_export.cc Outdated Show resolved Hide resolved
onnx/defs/__init__.py Outdated Show resolved Hide resolved
onnx/defs/schema.cc Outdated Show resolved Hide resolved
onnx/defs/schema.cc Outdated Show resolved Hide resolved
onnx/defs/schema.h Outdated Show resolved Hide resolved
Signed-off-by: opluss <opluss@qq.com>
Signed-off-by: opluss <opluss@qq.com>
Signed-off-by: opluss <opluss@qq.com>
This reverts commit f2577b9.

Signed-off-by: opluss <opluss@qq.com>
Signed-off-by: opluss <opluss@qq.com>
Signed-off-by: opluss <opluss@qq.com>
@gramalingam
Copy link
Contributor

LGTM ... the one question is whether to include it or not in the upcoming release, which has a deadline next weekend or so. Any opinions/thoughts welcome.

@justinchuby
Copy link
Contributor

justinchuby commented Feb 17, 2024

I think it would be beneficial to include this which will allow us to do further validation on models. If anything comes up we can always revert the PR and stage it for the next release.

That said, I don't have a strong opinion

onnx/defs/schema.h Outdated Show resolved Hide resolved
justinchuby and others added 2 commits February 17, 2024 09:16
Use `move` instead of `forward`
Remove default domain value in `DeregisterSchema`

Signed-off-by: opluss <opluss@qq.com>
@gramalingam
Copy link
Contributor

@OYCN : can you please take a look at the merge conflicts? We can try to merge this PR in this week, ahead of the next release cutoff.

Copy link
Contributor

@gramalingam gramalingam left a comment

Choose a reason for hiding this comment

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

Merge conflicts to be resolved, LGTM otherwise.

Signed-off-by: oPluss <opluss@qq.com>
@OYCN
Copy link
Contributor Author

OYCN commented Feb 21, 2024

The merge conflicts have been resolved. But some Windows-CI failing during the CMake configuration process. This might be due to an external problem?

@gramalingam gramalingam added this pull request to the merge queue Feb 23, 2024
Merged via the queue into onnx:main with commit 945d7be Feb 23, 2024
41 checks passed
cjvolzka added a commit that referenced this pull request Feb 26, 2024
* 'main' of https://github.com/onnx/onnx:
  Add attribute output_dtype to QuantizeLinear (#5956)
  Update inliner to propagate valueinfos (#5942)
  Fix ConstantOfShape type constraints (#5961)
  Support register custom OpSchema by python (#5906)
  Fix ReferenceEvaluator when run from a subclass (#5936)
@justinchuby justinchuby modified the milestones: 1.17, 1.16 Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants