-
Notifications
You must be signed in to change notification settings - Fork 604
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
Only evaluate ONNX models once in stateless model eval #20154
base: master
Are you sure you want to change the base?
Conversation
Whether it is "multi" is a property of the function so it seems wrong to make the caller choose? Since a FunctionEvaluator is a single-use object with a life-cycle how about storing all outputs on it instead, so you could do
We could just return null from evaluate if there is no default output, so that the return from evaluate is just "returns the default output, named the same as the function, if any". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm confused. Wasn't the purpose of this to support multiple outputs from one Onnx function? Why do we also need to put multiple regular functions into one evaluator? This doesn't seem to give us anything since we don't add results of those back into the context?
So, now, when calling:
The For ONNX models that have multiple outputs, the |
Yes, so you have a goal of doing the same also for regular functions? |
I hadn't planned on this for this iteration, but this might be something we could consider when/if the need arises? |
But why support multiple regular functions within one evaluator then? |
Because each output of a ONNX model output has their own |
Got it. But should we capture this in the model instead of creating an evaluator having (and always evaluating) every function of the model? We could add a List of outputs to ExpressionFunction? |
By model here you mean the ONNX model (model is such an overloaded term atm)? I don't think the overhead of always evaluating the entire model is practically that large: we do that in Proton today. However, we could do something like:
To specify exactly which outputs one would like. If no outputs are passed, all outputs are calculated. This might make the |
By model I mean the class owning these functions, ai.vespa.models.evaluation.Model. At line 185 in that you pass the entire list of functions to the FunctionEvaluator constructor if no name is given, and therefore all the functions in the model are always evaluated when that evaluator is used. However, this is just to achieve the effect of getting all the outputs of a OnnxModel, because each one is represented as a separate ExpressionFunction, as a workaround for ExpressionFunction not supporting multiple outputs. So, I suggest it might be better to add that capability to ExpressionFunction such that we could have an isomorphic mapping between the concepts of Onnx models and our function representation (and for that matter, be able to represent rank features accurately as functions). |
Rank features having multiple outputs is very close to an anti-feature. There might be use cases that might make sense, but I think is better to start off with simple ones. |
Notwithstanding different opinions on that, it's a feature we have and will continue to have. (The performance consideration cuts both ways, some times you solve computational problem once and that produces multiple values.) |
@bratseth Draft PR. For discussion. Ref #19901.
To avoid breaking existing API, this adds a new
MultiFunctionEvaluator
which ensures that ONNX models are only evaluated once for all outputs. Example use:Especially the name
MultiFunctionEvaluator
is up for discussion.