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 labels & eos_token for SFT #819

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

li-plus
Copy link
Contributor

@li-plus li-plus commented Nov 28, 2023

Fixed two issues:

  • Padding should be ignored in training. Their labels should be set to -100 for CrossEntropyLoss to ignore them.
  • Append correct eos_token to the response text. Otherwise the default <|endoftext|> will be tokenized into multiple tokens.

@li-plus
Copy link
Contributor Author

li-plus commented Sep 8, 2024

Just rebased and fixed code format. Could anybody merge this PR since it's been approved. I don't have merge permission.

@tjruwase
Copy link
Contributor

tjruwase commented Sep 9, 2024

@li-plus, apologies for the delay on this. Can you please share a bit more details on the motivation for this PR. For example, what improvements did you observe in your workloads? Thanks!

@li-plus
Copy link
Contributor Author

li-plus commented Sep 10, 2024

@tjruwase These are bugs that should be fixed. In master code, labels for padding are not ignored. Model will be trained to generate padding tokens, which is unnecessary. Also, eos token is not recognized as a single token. If you examine the input_ids in the training_scripts/opt/single_gpu/run_1.3b.sh example, it's tokenized into [49069, 15483, 1397, 1116, 29015, 15483, 15698], i.e. ['.<', '|', 'end', 'of', 'text', '|', '>'], which is wrong. This PR fixed these bugs.

@tjruwase
Copy link
Contributor

@li-plus, got it. Thanks for this great contribution.

@tjruwase tjruwase merged commit a256c04 into microsoft:master Sep 10, 2024
2 checks passed
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