-
Notifications
You must be signed in to change notification settings - Fork 31.6k
Enable granite speech 3.3 tests #37560
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
Enable granite speech 3.3 tests #37560
Conversation
|
Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. The CI will be paused while the PR is in draft mode. When it is ready for review, please click the |
|
|
||
| @slow | ||
| @pytest.mark.skip("Public models not yet available") | ||
| @pytest.mark.skipif(not is_peft_available(), reason="Outputs diverge without lora") |
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 don't understand very well here. Could you explain what are the different situations when peft is in the system and not in the system, it will cause this code in this test doing something differently and output will be different?
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.
Hi @ydshieh! Definitely - granite speech has a modality specific Lora that is only enabled when there are audio inputs. In this way, it lets users call it with audio for tasks like transcription, which will have the Lora enabled, and then choose to pass the raw text in through a second generate call if they want to, which is the same as calling the underlying llm (i.e., Lora is not enabled). This part of the model code will probably make it more clear:
| if is_peft_available and self._hf_peft_config_loaded: |
If Peft isn't installed, the model will not load the bundled audio Lora. This will cause the integration tests to fail with wrong outputs, which is why they are skipped if peft isn't installed
|
run-slow: granite_speech |
|
This comment contains run-slow, running the specified jobs: This comment contains run-slow, running the specified jobs: models: ['models/granite_speech'] |
|
There are 3 tests failed if you could take a look first |
|
Thanks @ydshieh - hopefully should be fixed now 🤞 |
d00dd5a to
968242b
Compare
|
Hi @ydshieh, can you please take another look at this PR when you have a moment? Our team just released a 2b variant of the speech model, so I've updated the tests to use it |
ydshieh
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.
Very nice, all green!
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
* Enable granite speech 3.3 tests * skip sdpa test for granite speech * Explicitly move model to device * Use granite speech 2b in tests --------- Co-authored-by: Yih-Dar <[email protected]>
Turns on the tests for granite speech now that the 3.3 model is up! Integration tests guard with peft, since they will fail if the lora isn't applied.
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@eustlb