-
Notifications
You must be signed in to change notification settings - Fork 544
[E2E][MM] Add e2e tests for InternVL model #3796
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
Conversation
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
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 adds end-to-end tests for the InternVL model family, which is a good validation for the related changes. The test covers several models and correctly verifies the consistency between eager and graph execution modes. I've found a significant amount of duplicated code in the test logic for the two modes. I've suggested a refactoring to improve maintainability by removing this duplication.
|
@wangxiyuan InternVL2-8B and InternVL2_5-8B still have 27,985 and 8,270 downloads last month in huggingface. I think we should keep them in the e2e tests. |
Signed-off-by: gcanlin <[email protected]>
Signed-off-by: gcanlin <[email protected]>
Signed-off-by: gcanlin <[email protected]>
Signed-off-by: gcanlin <[email protected]>
What this PR does / why we need it?
As a validation for #3664, add end-to-end tests to monitor the InternVL model and ensure its continuous proper operation. This PR is only for single-card. So the models that have more parameters than 8B like 78B are needed to test using multi-cards.
Does this PR introduce any user-facing change?
None.
How was this patch tested?
pytest -sv tests/e2e/singlecard/multi-modal/test_internvl.py