-
Notifications
You must be signed in to change notification settings - Fork 51
Namespace based customop registration #204
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
base: main
Are you sure you want to change the base?
Namespace based customop registration #204
Conversation
Replace dictionary-based registration with module namespace lookup: - Direct attribute lookup in module namespace (preferred) - Legacy custom_op dictionary support (backward compatibility) - Domain aliasing (e.g., onnx.brevitas -> qonnx.custom_op.general) - Runtime op registration via add_op_to_domain() - No expensive fallbacks (removed case-insensitive matching, full module scan) - Clear, actionable error messages Benefits: - Better IDE support with direct imports - Cleaner, more Pythonic API - O(1) lookup performance - Predictable behavior - External packages work via domain-based imports (e.g., finn.custom_op.fpgadataflow)
How are these ops in the custom domain suppose to execute?
However, the function
I also tested the code on chisel4ml, with a custom defined in the chisel4ml.preprocess namespace. I then called Am I missing something? should I be calling something else? Honestly, I just don't see how this can execute. |
src/qonnx/custom_op/registry.py
Outdated
Example: | ||
add_op_to_domain("qonnx.custom_op.general", "TestOp", TestOp) | ||
""" | ||
if not inspect.isclass(op_class) or not issubclass(op_class, CustomOp): |
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.
This logic is not correct. It evaluates to True
even if op_class
is not subclass of CustomOp
. This code will evaluate to True
on anything, even if op_class
is a float.
Regardless of that, I do not see the point of using inspect here. If the intention is to prevent the user from using an object instance here, issubclass
will raise a TypeError
. Which is a more appropriate error than ValueError
.
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.
This check is from an old iteration where it cached all CustomOps at a given domain on first access, this avoided illegal comparisons. Thanks for catching this, will remove in next commit.
src/qonnx/custom_op/registry.py
Outdated
# Strategy 1: Direct namespace lookup (preferred) | ||
if hasattr(module, op_type): | ||
obj = getattr(module, op_type) | ||
if inspect.isclass(obj) and issubclass(obj, CustomOp): |
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.
is obj
a class? judging from the code bellow it is a class not an object. This makes the variable name obj
somewhat misleading. Also the same argument about inspect holds here.
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.
Fixed in minor refactor, along with adding caching and thread safety.
@jurevreca12 Could you clarify the expected behavior of the "Brevitas exception?" Since it was only applied in the registry, I assumed similar aliasing should only be used for path resolution when the node's domain doesn't match its import path. Therefore, it's proper for |
- Replace eager module inspection with on-demand discovery - Add RLock for thread-safe registry access - Cache discovered ops in _OP_REGISTRY dict - Remove get_ops_in_domain() and add_op_to_domain() - Privatize registry state variables
Summary
This PR enables CustomOp registration through Python module namespaces, eliminating the need for manual dictionary registration. CustomOps can now be discovered automatically by importing the module corresponding to the ONNX domain name. The system maintains full backward compatibility with legacy
custom_op
dictionaries while providing better IDE support and a more Pythonic API.Key Changes
Core Registry (
src/qonnx/custom_op/registry.py
)getattr(module, op_type)
)custom_op
dictionariesadd_domain_alias()
)resolve_domain()
helper for alias resolutionPerformance & Safety Improvements
_OP_REGISTRY
)RLock
_OP_REGISTRY
,_DOMAIN_ALIASES
)Cleanup
add_op_to_domain()
andget_ops_in_domain()
functions__all__
exports from modulesBenefits
custom_op
dictionaries continue to workMigration Path
No code changes required. The system automatically discovers ops from both module namespaces and legacy
custom_op
dictionaries. New CustomOps can be registered by:custom_op
dictionaries (backward compatibility)add_domain_alias()
to map custom domains to module paths