-
Notifications
You must be signed in to change notification settings - Fork 544
Update model-libraries.ts #1847
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
Conversation
Co-authored-by: Joshua Lochner <[email protected]>
Wauplin
left a comment
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.
Quickly reviewed it, though we won't shipped it until sam3 repo is live (with sam3 in the model card metadata).
| prettyLabel: "sam3", | ||
| repoName: "sam3", |
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.
| prettyLabel: "sam3", | |
| repoName: "sam3", | |
| prettyLabel: "SAM 3", | |
| repoName: "SAM 3", |
(maybe?)
| repoName: "sam3", | ||
| repoUrl: "https://github.com/facebookresearch/sam3", | ||
| filter: false, | ||
| countDownloads: `path:"config.json" OR path_extension:"pt"`, |
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.
what is the repo file structure exactly? I expect the config.json file to be required to run the model, correct? If that's the case, then I would count downloads only on this file. The problem with counting both on config.json and weights files .pt is that we would be double-counting downloads.
So what I'm suggesting is to count only on config.json (same as for transformers models), which happens to be the default rule to count downloads, meaning we can just remove the rule and it will work out of the box:
| countDownloads: `path:"config.json" OR path_extension:"pt"`, |
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.
The repo structure is expected to be like SAM2: https://huggingface.co/facebook/sam2-hiera-large/tree/main. The problem is that it hosts weights for two libraries: the original code (backed by the pt file), and the transformers weights.
The goal with this PR was to question whether it is possible to count downloads for both. More specifically, if we set library_name to sam3, and add transformers as a tag:
- Will download counts work as expected? (I think so)
- Will the "Use this model" in transformers work? (Yes)
- Will the "Use this model" in colab/Kaggle create a transformers notebook? Not sure; I can check the code in our internal repo to verify. As an alternative, we may want to include a
notebook.ipynbfor this use case.
Are there any other features that you think could be affected by this setup?
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.
(And some of these changes may need to be backported to the SAM2 repos too)
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.
Sorry, didn't see the related thread in time! Then my suggestion would be to either:
- download the config.json in the SAM3 library even if not used (adds a very little overhead on first load but that's all) => if you go for this solution, no need to register a custom download count rule
- or set the download count rule to
| countDownloads: `path:"config.json" OR path_extension:"pt"`, | |
| countDownloads: `path:"config.json" OR path:"sam3.pt"`, |
if we know exactly what will be the pt filename.
|
(discussed offline and this PR is not needed) |
Track downloads for sam3 repo