-
Notifications
You must be signed in to change notification settings - Fork 67
Conversation
def test_download_hf_model(model): | ||
path = download_huggingface_model(model) | ||
assert os.path.isdir(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.
could we also check that we can successfully build the model from the contents of this path?
HF_URL_PREFIX = 'https://huggingface.co/jinaai/' | ||
HF_ORG_PREFIX = 'jinaai/' |
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.
when we have the push function, let's remove the org restriction
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.
LGTM!
finetuner/__init__.py
Outdated
@@ -517,6 +519,29 @@ def build_model( | |||
) | |||
|
|||
|
|||
def download_huggingface_model(model_name: str, token: Optional[str] = None) -> str: |
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.
is this meant to be public? if not, use an _
in front of the name
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.
I was not sure, maybe private is better
:param token: Optional token to access private models. | ||
:return: The local path to the downloaded model. | ||
""" | ||
from huggingface_hub import hf_hub_download, list_repo_files |
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.
where is this requirement?
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.
we have transformers already as a dependency which depends on it. Should we add it anyway?
Co-authored-by: George Mastrapas <32414777+gmastrapas@users.noreply.github.com>
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.
LGTM!
feat: support loading models from hf
This PR allows one to load models from https://huggingface.co/jinaai: