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

skip functional tests for models that are not downloaded. #48

Merged
merged 6 commits into from
Mar 9, 2024

Conversation

metropol
Copy link
Contributor

@metropol metropol commented Mar 4, 2024

hi all, it seems that functional tests require two models to be downloaded: openai_whisper-tiny (because of testRealTimeFactorTiny()) and openai_whisper-large-v3 (because of testInitLarge() and testRealTimeFactorLarge()). This should probably be added to the CONTRIBUTING.md, however this PR does not make this change.

Another functional test, testOutputAll() - actually allModelPaths(), assumes that folders in ./Models/whisperkit-coreml, which are created from the make setup-model-repo, contain actual models, while in fact only models explicitly downloaded with make download-model MODEL=x or make download-models are present.

testOutputAll() fails if not all models have been downloaded, which is not ideal. Running all tests for all models (takes a long time and) makes sense in some scenarios, but probably not to test some code changes.

I've implemented some changes so that allModelPaths() only returns folders that contain downloaded models. This is done by checking if a proxy file (MelSpectrogram.mlmodelc/coremldata.bin) is a git lfs pointer file (starting with version https://…). If it is, then the folder is not included.

@metropol
Copy link
Contributor Author

metropol commented Mar 4, 2024

Moved the typo fix to its own branch (for a future PR).

@ZachNagengast ZachNagengast self-requested a review March 6, 2024 17:00
@ZachNagengast ZachNagengast added the enhancement Improves existing code label Mar 6, 2024
Copy link
Contributor

@ZachNagengast ZachNagengast left a comment

Choose a reason for hiding this comment

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

This is a nice improvement, thank you!

I'd support fixing the typo in this PR, I just re-ran your commit and it passed all the tests, it may have just failed due to a failed api request. If you want to bring it back it lmk, otherwise I can merge this as is.

@metropol
Copy link
Contributor Author

metropol commented Mar 7, 2024

It failed when building for VisionOS, which I don't have as a platform locally.
For the typo fix (langauges -> languages), as you prefer: I can add it back to this branch or create another PR just for it.

@ZachNagengast
Copy link
Contributor

@metropol whatever is more convenient for you, probably just reverting the commit that removes it would be simplest.

@metropol
Copy link
Contributor Author

metropol commented Mar 9, 2024

@ZachNagengast I've reverted the commit the removes the typo.

@ZachNagengast ZachNagengast self-requested a review March 9, 2024 19:29
Copy link
Contributor

@ZachNagengast ZachNagengast left a comment

Choose a reason for hiding this comment

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

Good to merge 🚀

@ZachNagengast ZachNagengast merged commit 37cf113 into argmaxinc:main Mar 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants