Revert PayloadProcessor ModelId to remain consistent with original value

[RobGeada]

Motivation

PR 90 changed the modelId passed to the PayloadProcessor to the resolvedModelId, instead of the originalmodelOrVModelId.

Modifications

This PR reverts this change to restore the expected model ID to modelOrVModelId, whose value is either found inside vModelId or resolvedModelId depending on the flag isvModel

Result

modelId remains consistent with previous behaviour in payload processor payloads

[kserve-oss-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RobGeada
To complete the pull request process, please assign rafvasq after the PR has been reviewed.
You can assign the PR to them by writing /assign @rafvasq in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ckadner]

@RobGeada -- can you add some details on how to test this fix? And it would be great to add a unit test as well

[ckadner]

Thanks @RobGeada -- could you add a unit test that teases out the bug and shows how your change fixes it? The test should fail before your fix and pass with your fix. Thank you!

[RobGeada]

@ckadner it isn't actually a bug that this PR is fixing, it's just reverting to an older behavior. Prior to #90, the PayloadProcessor received the following modelId:

processPayload(reqMessage.readerIndex(reqReaderIndex),requestId, modelId, ...)
where String modelId = validateModelId(midIt.next(), isVModel); (L724)

After PR90, the payload processor receives:

processPayload(reqMessage.readerIndex(reqReaderIndex),requestId, resolvedModelId, ...)

where resolvedModelId is set by the following conditions:

String modelOrVModelId = validateModelId(midIt.next(), isVModel);
if (isVModel){
    vModelId = modelOrVModelId;
    resolvedModelId = ModelMeshApi.resolvedModelId.getIfExists();
} else {
    // vModelId remains null in this case
    resolvedModelId = modelOrVModelId;
} 

(see code here)

Essentially, the payload process used to receive the output of validateModelId(midIt.next(), isVModel), which is now stored in modelOrVModelId. We know that modelOrVModelId is stored into either vModelId or resolvedModelId depending on if isVModel is true or false, respectively. This PR checks which variable holds the modelOrVModelId value by checking if vModelId is null (in theory, we could just check isVModel), and then grabbing the value from the correct variable:

String payloadModelId = vModelId == null ? resolvedModelId : vModelId;

therefore restoring the original behavior.

[RobGeada]

@njhill any updates here?

[njhill]

@RobGeada apologies for the delay in getting to this. I think the reason that this was changed in #90 was to ensure consistency. I don't think it's good for the payload processor to get this id without knowing whether it's a model or vModel id.

When used as part of kserve/modelmesh-serving, the model id will always be the vModel id with some additional suffix. Do you think that this would be sufficient for your purposes? Otherwise we should probably just add a (possibly-null) vModelId string field to the Payload class.

[njhill]

And if we did the latter, we could even also add a Payload#getVModelOrModelId() method

[njhill]

Also related: #88

[RobGeada]

Yeah, that works for me, I'll close this

[njhill]

@RobGeada I've opened #123 for this. Would you mind reviewing it?