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 cache config error #3138

Closed
wants to merge 1 commit into from

Conversation

cocktailpeanut
Copy link

What does this fix?

Fixes this problem: #3074

What was the problem?

When the TTS library fails to download the config files for some reason (which has been happening very often recently), we end up with config.json, model.pth, vocab.json files that are empty (0 bytes).

When this happens, the json.load(f) line fails and it doesn't even reach the next line where it compares config_local with config_remote.

This is made worse by the exception handler at https://github.com/cocktailpeanut/TTS/blob/33c95daa55808817b67e5bf66c68134889059c15/TTS/utils/manage.py#L402 NOT handling the error and calling pass.

To fix this, I've added the try/except clause, and if the try fails it sets theconfig_local to None and at least moves to the next line and compares with config_remote, at which point it should correctly recognize that they are different and retry the download.

@CLAassistant
Copy link

CLAassistant commented Nov 3, 2023

CLA assistant check
All committers have signed the CLA.

config_local = json.load(f)
try:
config_local = json.load(f)
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

A bare except: is not a good idea. For instance, a KeyboardInterrupt will be caught by it.

Instead, catch the particular exception that json.load() would raise for an empty file (json.JSONDecodeError).

Copy link
Author

Choose a reason for hiding this comment

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

Actually IMO for this case a bare except is exactly what is needed, if you consider what problem this commit is solving.

The problem here is that it completely avoids downloading anything if something went wrong. The worst case scenario here is it re-downloads the files. It doesn't matter what goes wrong during the json.load, if something unexpected happens, it should just redownload the files.

Copy link
Member

Choose a reason for hiding this comment

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

IT should avoid downloading anything if something is wrong. Even if you download the remaining files, the model most prob would not work. We should debug why the files empty and fix that. Any ideas ?

Copy link
Contributor

Choose a reason for hiding this comment

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

A bare except: is never a good idea, though. except Exception: maybe, if you really need to catch everything, but except:, never.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for that

Copy link

stale bot commented Dec 15, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. You might also look our discussion channels.

@stale stale bot added the wontfix This will not be worked on but feel free to help. label Dec 15, 2023
@stale stale bot closed this Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on but feel free to help.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants