-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[TTS][refactor] Part 8 - added model inference tests to safeguard changes. #6129
Conversation
ca64f15
to
b8e4b35
Compare
f844318
to
d24662b
Compare
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.
Since some of these tests are on the order of 20s long, it looks like this will add several minutes to each CI run. Do we have any rule of thumb on unit test duration that we should stay under? And should these be nightly tests?
@ericharper @okuchaiev @titu1994 -- any thoughts?
These unit tests are necessary to control the quality of our TTS collection receipts, it can also save efforts for QA. IMHO, It is worth spending several minutes (~5min in NVIDIA RTX A6000). IIRC, TTS test coverage is still tiny in comparison to other domains. |
81fb37c
to
a369bb0
Compare
discussed offline, and revised according to the final solution. Please see the updated description. |
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.
Overall it looks fine but the explicit lang check is going to crash other domains when checkpoints are updated.
Thanks for your comment. This language ID check is actually on purpose because at least TTS models should follow the naming format, i.e. languageID on the second field, otherwise, the test would fail and we guide contributors to correct their model names. @titu1994 May I know why such strict checks will negatively affect other domains? this is only applied for TTS collections. |
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.
Thanks! LGTM.
The test will fail when TTS updates model list, irrespective of which other domains test is running. Don't break other PRs just for TTS test failure |
I am still confused why it would break any PRs since all these tests are marked as |
a369bb0
to
ec1b9a3
Compare
ec1b9a3
to
d45cef8
Compare
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.
LGTM
* added test for fastpitch. * added test for hifigan. * added test for mixtts. * added test for Tacotron2. * added test for VITS. * added test for UnivNet. * added test for waveglow. * remove old waveglow model that relies on torch_stft. * added test for aligner. Signed-off-by: Xuesong Yang <[email protected]>
Signed-off-by: Xuesong Yang <[email protected]>
Signed-off-by: Xuesong Yang <[email protected]>
…move duplicates. Signed-off-by: Xuesong Yang <[email protected]>
Signed-off-by: Xuesong Yang <[email protected]>
Signed-off-by: Xuesong Yang <[email protected]>
Signed-off-by: Xuesong Yang <[email protected]>
fe1eaca
to
53762bf
Compare
…nges. (NVIDIA#6129) * [TTS] added model inference unit tests. * added test for fastpitch. * added test for hifigan. * added test for mixtts. * added test for Tacotron2. * added test for VITS. * added test for UnivNet. * added test for waveglow. * added test for aligner. * explicitly adding encoding=utf-8 when open cmudict and other ipa dicts. * created nightly marker and its option for pytest. * fixed pretrained model names that are not following naming convention. --------- Signed-off-by: Xuesong Yang <[email protected]>
…nges. (NVIDIA#6129) * [TTS] added model inference unit tests. * added test for fastpitch. * added test for hifigan. * added test for mixtts. * added test for Tacotron2. * added test for VITS. * added test for UnivNet. * added test for waveglow. * added test for aligner. * explicitly adding encoding=utf-8 when open cmudict and other ipa dicts. * created nightly marker and its option for pytest. * fixed pretrained model names that are not following naming convention. --------- Signed-off-by: Xuesong Yang <[email protected]> Signed-off-by: hsiehjackson <[email protected]>
Download NGC pre-trained model checkpoints and run model inference for all models supported in
self.list_available_models()
.tts_hifigan
-->tts_en_hifigan
tts_waveglow_88m
-->tts_en_waveglow_88m
--nightly
option andnightly
marker for pytest. Without--nightly
option, all tests here will be skipped. We are setting it up this way on purpose to save running time from our CI, and it provides an option for QA.related PR: #6128
Collection: [Note which collection this PR will affect]
Changelog
Usage
# Add a code snippet demonstrating how to use this
Before your PR is "Ready for review"
Pre checks:
PR Type:
If you haven't finished some of the above items you can still open "Draft" PR.
Who can review?
Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.
Additional Information