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 Java ONNX Interface #199

Merged
merged 8 commits into from
Dec 10, 2021
Merged

Conversation

JackSullivan
Copy link
Member

Description

A general refactoring of the tools used to produce ONNX models from Tribuo, together with a re-implementation of all existing Tribuo ONNX interfaces in terms of the refactored tool.

Motivation

The existing method of producing ONNX models for Tribuo requires manually wiring up the computation graph by passing magic strings into and out of ONNX operations, and requires an implementer to add each element individually to the constructed graph. Failing/forgetting to do each of these things for each node leads to invalid graphs that will potentially not be detected until much later.

This refactor provides a thin interface that doesn't require the user to manually track strings (while still allowing them to name constructed nodes as desired) ensures that references actually exist before they can be applied. It also allows programming interfaces in a more fluent style (input.apply(Operation, arguments)) and by confining direct interaction with ONNX protobufs to a small handful of methods opens the possibility for later enhancements to add much more in the way of early-failing correctness checking (like ensuring compatible dimensions between arguments).

@JackSullivan JackSullivan requested a review from Craigacp December 7, 2021 19:58
@Craigacp Craigacp added Oracle employee This PR is from an Oracle employee squash-commits Squash the commits when merging this PR labels Dec 7, 2021
Copy link
Member

@Craigacp Craigacp left a comment

Choose a reason for hiding this comment

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

This is a lot smaller and better checked, which is really nice. Most of the comments are asking for more docs.

I'm not sure if the ONNXRef subclasses should be static inner classes on ONNXRef. It would be nice to make them into records one day, but that means ONNXRef needs to be an interface.

Copy link
Member

@Craigacp Craigacp left a comment

Choose a reason for hiding this comment

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

LGTM

@Craigacp Craigacp merged commit 87d51c2 into oracle:main Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Oracle employee This PR is from an Oracle employee squash-commits Squash the commits when merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants