fix: set non-empty modelPath for http storageURI #382

tjohnson31415 · 2023-05-26T21:05:35Z

Motivation

When using the storageURI form of an HTTP (non Azure blob) address to download a model, the modelPath needs to be an non-empty string. Before this change, the storageURI: http://models.r.us/my-model.json form would be equivalent to the following storage spec:

storage:
  type: http
  path: ''    # this being empty is problematic for later processing
  parameters:
    url: http://models.r.us/path/to/my-model.json

The http storage type is currently the only way to have a valid storage configuration with an empty path (mainly because it has a "url" parameter that could include the full path). That said, I'm not sure if we should make a path required for the HTTP storage type. In particular, if the url is just http://models.r.us/, there is no path portion.

Related: kserve/modelmesh-runtime-adapter#41 (comment)

Modifications

Set modelPath to the URL's Path and set the url parameter to not have the URL Path.

Result

With these changes, the storageURI example above changes to have a path field:

storage:
  type: http
  path: path/to/my-model.json
  parameters:
    url: http://models.r.us/

Signed-off-by: Travis Johnson <[email protected]>

tjohnson31415 · 2023-06-01T05:35:08Z

The changes in this PR are dependent on a fix in kserve/modelmesh-runtime-adapter#52 to not duplicate the "local path" for the downloaded file. Using the example from the issue description, without the fix in kserve/modelmesh-runtime-adapter#52, the file will be downloaded as my-model.json/my-model.json.

ckadner · 2023-06-01T20:33:24Z

I created a small FVT test case for this. I was about to push it to your branch assuming you will be away for a while.

Do you mind me adding it to your PR, or did you already have one ready?

#### Motivation Removes some buggy behavior related to the handling of an empty `modelPath` being passed into the puller #### Modifications - use `filepath.Join()` instead of manual string concatenation with the file separator to handle an empty `modelPathFilename` - in the http storageProvider, remove duplication of the `localPath` into the rendered path - just to not it: I think this bug has not been caught because using the HTTP provider with a `storageURI` means that the local path is empty - this will need to be fixed before kserve/modelmesh-serving#382 can be merged - add check to ensure that the local path for files downloaded by the puller is always a path within the generated model dir (prevents the generated dir name from being the model file as was seen in #41 (comment)) - if no path is extracted from the request `ModelPath`, use `_model` by default #### Result Resolves: #41 Signed-off-by: Travis Johnson <[email protected]>

Signed-off-by: Christian Kadner <[email protected]>

ckadner · 2023-06-01T23:30:28Z

I created a small FVT test case for this. I was about to push it to your branch assuming you will be away for a while.

The new FVT failed, before the dependent PR kserve/modelmesh-runtime-adapter#52 got merged / image build and pushed. As expected.

https://github.com/kserve/modelmesh-serving/actions/runs/5150015048/jobs/9273693622?pr=382#step:15:3366

Summarizing 1 Failure:
  [FAIL] ISVCs with HTTPS storage URI [It] should successfully load a model
  /home/runner/work/modelmesh-serving/modelmesh-serving/fvt/helpers.go:296

      Last Failure Info:
        Location:             7b8767-7wq2t
        Message:              INTERNAL: Failed to load Model due to MLServer runtime error: rpc error: code = Internal desc = builtins.IsADirectoryError: [Errno 21] Is a directory: '/models/_mlserver_models/isvc-https-x26xk__isvc-0ca2192877/mnist-svm.joblib'
        Model Revision Name:  isvc-https-x26xk__isvc-0ca2192877
        Reason:               ModelLoadFailed
        Time:                 2023-06-01T23:24:07Z

I will kick off another run after the modelmesh-runtime-adapter image was build and pushed to DockerHub

ckadner · 2023-06-02T00:12:36Z

The FVT completed successfully after the modelmesh-runtime-adapter image was built and pushed to DockerHub

https://github.com/kserve/modelmesh-serving/actions/runs/5150015048/jobs/9274211815#step:15:2367

------------------------------
• [2.413 seconds]
ISVCs with HTTPS storage URI should successfully load a model
/home/runner/work/modelmesh-serving/modelmesh-serving/fvt/storage/storage_test.go:59

  Timeline >>
  STEP: Creating inference service isvc-https-z7s2q @ 06/02/23 00:02:40.939
  STEP: Waiting for inference service isvc-https-z7s2q to be 'Ready' and model is 'Loaded' @ 06/02/23 00:02:40.953
  << Timeline
------------------------------
• [0.063 seconds]
ISVCs with HTTPS storage URI should successfully run inference
/home/runner/work/modelmesh-serving/modelmesh-serving/fvt/storage/storage_test.go:65

  Timeline >>
  2023-06-02T00:02:43Z	INFO	Deleting inference services isvc-https-z7s2q
  << Timeline
------------------------------

ckadner

Thank you @tjohnson31415 for working on these two fixes despite being on leave!

/lgtm

kserve-oss-bot · 2023-06-02T00:14:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ckadner, tjohnson31415

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:

~~OWNERS~~ [tjohnson31415]

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

#### Motivation When using the `storageURI` form of an HTTP (non Azure blob) address to download a model, the `modelPath` needs to be an non-empty string. Before this change, the `storageURI: http://models.r.us/my-model.json` form would be equivalent to the following `storage` spec: ``` storage: type: http path: '' # this being empty is problematic for later processing parameters: url: http://models.r.us/path/to/my-model.json ``` The http storage type is currently the only way to have a valid storage configuration with an empty `path` (mainly because it has a "url" parameter that could include the full path). That said, I'm not sure if we should make a `path` required for the HTTP storage type. In particular, if the `url` is just `http://models.r.us/`, there is no path portion. Related: kserve/modelmesh-runtime-adapter#41 (comment) #### Modifications Set `modelPath` to the URL's Path and set the `url` parameter to not have the URL Path. #### Result With these changes, the `storageURI` example above changes to have a `path` field: ``` storage: type: http path: path/to/my-model.json parameters: url: http://models.r.us/ ``` Signed-off-by: Travis Johnson <[email protected]> Signed-off-by: Christian Kadner <[email protected]> Co-authored-by: Christian Kadner <[email protected]> Signed-off-by: Luis Delossantos <[email protected]>

fix: set non-empty modelPath for http storageURI

9a0b8ff

Signed-off-by: Travis Johnson <[email protected]>

kserve-oss-bot added the do-not-merge/work-in-progress label May 26, 2023

kserve-oss-bot requested review from joerunde and rafvasq May 26, 2023 21:05

kserve-oss-bot added the approved label May 26, 2023

ckadner mentioned this pull request May 30, 2023

Restore SecureJoin logic to puller kserve/modelmesh-runtime-adapter#46

Closed

Merge branch 'main' into fix-http-modelpath

8139e55

ckadner added this to the v0.11.0 milestone May 31, 2023

ckadner self-assigned this May 31, 2023

Merge branch 'main' into fix-http-modelpath

0cc7537

tjohnson31415 mentioned this pull request Jun 1, 2023

fix(puller): fixes related to an empty model path kserve/modelmesh-runtime-adapter#52

Merged

ckadner linked an issue Jun 1, 2023 that may be closed by this pull request

v0.11.0: Model directory creation and loading error kserve/modelmesh-runtime-adapter#41

Closed

ckadner requested review from ckadner and njhill and removed request for joerunde and rafvasq June 1, 2023 20:34

Add test case

9440af4

Signed-off-by: Christian Kadner <[email protected]>

ckadner marked this pull request as ready for review June 2, 2023 00:13

kserve-oss-bot removed the do-not-merge/work-in-progress label Jun 2, 2023

ckadner approved these changes Jun 2, 2023

View reviewed changes

kserve-oss-bot added the lgtm label Jun 2, 2023

kserve-oss-bot merged commit 4a47d56 into kserve:main Jun 2, 2023

tjohnson31415 deleted the fix-http-modelpath branch September 5, 2023 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: set non-empty modelPath for http storageURI #382

fix: set non-empty modelPath for http storageURI #382

tjohnson31415 commented May 26, 2023

tjohnson31415 commented Jun 1, 2023

ckadner commented Jun 1, 2023

ckadner commented Jun 1, 2023 •

edited

Loading

ckadner commented Jun 2, 2023

ckadner left a comment •

edited

Loading

kserve-oss-bot commented Jun 2, 2023

fix: set non-empty modelPath for http storageURI #382

fix: set non-empty modelPath for http storageURI #382

Conversation

tjohnson31415 commented May 26, 2023

Motivation

Modifications

Result

tjohnson31415 commented Jun 1, 2023

ckadner commented Jun 1, 2023

ckadner commented Jun 1, 2023 • edited Loading

ckadner commented Jun 2, 2023

ckadner left a comment • edited Loading

Choose a reason for hiding this comment

kserve-oss-bot commented Jun 2, 2023

ckadner commented Jun 1, 2023 •

edited

Loading

ckadner left a comment •

edited

Loading