fix(tts): restore Lightning-2.x on_validation_epoch_end signature in SSL models - #16217
Open
udsy19 wants to merge 1 commit into
Open
fix(tts): restore Lightning-2.x on_validation_epoch_end signature in SSL models#16217udsy19 wants to merge 1 commit into
udsy19 wants to merge 1 commit into
Conversation
…SSL models FastPitchModel_SSL and SSLDisentangler still define on_validation_epoch_end(self, outputs), the pre-Lightning-2.0 signature. The PL 2.0 migration (NVIDIA-NeMo#6433) intentionally removed the outputs parameter everywhere else in this repo, renaming validation_epoch_end(self, outputs) to on_validation_epoch_end(self) and switching every other model to self.validation_step_outputs (e.g. FastPitchModel.on_validation_epoch_end). It renamed these two methods but left their outputs parameter in place. Lightning's evaluation loop calls the hook with zero positional arguments (_EvaluationLoop._on_evaluation_epoch_end -> call._call_lightning_module_hook(trainer, "on_validation_epoch_end")), so any real validation epoch on either model raises "TypeError: ... on_validation_epoch_end() missing 1 required positional argument: 'outputs'" before user code runs, aborting training at the very first sanity-check validation pass -- reproduced against a real lightning.pytorch.Trainer.fit() call. Both models ship with a validation_ds and check_val_every_n_epoch configured in their example configs (examples/tts/conf/fastpitch_ssl.yaml, ssl_tts_22050.yaml), so this is on the default path, not a corner case. Fixed by following the same self.validation_step_outputs pattern already used by the sibling FastPitchModel: append each validation batch's output in validation_step, read from self.validation_step_outputs in on_validation_epoch_end, and clear it afterward. Added test_ssl_models_validation_hook.py, which calls on_validation_epoch_end() the same way Lightning does (zero arguments) and asserts it completes and logs the expected values. Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
svcnvidia-nemo-ci
added
the
waiting-on-maintainers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do ?
Fixes #16216. Restores the Lightning-2.x
on_validation_epoch_endhook signature onFastPitchModel_SSLandSSLDisentangler, which currently crash on the first validationsanity check with
TypeError: ... missing 1 required positional argument: 'outputs'.Collection: tts
Changelog
fastpitch_ssl.py,ssl_tts.py:on_validation_epoch_end(self, outputs)->on_validation_epoch_end(self),reading from
self.validation_step_outputs(populated invalidation_step, cleared at the end),matching the sibling
FastPitchModel.on_validation_epoch_endpattern already used elsewhere in this file.tests/collections/tts/models/test_ssl_models_validation_hook.py.Negative control
The new test calls
on_validation_epoch_end()with zero arguments, exactly howlightning.pytorch.trainer.call._call_lightning_module_hookinvokes it. Reverting only the twoproduction files (test unchanged) and re-running:
both fail with the exact
TypeErrorabove. With the fix restored, both pass.git log -S'def on_validation_epoch_end(self, outputs)'shows one commit, #6433 (the PL 2.0migration), which renamed the method in both files but is the same commit whose own message says
"Remove outputs arg from on_validation_epoch_end, on_test_epoch_end" for every other model it
touched -- an oversight on these two, not a decision.
Usage
No API change; validation runs instead of crashing.
Before your PR is "Ready for review"
Pre checks:
PR Type:
Additional Information
Signed-off-by: Udaya Tejas udayatejas2004@gmail.com