feat: Make it possible to attach a PayloadProcessor to process model predictions #1

tteofili · 2023-03-09T18:43:02Z

Motivation

This PR seeks to address the model-mesh side of kserve/modelmesh-serving#284.

Modifications

It provides a PayloadProcessor interface. PayloadProcessors are picked by ModelMesh instances at startup and predictions (Payloads) are processed asynchronously at fixed timing.
A first logger implementation allows to log Payloads (at info level).

Result

A SPI for post processing model predictions.

netty 4.1.85.Final grpc-java 1.51.0 jackson-databind 2.14.1 Signed-off-by: Nick Hill <nickhill@us.ibm.com>

- protobuf 3.21.10 Signed-off-by: Nick Hill <nickhill@us.ibm.com> Signed-off-by: Nick Hill <nickhill@us.ibm.com>

#### Motivation `fq_names` is a boolean sub-parameters that can be used to alter the metrics exposed by model-mesh. Unfortunately however its value is inverted, so setting it to "true" actually disables fully qualified method names (which is already the default). #### Modification Change to interpret anything other than false as enabling (default is still disabled). #### Result Metrics configuration behaves as expected. Signed-off-by: Nick Hill <nickhill@us.ibm.com>

@njhill

#### Motivation Follow up to PR #75, use short names (by default) unless `fq_names` is set to `true` #### Modifications Correct the boolean expression that sets `shortNames` based on the negated `fq_names` parameter value. #### Result Brief example to illustrate the behavior after this code change: ```Java String[] fq_names_values = {"true", "True", "TRUE", "false", "False", "FALSE", "", "off", "on", "N/A"}; for (String fq_names_value: fq_names_values) { boolean shortNames = !"true".equalsIgnoreCase(fq_names_value); System.out.println("fq_names='%s' => shortNames=%s".formatted(fq_names_value, shortNames)); } ``` ``` fq_names='true' => shortNames=false fq_names='True' => shortNames=false fq_names='TRUE' => shortNames=false fq_names='false' => shortNames=true fq_names='False' => shortNames=true fq_names='FALSE' => shortNames=true fq_names='' => shortNames=true fq_names='off' => shortNames=true fq_names='on' => shortNames=true fq_names='N/A' => shortNames=true ``` /cc @njhill Signed-off-by: Christian Kadner <ckadner@us.ibm.com>

#### Motivation Some issues have been observed in production where the cache appears to get into a broken state, including a cascading eviction of all models. #### Modifications - Fix cache accounting in loading failure + unload case - Eagerly remove expired failure records from cache - Correct misworded log message - Log some internal cache values along with published instance records, to facilitate further diagnoses - Increase load failure expiry times #### Result Hopefully fixed cache accounting issue Signed-off-by: Nick Hill <nickhill@us.ibm.com>

grpc, netty, protobuf, thrift Signed-off-by: Nick Hill <nickhill@us.ibm.com>

Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>

@ckadner

This reverts commit d9892e3. These changes actually broke our main image build. Unfortunately the actions that run on the PR don't actually build the image. @ckadner is addressing that and then we can reopen this if needed post 0.10 release. @ddelange it probably only makes sense anyhow if we do this for all the modelmesh images i.e. also including `modelmesh-serving` and `modelmesh-runtime-adapter` Signed-off-by: Nick Hill <nickhill@us.ibm.com>

* Exclude 32bit arm and s390x builds * Use single workflow definition for pull-request and push event * Delete pull-request-validation.yml * Add layer caching * Add TARGETARCH arg to Dockerfile Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>