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.

Sign up for GitHub

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

Jump to bottom

Feat: support modelscope #45

Merged
merged 1 commit into from
Aug 5, 2024
Merged

Feat: support modelscope #45

merged 1 commit into from
Aug 5, 2024

Conversation

vicoooo26
Copy link
Contributor

Support loading model from Modelscope using Modelscope Python SDK

Ref #19

@InftyAI-Agent InftyAI-Agent added needs-triage Indicates an issue or PR lacks a label and requires one. needs-priority Indicates a PR lacks a label and requires one. do-not-merge/needs-kind Indicates a PR lacks a label and requires one. feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Aug 2, 2024
@kerthcet
Copy link
Member

kerthcet commented Aug 2, 2024

Hey bro, thank you very much for your contribution, we're at very early stage so some baselines haven't been established like the tests, I'll try my best this weekend to build it.

@kerthcet
Copy link
Member

kerthcet commented Aug 2, 2024

Please add an example under /docs/examples for reference, and have you tried this?

kerthcet
kerthcet reviewed Aug 2, 2024
View reviewed changes
Copy link
Member

@kerthcet kerthcet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Generally LGTM, let's add an example.

llmaz/loader/model_hub/model_scope.py Outdated
from modelscope import snapshot_download
from loader.model_hub.model_hub import ModelHub, MODEL_LOCAL_DIR

MODEL_SCOPE = "Modelscope"
Copy link
Member

Choose a reason for hiding this comment

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

I guess ModelScope makes more sense?

kerthcet
kerthcet reviewed Aug 2, 2024
View reviewed changes
llmaz/loader/model_hub/model_scope.py Outdated
MAX_WORKERS = 4


class Modelscope(ModelHub):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class Modelscope(ModelHub):
class ModelScope(ModelHub):

@kerthcet
Copy link
Member

kerthcet commented Aug 3, 2024

One more thing, can you add an integration test for Defaulting test at /test/integration/webhoook/model_test.go to make sure configuring modelScope is allowed? We currently have a test for huggingface there.

@kerthcet
Copy link
Member

kerthcet commented Aug 5, 2024

/lgtm
/approve

Thanks @vicoooo26

@InftyAI-Agent InftyAI-Agent added lgtm Looks good to me, indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 5, 2024
@InftyAI-Agent InftyAI-Agent merged commit c651c9f into InftyAI:main Aug 5, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kerthcet kerthcet kerthcet left review comments

Assignees

@kerthcet kerthcet

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. feature Categorizes issue or PR as related to a new feature. lgtm Looks good to me, indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a label and requires one. needs-triage Indicates an issue or PR lacks a label and requires one.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vicoooo26 @kerthcet @InftyAI-Agent