Skip to content

Fix slm training bug (https://github.com/yl4579/StyleTTS2/issues/72) #74

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 24, 2023
Merged

Fix slm training bug (https://github.com/yl4579/StyleTTS2/issues/72) #74

merged 2 commits into from
Nov 24, 2023

Conversation

kmn1024
Copy link
Contributor

@kmn1024 kmn1024 commented Nov 24, 2023

SLM joint training bug in finetuning code: #72 (comment)

@yl4579
Copy link
Owner

yl4579 commented Nov 24, 2023

It’s probably related to this: #15
I couldn’t reproduce because my pytorch version doesn’t have this problem. Does the order of backward matter though? Do you also have to change the order of the generator loss backward?

@kmn1024
Copy link
Contributor Author

kmn1024 commented Nov 24, 2023

Sorry you are correct! The order of backwards also needs to be changed. The run now works on my setup.

For completeness, this if my setup:

> python -c "import torch; print(torch.version.cuda)"
12.1
> python -c "import torch; print(torch.__version__)"
2.1.1
> nvidia-smi
Fri Nov 24 04:49:56 2023       
+---------------------------------------------------------------------------------------+
| NVIDIA-SMI 545.23.06              Driver Version: 545.23.06    CUDA Version: 12.3     |
|-----------------------------------------+----------------------+----------------------+
| GPU  Name                 Persistence-M | Bus-Id        Disp.A | Volatile Uncorr. ECC |
| Fan  Temp   Perf          Pwr:Usage/Cap |         Memory-Usage | GPU-Util  Compute M. |
|                                         |                      |               MIG M. |
|=========================================+======================+======================|
|   0  NVIDIA RTX A6000               On  | 00000000:05:00.0 Off |                  Off |
| 30%   51C    P2             118W / 300W |  32545MiB / 49140MiB |     28%      Default |
|                                         |                      |                  N/A |
+-----------------------------------------+----------------------+----------------------+
|   1  NVIDIA RTX A6000               On  | 00000000:45:00.0 Off |                  Off |
| 30%   53C    P2             111W / 300W |  27637MiB / 49140MiB |     30%      Default |
|                                         |                      |                  N/A |
+-----------------------------------------+----------------------+----------------------+
|   2  NVIDIA RTX A6000               On  | 00000000:85:00.0 Off |                  Off |
| 30%   55C    P2             113W / 300W |  27617MiB / 49140MiB |     22%      Default |
|                                         |                      |                  N/A |
+-----------------------------------------+----------------------+----------------------+
|   3  NVIDIA RTX A6000               On  | 00000000:C5:00.0 Off |                  Off |
| 30%   52C    P2             107W / 300W |  27529MiB / 49140MiB |     26%      Default |
|                                         |                      |                  N/A |
+-----------------------------------------+----------------------+----------------------+

@kmn1024 kmn1024 mentioned this pull request Nov 24, 2023
Closed
@yl4579 yl4579 merged commit 23c16b7 into yl4579:main Nov 24, 2023
@yl4579
Copy link
Owner

yl4579 commented Nov 24, 2023

I actually found a bug in this fix. It calls optimizer.zero_grad() twice when the discriminator loss isn’t 0, so it implicitly overwrites the gradient in that iteration and optimizes against the generator loss. I think we have to bring optimizer steps lines before the discriminator line as well.

@yl4579
Copy link
Owner

yl4579 commented Nov 24, 2023

Can you please make the change and test if it doesn’t cause any problems in your settings?

@kmn1024
Copy link
Contributor Author

kmn1024 commented Nov 24, 2023 •
edited
Loading

Ah, another good catch. Trying...

This issue also seems to be in train_second.py?

@yl4579
Copy link
Owner

yl4579 commented Nov 24, 2023

Yes, so if you could verify it has no problems running on your system I’ll change that too. I couldn’t reproduce in my environment.

@kmn1024 kmn1024 mentioned this pull request Nov 24, 2023
Merged
yl4579 added a commit that referenced this pull request Nov 24, 2023
@yl4579
Merge pull request #75 from kmn1024/finetune-slm-bug
d783aec 
Continued fix of SLM training (see #74)
yl4579 added a commit that referenced this pull request Nov 24, 2023
@yl4579
fix buggy logic from #15 per #74
e2fabb3
nawed2611 pushed a commit to team-listnr/StyleTTS2 that referenced this pull request Feb 8, 2024
@yl4579
Merge pull request yl4579#74 from kmn1024/finetune-slm-bug
027b085 
Fix slm training bug (yl4579#72)
nawed2611 pushed a commit to team-listnr/StyleTTS2 that referenced this pull request Feb 8, 2024
@kmn1024
Move generator related optimizer.steps up, so they won't get zeroed o…
ecec3a7 
…ut when . See yl4579#74 (comment). Also tuck all logic related to SLM under
nawed2611 pushed a commit to team-listnr/StyleTTS2 that referenced this pull request Feb 8, 2024
@yl4579
Merge pull request yl4579#75 from kmn1024/finetune-slm-bug
1bc21c8 
Continued fix of SLM training (see yl4579#74)
nawed2611 pushed a commit to team-listnr/StyleTTS2 that referenced this pull request Feb 8, 2024
@yl4579
fix buggy logic from yl4579#15 per yl4579#74
90b9e6d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kmn1024 @yl4579