Support upto 4D array as REST input payload.

[ScrapCodes]

closes #5

[ScrapCodes]

@pvaneck Please take a look !

[pvaneck]

Thanks @ScrapCodes , I will test this out soon. Currently having issues with my workstation I need to resolve. I'm not entirely sure why the lint job is suddenly failing with that Running error: context loading failed: no go files to analyze error. That will need to be looked into.

@njhill Would be great to have your eyes on this, too. :)

[ScrapCodes]

@pvaneck and @njhill thanks for taking a look, I have sent another PR to fix the CI. #7

[njhill]

Thanks @ScrapCodes. I think there should be a way we can streamline this and support arbitrary dimensions without having to deal with them as separate cases. I updated some experiments I was making before and can push that to a different branch to illustrate.

W.r.t. to test cases, I think we want to just have a set of raw json request payload examples along with the expected protobuf equivalent, and a bunch of raw protobuf response payload examples along with the expected REST equivalent. Just testing more cases end-to-end would be more valuable and simpler imo for this than more granular tests.

[njhill]

@ScrapCodes @pvaneck here's my proposed approach: https://github.com/kserve/rest-proxy/compare/main...njhill:dimensions?expand=1. It does not yet handle string / BYTES values though - I think there's some ambiguity in the spec around how BYTES values should/can be represented within the data array(s) so needs a bit more investigation.

[ScrapCodes]

Thanks @njhill, tests are not exactly as you had asked. Please take a look!

For handling BYTES in json, are you considering base64 encoding?

[njhill]

Thanks @ScrapCodes the tests are going in the right direction but I would suggest the following:

• Start with full REST request payload rather than just individual tensors, and test using the CustomJSONPb.NewDecoder func:

out = &gw.ModelInferRequest{}
err := CustomJsonPb{}.NewDecoder(...).Decode(out)
// verify contents of out

• No need to duplicate the verification logic in separate functions, just put the jsons in an array and have the test func iterate over that to test each
• No need to diff individual fields of the target protobuf struct, can just compare the entire struct
• We should also test the response path in a similar way, i.e. CustomJSONPb.Marshal

For BYTES as mentioned I think the spec is currently ambiguous (to me at least). For example if strings are used then how should the encoding be determined. Also it's not clear how the byte lengths should be encoded when flattening. One option is to say that they must all be the same length and add a dimension to the shape for this. If their lengths can vary however then something extra is needed, like a corresponding integer tensor to hold them.

Previously I looked into how MLServer handles these kinds of inputs for some clues but do not recall the details now. Maybe you could look into that?

[njhill]

@ScrapCodes @pvaneck any thoughts/update on this? We should aim to get it in for the release right?

[pvaneck]

@njhill Yep, this should definitely be in the next release. I believe @ScrapCodes is currently finalizing the tests, then we can hopefully get this merged in. I think BYTES can be handled in a separate PR. I remember looking at some of MLServer's BYTES string tests and base64 tests before, but yea, this needs some more investigation for how we should handle it here.

[ScrapCodes]

@njhill thanks for taking a look, hope this is close to what we expect. Code looks cleaner and should perform faster thanks to you.

[njhill]

Thanks @ScrapCodes, lgtm

[njhill]

Thanks @pvaneck, I opened an issue #8 for this.

Also I noticed that the files in this repo don't have the apache license/copyright headers, do you know if these are needed?

[pvaneck]

Thanks, @ScrapCodes

[pvaneck]

@njhill Ah, yea, they should be added. I can do that real quick.