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

Refactor/ops step1 : Introduce OpsRegistry to simplify operators development and enable incremental refactoring (#2638) #2644

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

imihalcea
Copy link
Contributor

I'm proposing the introduction of an OpsRegistry to manage ONNX operators, along with the implementation of the Sign operator. This change requires minimal modifications but it paves the way for incremental refactoring and the development of a clean, optimized ONNX execution engine separate from individual operations.

Key Changes:

  • Introduced OpsRegistry: A registry mapping operator names to their evaluation functions.
  • Move the Sign operator: Added the Sign operator using the new registry system.
  • 6 new LoC: Modified simple_eval slightly to utilize the OpsRegistry, keeping existing functionality intact.

Impact:

  • The modifications are focused and non-disruptive. All current tests pass without changes.
  • Sets the stage for further optimizations and refactoring, might lead to a robust and efficient ONNX execution engine.

Next Steps:

If this PR is accepted, I'm committed to:

  • Gradually moving other operators to the OpsRegistry.
  • Collaborating on enhancements for better execution engine (free obsolete resources, use hardware acceleration if possible etc..).

Regards,

Ionut

@LaurentMazare
Copy link
Collaborator

As mentioned in the other issues, I think it would be better to do this in an external crate to start with, this will allow you to iterate quicker rather than having to merge things in the main repo. And if it goes well and end up being a good replacement for the current implementation, we could just deprecate candle-onnx in favor of this other crate.

@LaurentMazare
Copy link
Collaborator

You can have it in a repo under your own name to start with, as for the name whatever you feel like :)

@imihalcea
Copy link
Contributor Author

@LaurentMazare I'll move forward with developing the refactored ONNX runtime engine as an external crate, which I've named candle-onnx-ng (to be disruptive^^). I look forward to sharing updates with you and potentially collaborating down the line.

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.

2 participants