-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Made python kfserving.storage return uri if local path exists #92
Conversation
Hi @Tomcli. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
docs/samples/sklearn/README.md
Outdated
@@ -16,7 +16,7 @@ dump(clf, 'model.joblib') | |||
Then, we can run the Scikit-learn Server using the generated model and test for prediction. Models can be on local filesystem, S3 compatible object storage or Google Cloud Storage. | |||
|
|||
```shell | |||
python -m sklearnserver --model_dir model.joblib --model_name svm | |||
python -m sklearnserver --model_dir file://model.joblib --model_name svm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'd prefer if the "recommended" usage for the python server just used relative paths. This file convention is less and less common these days. I'm happy to support it, but maybe not recommend it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can stick to "only" relative path, but relative path can start with "/" OR "."
python/kfserving/README.md
Outdated
* Local filesystem with a prefix "/" | ||
* Local filesystem with a prefix "file://". For example: | ||
* With absolute path: `file:///absolute/path` | ||
* With relative path: `file://./relative/path` or `file://relative/path` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file://./
feels really awkward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I added back the os.path.exists
case now, users can either use the file://
prefix or without any prefix.
Thanks Tommy! Sorry to thrash your implementation but it looks like I miscommunicated a bit. I think the file:// convention is fine, but it's less and less common these days. Modern browsers support the convention, but I'm struggling to find other examples of libraries using it like this. I'm happy to support it, but perhaps not as the recommended usage pattern as it results in somewhat awkward syntax. |
Hi @ellis-bigelow, thanks for the feedback. So I added back Thanks. |
Awesome thanks. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ellis-bigelow 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 |
The current
kfserving.storage
implementation only takes absolute local paths. It should also apply to any relative/absolute local path that exists on the local system.This change is