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

Expose downloadBase in WhisperKit init #57

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

finnvoor
Copy link
Contributor

@finnvoor finnvoor commented Mar 7, 2024

Allows setting a custom download location while still using the automatic model downloading.

@finnvoor
Copy link
Contributor Author

finnvoor commented Mar 7, 2024

This could maybe be replaced by downloading the model to modelFolder when download is true, but right now I think there is a mismatch between modelFolder and downloadBase (modelFolder contains the models, downloadBase contains models/argmaxinc/whisperkit-coreml), so it's a bit tricky to work with.

@finnvoor finnvoor marked this pull request as draft March 7, 2024 14:29
@ZachNagengast
Copy link
Contributor

Yes it's a bit tricky I agree, will have to be careful with this because changing the location could easily create "orphan" models on the filesystem - downloadBase is relative to huggingface since that's where various other HF repos go if someone decides to use a different model than our pre-generated ones. This location's default (user documents folder) comes from the swift-transformers library, similar to how the python transformers places HF models in all the same place. The modelFolder is the specific folder within that repo that contains the models that actually get used. The apps should be fairly sheltered from having to deal with this because we return the local path upon download completion, so I think it makes most sense to allow changing the downloadBase, and letting that define the modelFolder location. We are also discussing whether we want to move all the variants into their own repos to match HF's typical way of storing models, open to input!

This PR looks good because it'll let folks set the downloadBase to any preferred location and is a good followup to #34 . In any case, since its just adding an optional default nil param, I don't expect it to cause much conflict with existing code.

@finnvoor finnvoor marked this pull request as ready for review March 7, 2024 16:51
@ZachNagengast ZachNagengast self-requested a review March 7, 2024 21:11
@ZachNagengast ZachNagengast merged commit d08fb1b into argmaxinc:main Mar 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants