fix: correct mlserver uri conversion regardless of filenames #43
Conversation
There was a problem hiding this comment.
Looks great! Thanks for reporting the bug and providing the fix plus a test case!
You'll need to follow the steps mentioned in the DCO check to sign off on the commits in the branch, but the changes LGTM!
Signed-off-by: xinyitan <[email protected]>
Signed-off-by: xinyitan <[email protected]>
xvnyv
force-pushed
the
fix-mlserver-uri-conversion
branch
from
c8b8242
to
1b628a1
Compare
tjohnson31415
changed the title
Signed-off-by: xinyitan <[email protected]>
|
/assign @animeshsingh |
ckadner
requested review from
rafvasq and
ckadner
and removed request for
tedhtchang and
Tomcli
| ModelPath: "data", | ||
| InputFiles: []string{ | ||
| "data/model-settings.json", | ||
| "data/aaaaa.json", |
There was a problem hiding this comment.
Nit: use a more descriptive name for the model with alphabetical precedence such as a-model.json or alpha-model.json
There was a problem hiding this comment.
I generally would agree, but here I think the aaaaa drives home the point of this being a alphabetical sorting related issue
There was a problem hiding this comment.
Fair enough! I went back and forth on it myself and figured test case's description above it could be enough. Your call :)
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ckadner, tjohnson31415, xvnyv The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
… - bug fixes Signed-off-by: jooho <[email protected]>
…ahub-io#43) #### Motivation As documented in [this issue](kserve#40), there is currently a bug in the conversion of the `uri` provided in the MLServer `model-settings.json` file if the model files are symlinked before the uri conversion occurs. This PR is intended to fix the issue by ensuring that `model-settings.json` will always be processed first. #### Modifications The function `adaptModelLayoutForRuntime` in the MLServer adapter `server.go` was modified to ensure that, if present, the `model-settings.json` file will be placed at the head of the list of files to be processed. This makes sure that the config file will always be processed before any symlinks are created for the model files. A unit test was also added to ensure that the bug was fixed. #### Result Incorrect uri conversion bug no longer occurs even when the model files alphabetically precede the filename`model-settings.json`. Signed-off-by: xinyitan <[email protected]> Co-authored-by: xinyitan <[email protected]>
Motivation
As documented in this issue, there is currently a bug in the conversion of the
uriprovided in the MLServermodel-settings.jsonfile if the model files are symlinked before the uri conversion occurs. This PR is intended to fix the issue by ensuring thatmodel-settings.jsonwill always be processed first.Modifications
The function
adaptModelLayoutForRuntimein the MLServer adapterserver.gowas modified to ensure that, if present, themodel-settings.jsonfile will be placed at the head of the list of files to be processed. This makes sure that the config file will always be processed before any symlinks are created for the model files.A unit test was also added to ensure that the bug was fixed.
Result
Incorrect uri conversion bug no longer occurs even when the model files alphabetically precede the filename
model-settings.json.Resolves #40