Py: Add support for importing models from Hugging Face Hub #260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tarilabs
previously approved these changes
Jan 4, 2024
Comment on lines
170
to
172
| urljoin( | ||
| "https://huggingface.co", f"{repo}/resolve/{git_ref or 'main'}/{path}" | ||
| ), |
There was a problem hiding this comment.
@lampajr as we're storing the "raw binary URL" when import from HF, I believe we will need to make sure in MC reconciliation we're not using Secret/S3 but the storageURI similar to:
apiVersion: serving.kserve.io/v1beta1
kind: InferenceService
metadata:
annotations:
openshift.io/display-name: example
serving.kserve.io/deploymentMode: ModelMesh
name: example
namespace: ds-mre2e
labels:
opendatahub.io/dashboard: 'true'
spec:
predictor:
model:
modelFormat:
name: onnx
version: '1'
name: ''
resources: {}
runtime: mmserver1
storageUri: https://github.com/tarilabs/demo20231212/blob/main/v1.nb20231206162408/mnist.onnx?raw=true(this example demonstrates using binary from GitHub, but analogously using the raw binary URI from HF as highlighted.
Wdyt?
There was a problem hiding this comment.
+1 if we have different ways to create IS based on the nature of the model (either "internal" using s3 and the data connection or "external" using the storageUri).
Instead, if we decide to go with https://github.com/opendatahub-io/model-registry/issues/254 we will have a standardized way to create IS using storageUri only, this way we don't have to create different IS based on the model nature but just use the single model-registry specific way.
|
I'd like your opinion on the metadata imports, specifically in regards to
|
|
Also, as discussed previously, regarding the upload limits on deployed clusters, do you think it'd be better to crop large |
|
Changes in v2:
Edit: updated the README.md to add install instructions for the extra group. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #260 +/- ##
=========================================
+ Coverage 0 74.16% +74.16%
=========================================
Files 0 17 +17
Lines 0 2295 +2295
Branches 0 73 +73
=========================================
+ Hits 0 1702 +1702
- Misses 0 426 +426
- Partials 0 167 +167 ☔ View full report in Codecov by Sentry. |
|
Should we test extras on the default CI jobs or add new ones? @tonyxrmdavidson @rkubis |
I believe we need to decide about this more in general since we'll be asked from UI team how to store lists as custom_property, and I recall there is some limitations in the tradeoff between what allowed by MLMD and what possible as k8s resources. To be determined.
Did we confirm where the gRPC limit is coming from? 🤔 is this an hard-coded limit in MLMD server? |
lampajr
requested changes
Jan 4, 2024
I'll assume it's safer to discard those for now then. As for the tags, see the discussion above.
Unfortunately, just looking at the source code, it's only apparent that it's an absl warning from the server, but I wasn't able to find where it's being thrown. For future reference, the error we get for e.g. From the gRPC status code reference this means that "Some resource has been exhausted, perhaps a per-user quota, or perhaps the entire file system is out of space." which isn't too clear still. I'm investigating whether it's possible to override some connection parameter (as per this SO answer) to make that work, but I'm confused as to why local tests don't raise this. |
So far, I was only able to get a different error: edit: can be replicated beyond MR python client using REST with: when using a big description, and noticed if using Markdown in a description fails with syntax error: edit: can be replicated beyond MR python client with REST using: (unrelated to HR import functions from this PR) Based on the fact I could replicate these errors beyond the MR python client, I would say these are unrelated but affecting limitations. |
|
Importing the |
I'd recommend to default For the |
This can be left for later, as discussed privately, due to the additional complexity.
Documented on #263. |
dtrifiro
reviewed
Jan 11, 2024
dtrifiro
approved these changes
Jan 11, 2024
Signed-off-by: Isabella Basso do Amaral <[email protected]>
Signed-off-by: Isabella Basso do Amaral <[email protected]>
As discussed on [1], there's no need for a tags property just yet, and there's also no agreed format for list metadata, so let's just get rid of it. [1]: https://github.com/opendatahub-io/model-registry/pull/260#discussion_r1441786749 Signed-off-by: Isabella Basso do Amaral <[email protected]>
Signed-off-by: Isabella Basso do Amaral <[email protected]>
Signed-off-by: Isabella Basso do Amaral <[email protected]>
Signed-off-by: Isabella Basso do Amaral <[email protected]>
|
How should we deal with versions? I'm thinking about users that want to update a model after the initial import, so we probably could default the version to be the latest commit ID. wdyt? |
|
The user should provide version labels explicitly. Commit IDs should not be used for version labels, since model version labels are more like Docker image labels. And a registered model version is more like a Docker image release. |
Signed-off-by: Isabella Basso do Amaral <[email protected]>
lampajr
approved these changes
Jan 17, 2024
isinyaaa
referenced
this pull request
Jan 17, 2024
As discussed on [1], there's no need for a tags property just yet, and there's also no agreed format for list metadata, so let's just get rid of it. [1]: https://github.com/opendatahub-io/model-registry/pull/260#discussion_r1441786749 Signed-off-by: Isabella Basso do Amaral <[email protected]>
tarilabs
approved these changes
Jan 17, 2024
There was a problem hiding this comment.
tested latest using https://huggingface.co/tarilabs/mnist/tree/v1.nb20231206162408
and result indexed on MR:
registered_model:
curl --silent -X 'GET' \
"$MR_HOSTNAME/api/model_registry/v1alpha1/registered_models?pageSize=100&orderBy=ID&sortOrder=DESC&nextPageToken=" \
-H 'accept: application/json' | jq .items
[
{
"createTimeSinceEpoch": "1705514870832",
"customProperties": {},
"id": "37",
"lastUpdateTimeSinceEpoch": "1705514870832",
"name": "tarilabs-mnist-from-hf",
"state": "LIVE"
},
...
model_version:
curl --silent -X 'GET' \
"$MR_HOSTNAME/api/model_registry/v1alpha1/registered_models/37/versions?pageSize=100&orderBy=ID&sortOrder=DESC&nextPageToken=" \
-H 'accept: application/json' | jq .items
[
{
"author": "tarilabs",
"createTimeSinceEpoch": "1705514871435",
"customProperties": {
"license": {
"string_value": "apache-2.0"
},
"model_author": {
"string_value": "tarilabs"
},
"model_origin": {
"string_value": "huggingface_hub"
},
"repo": {
"string_value": "tarilabs/mnist"
},
"source_uri": {
"string_value": "https://huggingface.co/tarilabs/mnist/resolve/main/mnist.onnx"
}
},
"description": "testing out PR 260",
"id": "38",
"lastUpdateTimeSinceEpoch": "1705514871435",
"name": "v1.nb20231206162408",
"state": "LIVE"
}
]
model_artifact:
curl --silent -X 'GET' \
"$MR_HOSTNAME/api/model_registry/v1alpha1/model_versions/38/artifacts" \
-H 'accept: application/json' | jq .items
[
{
"artifactType": "model-artifact",
"createTimeSinceEpoch": "1705514872127",
"customProperties": {},
"id": "7",
"lastUpdateTimeSinceEpoch": "1705514872127",
"modelFormatName": "onnx",
"modelFormatVersion": "1",
"name": "tarilabs-mnist-from-hf",
"state": "UNKNOWN",
"storagePath": "mnist.onnx",
"uri": "https://huggingface.co/tarilabs/mnist/resolve/main/mnist.onnx"
}
]
@lampajr does it match your expectations on entity attributes? Seems to me it's coherent/as expected.
+1 it seems coherent to me too, everything is properly populated (worth to note that |
dhirajsb
pushed a commit
to dhirajsb/model-registry
that referenced
this pull request
Jan 20, 2025
…tend (opendatahub-io#260) Bumps [react-dom](https://github.com/facebook/react/tree/HEAD/packages/react-dom) from 18.2.0 to 18.3.1. - [Release notes](https://github.com/facebook/react/releases) - [Changelog](https://github.com/facebook/react/blob/main/CHANGELOG.md) - [Commits](https://github.com/facebook/react/commits/v18.3.1/packages/react-dom) --- updated-dependencies: - dependency-name: react-dom dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related: #133
Description
Fixes docstrings and some type errors, also simplifies type checking in
ProtoBase.unmapandMLMDStore.get_type_id.As laid out in opendatahub-io/model-registry#133 (comment), the API provides
utility calls to fetch metadata from a given Hub repo automatically.
The following metadata is fetched:
Additional data regarding the origin of the model is also stored in the model version created by the import.
TODO:
huggingface-hublib isn't installed.How Has This Been Tested?
Merge criteria: