Skip to content
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

Add support to download model from Azure file share #1985

Closed
wants to merge 7 commits into from

Conversation

laozc
Copy link
Contributor

@laozc laozc commented Jan 12, 2022

What this PR does / why we need it:
Add support to download model from Azure file share

Which issue(s) this PR fixes:
Fixes #1967

Release note:

Support download model from Azure file share

@aws-kf-ci-bot
Copy link
Contributor

Hi @laozc. Thanks for your PR.

I'm waiting for a kserve member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@laozc laozc changed the title [WIP] Add support to download model from Azure file share Add support to download model from Azure file share Jan 13, 2022
laozc added 2 commits January 17, 2022 22:51
Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
@yuzisun
Copy link
Member

yuzisun commented Jan 23, 2022

/ok-to-test

@yuzisun
Copy link
Member

yuzisun commented Feb 4, 2022

@laozc Thanks for this great contribution! Would you be able to add e2e tests for the Azure storage?

@laozc
Copy link
Contributor Author

laozc commented Feb 9, 2022

@yuzisun I saw the e2e test cluster is set up on EKS.
It would be a bit diffcult to maintain an Azure storage in compliance for this case.

@yuzisun
Copy link
Member

yuzisun commented Feb 12, 2022

/cc @maganaluis for review

@kserve-oss-bot
Copy link
Collaborator

@yuzisun: GitHub didn't allow me to request PR reviews from the following users: review, maganaluis, for.

Note that only kserve members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @maganaluis for review

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.

@yuzisun
Copy link
Member

yuzisun commented Feb 15, 2022

@yuzisun I saw the e2e test cluster is set up on EKS. It would be a bit diffcult to maintain an Azure storage in compliance for this case.

We can create a kind cluster to test the Azure storage?

@Suresh-Nakkeran
Copy link
Contributor

Suresh-Nakkeran commented Mar 9, 2022

@laozc I'm receiving credential error.
ValueError("Token credentials not supported by the File service.")
It seems we should use account key to access file share.
Refer: Azure/azure-sdk-for-python#13033 (comment)
cc: @yuzisun

laozc added 4 commits April 2, 2022 16:20
Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: laozc
To complete the pull request process, please assign animeshsingh after the PR has been reviewed.
You can assign the PR to them by writing /assign @animeshsingh 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:

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

@laozc
Copy link
Contributor Author

laozc commented Apr 3, 2022

@Suresh-Nakkeran Storage Account Access Key auth is added in the latest commit.
You may check if you still encounter the same issue.

@yuzisun
Copy link
Member

yuzisun commented Apr 14, 2022

@Suresh-Nakkeran Storage Account Access Key auth is added in the latest commit. You may check if you still encounter the same issue.

@Suresh-Nakkeran Can you help verify this?

@Suresh-Nakkeran
Copy link
Contributor

Suresh-Nakkeran commented Apr 22, 2022

@laozc @yuzisun I am receiving below error when I test a model from azure file share. The access key is provided in secret. I also check the changes.

image

@Suresh-Nakkeran
Copy link
Contributor

@laozc is it AZURE_STORAGE_ACCESS_KEY or AZURE_STORAGE_KEY?

@laozc
Copy link
Contributor Author

laozc commented Apr 22, 2022

@laozc is it AZURE_STORAGE_ACCESS_KEY or AZURE_STORAGE_KEY?

AZURE_STORAGE_ACCESS_KEY should be the correct one.

@yuzisun yuzisun mentioned this pull request May 1, 2022
15 tasks
@@ -196,6 +197,10 @@ func (c *CredentialBuilder) CreateSecretVolumeAndEnv(namespace string, serviceAc
log.Info("Setting secret envs for azure", "AzureSecret", secret.Name)
envs := azure.BuildSecretEnvs(secret)
container.Env = append(container.Env, envs...)
} else if _, ok := secret.Data[azure.AzureStorageAccessKey]; ok {
log.Info("Setting secret envs for azure", "AzureSecret", secret.Name)
envs := azure.BuildSecretEnvs(secret)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laozc we should call the new azure.BuildStorageAccessKeySecretEnv function for Storage Access Key.

@yuzisun
Copy link
Member

yuzisun commented May 21, 2022

@laozc We have merged the PR on top of your commits, thanks for your awesome contributions!

@yuzisun yuzisun closed this May 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage Initializer Azure Fileshare download tgz RuntimeError ContentType check
5 participants