Skip to content

Conversation

jurevreca12
Copy link
Collaborator

This PR makes it possible to add QONNX Ops from outside the brevitas/finn/qonnx repositories. It adds a function register_custom_domain .

This is an example of usage of this functionality in chisel4ml:
- https://github.com/cs-jsi/chisel4ml/blob/auto_parameters/chisel4ml/preprocess/fft_torch_op.py
- https://github.com/cs-jsi/chisel4ml/blob/auto_parameters/chisel4ml/preprocess/fft_qonnx_op.py

It defines a custom pytorch op, that then gets mapped to a (Q)ONNX op.

This PR should not affect other peoples code.

@jurevreca12
Copy link
Collaborator Author

I just realized I forgot to add the example code where register_custom_domain function is used. The examples above define the ops. However, first the register_custom_domain is called https://github.com/cs-jsi/chisel4ml/blob/auto_parameters/chisel4ml/transform.py#L87, and the ops are imported into the "chisel4ml" namespace inside chise4ml/__init__.py (because QONNX equates the domain name with the python namespace).

@jmitrevs
Copy link
Collaborator

jmitrevs commented Sep 4, 2025

Can maybe more documentation be added?

@jurevreca12
Copy link
Collaborator Author

There are now two PRs on this topic. The other being #204 . It also proposes more changes. However, from a quick glance, it seems that it removes the "brevitas exception". Which might break FINN code?

@tafk7
Copy link
Collaborator

tafk7 commented Sep 29, 2025

However, from a quick glance, it seems that it removes the "brevitas exception". Which might break FINN code?

The PR adds a "domain aliasing" system (Lines 36-39 in registry.py) to more generically address the issue of CustomOp domain redirection, with the "brevitas exception" aliased by default. This has worked with FINN in my testing, but please let me know if you encounter any issues.

@maltanar
Copy link
Collaborator

@jurevreca12 the PR #204 has been tested against FINN previously, and it seems a bit more documented. Can you please review it and see if it solves what you were trying to address in this PR (#144) ? If it does, I suggest we go for #204 instead.

@jurevreca12
Copy link
Collaborator Author

@maltanar agree. I will close this PR, and review #204 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants