Skip to content

fix: exported model name should match generator class name - #593

Merged
joanise merged 3 commits into
mainfrom
dev.ap/fix-model-name
Nov 26, 2024
Merged

fix: exported model name should match generator class name#593
joanise merged 3 commits into
mainfrom
dev.ap/fix-model-name

Conversation

@roedoejet

@roedoejet roedoejet commented Nov 25, 2024

Copy link
Copy Markdown
Member

PR Goal?

When you run everyvoice export the vocoder HiFiGAN is exported as a HiFiGANGenerator which then breaks the model name validation.

Fixes?

I just fixed the bug before creating an issue report since I just noticed it this weekend.

Feedback sought?

@SamuelLarkin - I'm not sure if the quick fix I've done here is the best way of addressing this, can you please advise if there is another place I should address the fix?

Priority?

high - currently demos are broken with exported vocoders

Tests added?

I can't really test the exporting of a model because of the size of the vocoder model (1GB)

How to test?

Confidence?

Version change?

Related PRs?

EveryVoiceTTS/HiFiGAN_iSTFT_lightning#42

@semanticdiff-com

semanticdiff-com Bot commented Nov 25, 2024

Copy link
Copy Markdown

Review changes with  SemanticDiff

Changed Files
File Status
  everyvoice/cli.py  6% smaller
  everyvoice/model/vocoder/HiFiGAN_iSTFT_lightning  0% smaller

@codecov

codecov Bot commented Nov 25, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.58%. Comparing base (50b30dc) to head (945ccb6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #593   +/-   ##
=======================================
  Coverage   76.58%   76.58%           
=======================================
  Files          46       46           
  Lines        3451     3451           
  Branches      470      470           
=======================================
  Hits         2643     2643           
  Misses        706      706           
  Partials      102      102           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@github-actions

github-actions Bot commented Nov 25, 2024

Copy link
Copy Markdown
Contributor
CLI load time: 0:00.30
Pull Request HEAD: 8608240e88a2f2cfc42ebd2b1644ffbc225843d6
Imports that take more than 0.1 s:
import time: self [us] | cumulative | imported package
import time:       257 |     107547 |   typer
import time:      6531 |     226183 | everyvoice.cli

@joanise joanise changed the title chore: update submodules fix: exported model name should match generator class name Nov 25, 2024
@joanise

joanise commented Nov 26, 2024

Copy link
Copy Markdown
Member

Samuel and I reviewed this together, and we agree it is implemented correctly, but there is some refactoring we want to apply before merging. PR coming soon for that.

@joanise

joanise commented Nov 26, 2024

Copy link
Copy Markdown
Member

@roedoejet Please review EveryVoiceTTS/HiFiGAN_iSTFT_lightning#43 and #595
Sam and I agreed this PR here was conceptually OK but needed refactoring before merging.

@joanise joanise left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, works correctly, I'll merge #595 at the same time.

@joanise
joanise merged commit 8f8416e into main Nov 26, 2024
@joanise
joanise deleted the dev.ap/fix-model-name branch November 26, 2024 19:36
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.

3 participants