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

Change operators interface #19

Closed
wants to merge 0 commits into from
Closed

Conversation

alrevuelta
Copy link
Owner

@alrevuelta alrevuelta commented May 22, 2020

-Add new interface for operators. See node_context
-Adapt existing operators to the new interface
-Add resolving function
-Modify inference function using the resolved context
-Minor modification in tests to use the new context

@alrevuelta alrevuelta requested a review from nopeslide May 22, 2020 11:00
@alrevuelta alrevuelta marked this pull request as draft May 22, 2020 11:00
@alrevuelta
Copy link
Owner Author

@nopeslide
This is more or less what I suggested in #17

Some highlights.

The new interface is this simple struct. So we reuse the NodeProto and just store the inputs/outputs as Tensors (because inside NodeProto we just have the names)

struct node_context{
  Onnx__NodeProto     *onnx_node;
  Onnx__TensorProto  **inputs;
  Onnx__TensorProto  **outputs;
  int (*resolved_op)(node_context *ctx);
};

So the operators looks like this:

int operator_add(node_context *ctx)

And this is how tensors are found. For add.

Onnx__TensorProto *A = searchInputByName(ctx, 0);
Onnx__TensorProto *B = searchInputByName(ctx, 1);
Onnx__TensorProto *C = searchOutputByName(ctx, 0);

Yes, we are searching by the name, but the time we are losing is insignificant, because we are searching within the node, so among only 3-5 depending the operator. I find it quite easy, we just need the knowledge of the index. Can even be autogenerated, not a big deal I think.

As agreed we first resolve (currently is done in the inference phase, but will be moved out) and once resolved we just run inference. As simple as this:

  for (int nodeIdx = 0; nodeIdx < model->graph->n_node; nodeIdx++)
  {
    all_context[nodeIdx].resolved_op(&all_context[nodeIdx]);
  }

MNIST operator is working, and with this solution we can get rid of the ugly _outputs table. Everything will be stored in all_context, even the calculated outputs.

For variadic tensors I have to code another function to get the array of tensors, but all names are in Onnx__NodeProto and its totally compatible.

So with this solutions:

  • We meet our requirement of resolving operators. Currently is done in a very simple way but this is the idea.
  • We meet also the requirement of resolving the inputs/outputs. Its not a 100% resolve because then we have to search within a node, but at least we don't have to search among all the model as we were doing.

So I would say this is the way to go. Its simple an effective and doesn't overcomplicate things more than needed. Will work on removing unused code and doing some cleanup.

Any thoughts?

@alrevuelta alrevuelta marked this pull request as ready for review May 23, 2020 09:43
@nopeslide
Copy link
Collaborator

thumbs up.
Small steps seem to be the best idea at this point.
Regarding variadic tensors: let us handle these later. the node_context allows easy modifications at this point

@nopeslide nopeslide closed this Jun 2, 2020
@nopeslide
Copy link
Collaborator

cherry picked the commits. had to force-push once so github registers this PR as merged

@alrevuelta
Copy link
Owner Author

@nopeslide Perfect. Just opened issue #21 the discuss whats next.
Cheers!

@alrevuelta alrevuelta deleted the operators-interface-change branch June 30, 2020 15:49
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