ONNX Support: A proposal for Operator implementation

[imihalcea]

Discussed in #2634

^{Originally posted by imihalcea November 21, 2024}
Hello,

I am currently unable to perform inference on my model because it uses the Sign operator, which isn't implemented yet in candle-onnx. I'd like to contribute by submitting a PR to implement this operator, but I'm finding the current code design a bit challenging.

At the moment, all operator code is contained within a single module with pattern matching on the operators. I feel that this approach may hinder scalability and make it more difficult to move towards a complete implementation of the specifications.

I propose introducing an OpsRegistry, which would be a map with the operator name as the key and the evaluation function as the value. The evaluation functions could reside in their own modules and include unit tests. This would simplify contributions, as contributors would only need to focus on the operator they want to implement without modifying the execution engine.

What do you think of this suggestion? Would a PR in this direction interest you?

Looking forward to your feedback.

Best regards,

Ionut

[LaurentMazare]

If you care mostly about the sign operator, I would encourage you to add it in the current design.
Longer term, I think your suggestion make changes but also that would entail a lot of changes so I would suggest starting this effort as an external crate and see how it goes.

[imihalcea]

Thank you for your response.

I understand your concern about the potential scope of changes required to implement the OpsRegistry. Initially, I shared the same impression that it might entail significant modifications. However, after experimenting with the idea, I discovered that the changes to the existing codebase are minimal and can be introduced incrementally.

I've created a prototype implementation where the adjustments to the simple_eval function are minimal and localized. The core logic remains largely untouched, and the existing functionality continues to operate as before. This approach allows to refactor the codebase step by step without disrupting the current design.

You can view my implementation on this branch:

https://github.com/imihalcea/candle/tree/feature/ops/candle-onnx/src

Given that the proposed changes are minimal and offer an incremental refactoring option, would it be acceptable for me to open a PR with this implementation?