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

Add PRC models to test_text_generation_example.py #1695

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

wenbinc-Bin
Copy link
Contributor

What does this PR do?

Add Chinese models to test_text_generation_example.py

Qwen/Qwen1.5-14B
Qwen/Qwen1.5-32B
THUDM/chatglm2-6b
Qwen/Qwen2.5-7B
Qwen/Qwen2.5-14B
Qwen/Qwen2.5-32B
Qwen/Qwen1.5-72B
Qwen/Qwen2-72B
Qwen/Qwen2.5-72B

@wenbinc-Bin wenbinc-Bin requested a review from regisss as a code owner January 14, 2025 05:49
@yafshar
Copy link
Contributor

yafshar commented Jan 14, 2025

@wenbinc-Bin would you please post the test results with these additions.

Copy link
Contributor

@yafshar yafshar left a comment

Choose a reason for hiding this comment

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

LGTM!

Hi @regisss, this PR is ready for your final review. Could you please take a look?

@yafshar
Copy link
Contributor

yafshar commented Jan 14, 2025

>>> RUN_SLOW=true GAUDI2_CI=1 python -m pytest tests/test_text_generation_example.py -s -v -k qwen --token $(TOKEN)

12 passed, 64 deselected, 1 warning in 640.85s (0:10:40)

@wenbinc-Bin
Copy link
Contributor Author

Also chaglm2

>>> GAUDI2_CI=1 python -m pytest tests/test_text_generation_example.py -s -v -k "THUDM/chatglm2-6b"

1 passed, 75 deselected, 1 warning in 26.50s

@yafshar
Copy link
Contributor

yafshar commented Jan 16, 2025

@wenbinc-Bin do we need to add all the tests? Maybe we can remove Qwen/Qwen1.5-14B & Qwen/Qwen2.5-14B if not necessary

Signed-off-by: Chen, Wenbin <[email protected]>
@wenbinc-Bin
Copy link
Contributor Author

@wenbinc-Bin do we need to add all the tests? Maybe we can remove Qwen/Qwen1.5-14B & Qwen/Qwen2.5-14B if not necessary

I can remove them. 14B models are relatively less important.

Copy link
Contributor

@yafshar yafshar left a comment

Choose a reason for hiding this comment

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

Hi @regisss, this PR is ready for your final review. Could you please take a look?

@@ -41,6 +41,7 @@
("bigcode/starcoder2-3b", 1, False, 261.07213776344133, True),
("adept/persimmon-8b-base", 4, False, 366.73968820698406, False),
("Qwen/Qwen1.5-7B", 4, False, 490.8621617893209, False),
("Qwen/Qwen1.5-32B", 4, False, 120, False),
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need both 7B and 32B of 1.5 if we already have 2.5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we said we support these models, we need to make sure there are no regression. We don't know what models users are using.

Copy link
Collaborator

Choose a reason for hiding this comment

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

one type of each is enough, we can't test all. but if you want to test on your own for each release, you can do that.

Comment on lines 61 to 62
("Qwen/Qwen2.5-7B", 4, False, 490, False),
("Qwen/Qwen2.5-32B", 4, False, 120, False),
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you remove 7B from multi test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean remove "Qwen/Qwen2.5-7B"? I think we need to test small models. Customers use small models to test and build their APPs.

@@ -87,6 +91,9 @@
("meta-llama/Meta-Llama-3-70B-Instruct", 8, 1, 64),
("facebook/opt-66b", 2, 1, 28.48069266504111),
("google/gemma-2-9b", 8, 1, 110.12610917383735),
("Qwen/Qwen1.5-72B", 2, 1, 26),
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need 1.5 and 2.0 if we have 2.5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we said we support these models, we need to make sure there are no regression. We don't know what models users are using.

@yafshar
Copy link
Contributor

yafshar commented Jan 30, 2025

@wenbinc-Bin can you please remove the extra ones as Libin suggested and only keep one type of each, so we can merge and close this PR. Thanks

@wenbinc-Bin
Copy link
Contributor Author

Ok, I update the PR.

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