-
Notifications
You must be signed in to change notification settings - Fork 295
fix unit test #1173
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 unit test #1173
Conversation
Summary of ChangesHello @hiworldwzj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on enhancing the maintainability and robustness of the unit test suite. It involves a substantial refactoring of test file locations and import paths to align with a more generalized common base model architecture. Additionally, it introduces mechanisms to gracefully handle missing dependencies and hardware-specific features, preventing unnecessary test failures. A notable change is the removal of several dedicated FP8 quantization tests, suggesting an evolution in how these features are validated. Overall, the changes aim to streamline the testing process and improve the clarity of internal data structures. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request primarily focuses on fixing and refactoring unit tests. The changes include moving kernels to a common location and updating import paths, renaming variables for clarity, and simplifying function call APIs. Several tests are now correctly skipped if their dependencies (like custom kernels or FP8 support) are not met, which improves the robustness of the test suite. A critical fix was made to a benchmark test by adding torch.cuda.Event.synchronize() for accurate timing. Overall, these changes significantly improve the correctness and maintainability of the unit tests. I have added a couple of minor suggestions to improve consistency in the pytest.skip messages.
| @@ -1,4 +1,7 @@ | |||
| import pytest | |||
|
|
|||
| pytest.skip(reason="need install lightllmkernel", allow_module_level=True) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency across the test suite, it's better to use a consistent name for the kernel in the skip reason. In test_ppl_int8kv_flash_decoding_diverse.py, the reason is 'need install lightllmKernel'. Please consider using the same capitalization here.
| pytest.skip(reason="need install lightllmkernel", allow_module_level=True) | |
| pytest.skip(reason="need install lightllmKernel", allow_module_level=True) |
|
|
||
|
|
||
| if not is_fp8_native_supported(): | ||
| pytest.skip("not support fp8 in this gpu card", allow_module_level=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better readability and consistency, it's good practice to use the reason keyword argument for pytest.skip. Also, to align with other tests checking for FP8 support (e.g., in test_moe_silu_and_mul_mix_quant_ep.py), consider using a more descriptive and consistent reason like 'not support fp8 test in this gpu card'.
| pytest.skip("not support fp8 in this gpu card", allow_module_level=True) | |
| pytest.skip(reason="not support fp8 test in this gpu card", allow_module_level=True) |
No description provided.