Skip to content

Fix train loss to be normal regardless of num_items_in_batch - #35527

Closed
petil777 wants to merge 1 commit into
huggingface:mainfrom
petil777:me/suggest
Closed

Fix train loss to be normal regardless of num_items_in_batch#35527
petil777 wants to merge 1 commit into
huggingface:mainfrom
petil777:me/suggest

Conversation

@petil777

@petil777 petil777 commented Jan 6, 2025

Copy link
Copy Markdown

What does this PR do?

This PR tries to fix train loss scale bug.

Currently, num_items_in_batch is calculated by sum of labels having no IGNORE_INDEX.
In case labels having some no IGNORE_INDEX, num_items_in_batch will be not None, leading to grad_acc not applied.

(Ex. Any kinds of generative loss training having labels to be trained to remember some format)

#35438 also pointed out that this num_items_in_batch can make train loss weird.
Also, #35203 says grad_norm should be fixed by manipulating num_items_in_batch.

I'll close this PR if there is any better fixing logic or this can affect other side effects.

Please fix train loss scale bug in no time.

+) Current code also makes accelerator backward loss bigger if accumulation step is larger with same global_batch settings.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@muellerzr and @SunMarc

- Currently num_items_in_batch is calculated by sum of labels having no IGNORE_INDEX
- In case labels having some no IGNORE_INDEX, num_items_in_batch is not None, leading to grad_acc not applied
@hiyouga

hiyouga commented Jan 7, 2025

Copy link
Copy Markdown
Contributor

I think this fix is wrong, you can simply perform some experiments on Llama models with [micro bsz=1, grad accu=4] and [micro bsz=4, grad accu=1]


We have already found abnormal loss in Llama3 model using [micro bsz=1, grad accu=4] (correct value is ~1.0)
image

@petil777

petil777 commented Jan 7, 2025

Copy link
Copy Markdown
Author

I think this fix is wrong, you can simply perform some experiments on Llama models with [micro bsz=1, grad accu=4] and [micro bsz=4, grad accu=1]

We have already found abnormal loss in Llama3 model using [micro bsz=1, grad accu=4] (correct value is ~1.0) image

Thanks for the reply. Due to lack of resource, I couldn't fully experiment (Still hard to experiment...). Seems like the code snippet in #35438 may have loss error as you pasted.(Which is kind of default training code, no need to set labels IGNORE_INDEX selectively).
Since all of them are affected by num_items_in_batch, as #35438 suggested, if we carefully handle num_items_in_batch and use that value to adjust the right value when calculating loss/grad (Ex. loss / grad_acc * num_items_in_batch), do you think this kind of access can solve this problem?

@hiyouga

hiyouga commented Jan 7, 2025

Copy link
Copy Markdown
Contributor

@petil777 No, we didn't use the code from #35438, instead, we performed experiments based this PR (#35527 ). We just want to validate that the code based on this PR is incorrect.
Please be responsible for your code. Results without experimental verification are not convincing. You can choose a smaller model to verify the results (such as TinyLlama).

@Rocketknight1

Copy link
Copy Markdown
Member

cc @muellerzr @SunMarc

@petil777 petil777 closed this Jan 9, 2025
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