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

Add new method for custom formatter handling #293

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

ivuorio
Copy link

@ivuorio ivuorio commented Feb 7, 2025

Adding a method for setting new formatter into the native formatter handling. This does not change how the existing solutions work, and thus should not cause any backwards compatibility issue. But also allows user to implement multiple custom formatters and allows using them in datasets according to their name just like the builtin formatters. This solves both problems from the issue #290 as the the custom formatter is handled just like builtin ones.

Adding:

  • New method for adding custom formatters
  • Updating the documentation about the custom formatter using

Testing:

  • Manually tested with multiple datasets in YourTTS model training. Works with both compute embeddings and dataset loader.

ToDo:

  • Add unit tests for the new method
  • Add unit tests for custom formatter
  • Run full test set locally

@ivuorio ivuorio marked this pull request as ready for review February 11, 2025 16:09
@ivuorio
Copy link
Author

ivuorio commented Feb 11, 2025

I seem to have issues with running tests locally, thus the open todo. There are multiple test that seem to fail with issue where some of the tensor are on GPU and some on CPU. I wonder if the test are to be run on no GPU setup?

@eginhard
Copy link
Member

Ah, don't try to run all tests locally, it will take forever. I've just enabled the CI for this PR, it should also rerun automatically now when you push new changes. But thanks for also adding tests! I only run them on CPU, so not sure how things go with a GPU.

I have not looked at the implementation yet and might not have time to test it in the next 2 weeks, just as a heads-up.

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.

2 participants