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

Fix ModelManager.list_models() #3128

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Fix ModelManager.list_models() #3128

merged 2 commits into from
Nov 8, 2023

Conversation

eginhard
Copy link
Contributor

This removes the hard-coded model_type variable, so that the following now returns all models, not only tts_models:

from TTS.utils.manage import ModelManager
ModelManager().list_models()

The second commit addresses the output of make lint for that file.

Addressed the following:
TTS/utils/manage.py:307:12: R1705: Unnecessary "else" after "return" (no-else-return)
TTS/utils/manage.py:308:21: W1514: Using open without explicitly specifying an encoding (unspecified-encoding)
TTS/utils/manage.py:299:4: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)
TTS/utils/manage.py:299:4: R0201: Method could be a function (no-self-use)
TTS/utils/manage.py:314:4: R0201: Method could be a function (no-self-use)
@CLAassistant
Copy link

CLAassistant commented Oct 31, 2023

CLA assistant check
All committers have signed the CLA.

"""Ask the user to agree to the terms of service"""
tos_path = os.path.join(model_full_path, "tos_agreed.txt")
if not os.path.exists(tos_path):
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 This whole if is gone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same condition is already checked in tos_agreed(), so no need to repeat it here (and if it were kept, a return True would be necessary after the if block actually).

Copy link
Contributor

@akx akx Oct 31, 2023

Choose a reason for hiding this comment

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

Ah, I see. (Also, it seems this wouldn't have taken the COQUI_TOS_AGREED envvar into account...)

@akx
Copy link
Contributor

akx commented Oct 31, 2023

@eginhard make lint isn't being run in the repo right now (see #3004, #3127), I would advise against running it locally too, because there'd be a whole bunch of changes that are irrelevant...

@eginhard
Copy link
Contributor Author

@eginhard make lint isn't being run in the repo right now (see #3004, #3127), I would advise against running it locally too, because there'd be a whole bunch of changes that are irrelevant...

Ok, I'll keep this in mind in the future. Looking forward to ruff linting :)

@erogol
Copy link
Member

erogol commented Nov 8, 2023

yeah, linting is too much for ML projects.

@erogol erogol merged commit 99edd6d into coqui-ai:dev Nov 8, 2023
53 checks passed
@eginhard eginhard deleted the fix-utils-manage branch November 9, 2023 15:53
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.

4 participants