feat: Implement storage initializer for Seldon and KF #35
Conversation
caicloud-bot
added
release-note-none
Signed-off-by: Ce Gao <gaoce@caicloud.io>
Signed-off-by: Ce Gao <gaoce@caicloud.io>
caicloud-bot
added
size/XXL
Signed-off-by: Ce Gao <gaoce@caicloud.io>
Signed-off-by: Ce Gao <gaoce@caicloud.io>
Signed-off-by: Ce Gao <gaoce@caicloud.io>
gaocegege
changed the title
caicloud-bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
I see that we are using a forked version of seldon, what's the plan going forward? to wait until we have a draft UI to present to seldon community and hopefully merge the storage initializer?
PS. I'll leave code level review to @simon-cj
Yes. |
|
And we will also investigate if we can run without modification of seldon core. If we can do it, there is no need to contribute to upstream. |
|
ok. SGTM
…On Thu, Jun 11, 2020 at 10:25 PM Ce Gao ***@***.***> wrote:
And we will also investigate if we can run without modification of seldon
core. If we can do it, there is no need to contribute to upstream.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#35 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQXAAMWTPNVWT4PSTPDZHLRWDSMFANCNFSM4N2EK4LQ>
.
|
Signed-off-by: Ce Gao <gaoce@caicloud.io>
Seldon Core does not support renaming the environment variable name. Signed-off-by: Ce Gao <gaoce@caicloud.io>
Signed-off-by: Ce Gao <gaoce@caicloud.io>
|
FYI, we do not need to modify seldon core after 3a2f7ab . But we may need to update some configurations to use our own initializer. I will update the doc. /hold |
caicloud-bot
added
the
do-not-merge/hold
Signed-off-by: Ce Gao <gaoce@caicloud.io>
|
/hold cancel |
caicloud-bot
removed
the
do-not-merge/hold
| } | ||
| fmt.Printf("Logging to the remote registry %s\n", strs[0]) | ||
| fmt.Printf("Username: %s\n", username) | ||
| if err := ormbClient.Login(strs[0], username, pwd, true); err != nil { |
There was a problem hiding this comment.
Why it need login before ormb.NewOCIORMB, I think if ormbClient.Login between ormb.NewOCIORMB and ormbClient.Pull, it is more reasonable. And PreRunE can be deleted.
There was a problem hiding this comment.
We need two calls for ormb.NewOCIORMB to let it know the credentials.
There was a problem hiding this comment.
OK, If it is right, LGTM, I'am not understand it now, let me think it carefully.
There was a problem hiding this comment.
BTW, we only have one command in storage initializer, then we do not actually need the prerun here since it does not benefit us by code reusing
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gaocegege, simon-cj 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 |
caicloud-bot
added
the
approved
Signed-off-by: Ce Gao gaoce@caicloud.io
What this PR does / why we need it:
The model will be downloaded from the remote registry to the container via a storage initializer in Seldon Core and KFServing. Now it does not support ORMB. Thus we implement such a new initializer.
Which issue(s) this PR is related to (optional, link to 3rd issue(s)):
Fixes #
Reference to #25
Special notes for your reviewer:
/cc @your-reviewer
Release note: