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 convergence test, phi3 import and update benchmark #155

Merged
merged 2 commits into from
Aug 29, 2024
Merged

Conversation

yundai424
Copy link
Collaborator

@yundai424 yundai424 commented Aug 28, 2024

Summary

  1. bf16 loss rtol is a bit too loose, tighten it by 1 digit
  2. slightly loosen gemma1 atol, it's been failing
  3. old transformers version doesn't carry phi3 source code (testing on 4.40.1), since we claim support for >= 4.40.1, change the import a bit so things still work on older HF ver
  4. rerun all benchmark to reflect latest performance in preparation for new release

Testing Done

  • Hardware Type:
  • run make test to ensure correctness
  • run make checkstyle to ensure code style
  • run make test-convergence to ensure convergence

@yundai424 yundai424 changed the title fix convergence test and update benchmark [WIP] fix convergence test and update benchmark Aug 28, 2024
@yundai424 yundai424 changed the title [WIP] fix convergence test and update benchmark [WIP] fix convergence test, phi3 import and update benchmark Aug 28, 2024
@yundai424 yundai424 changed the title [WIP] fix convergence test, phi3 import and update benchmark fix convergence test, phi3 import and update benchmark Aug 28, 2024
ByronHsu
ByronHsu previously approved these changes Aug 29, 2024
Copy link
Collaborator

@ByronHsu ByronHsu left a comment

Choose a reason for hiding this comment

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

"transformers>=4.41.0",

btw, we actually bump transformers to >= 4.41.0

@@ -268,6 +267,8 @@ def apply_liger_kernel_to_phi3(
if cross_entropy:
modeling_phi3.CrossEntropyLoss = LigerCrossEntropyLoss
if fused_linear_cross_entropy:
from liger_kernel.transformers.model.phi3 import lce_forward as phi3_lce_forward
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can move this back. we don't want to support too old transformers

Copy link
Collaborator Author

@yundai424 yundai424 Aug 29, 2024

Choose a reason for hiding this comment

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

oh! apparently my mind swapped the 0 and 1 in the version, lemme revert these phi3 related stuff

@yundai424 yundai424 enabled auto-merge (squash) August 29, 2024 03:01
@yundai424 yundai424 merged commit f47dd81 into main Aug 29, 2024
2 checks passed
@yundai424 yundai424 deleted the yudai/release branch August 29, 2024 03:01
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.

2 participants