Create test_deepseek_v32_config.py#35247
Create test_deepseek_v32_config.py#35247puririshi98 wants to merge 20 commits intovllm-project:mainfrom
Conversation
This test aims to cover: vllm-project#34899 Not sure where this test would be put in the tests folder, open to suggestions from reviewers. Signed-off-by: Rishi Puri <riship@nvidia.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a new test file, test_deepseek_v32_config.py, to validate the changes from PR #34899. The tests cover the architectural changes and config update logic for DeepseekV32ForCausalLM. While the tests are generally well-structured, I've found a significant issue where one of the tests is ineffective because it checks for a class's removal in the wrong module. This could lead to a false sense of security that the refactoring was completed correctly.
| from vllm.model_executor.models import config as config_module | ||
|
|
||
| assert not hasattr(config_module, 'DeepseekV3ForCausalLM'), ( | ||
| "DeepseekV3ForCausalLM workaround class should be removed" | ||
| ) |
There was a problem hiding this comment.
This test is intended to verify the removal of the DeepseekV3ForCausalLM class. However, it only checks for its absence in the vllm.model_executor.models.config module. Based on the existing codebase, this class is defined elsewhere (e.g., in vllm.model_executor.models.deepseek_v2.py) and is not expected to be in the config module. This means the test will pass regardless of whether the class has been properly removed, making it ineffective.
To correctly verify the removal, the assertion should check against the vllm.model_executor.models package, which is where the class would be exposed if it still exists.
| from vllm.model_executor.models import config as config_module | |
| assert not hasattr(config_module, 'DeepseekV3ForCausalLM'), ( | |
| "DeepseekV3ForCausalLM workaround class should be removed" | |
| ) | |
| from vllm.model_executor import models | |
| assert not hasattr(models, 'DeepseekV3ForCausalLM'), ( | |
| "DeepseekV3ForCausalLM workaround class should be removed" | |
| ) |
|
Hi @puririshi98, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
|
Hi @puririshi98, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
|
Hi @puririshi98, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
…eepseek_v32_config.py Signed-off-by: Rishi Puri <riship@nvidia.com>
Signed-off-by: Rishi Puri <riship@nvidia.com>
|
kv cache dtype is not converted automatically from "fp8" to "fp8_ds_mla" after PR #35891 for DS v3.2 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new test file to validate changes from a previous pull request related to DeepseekV32ForCausalLM configuration. The tests are generally well-structured and use mocking effectively to isolate the logic under test. However, I've identified a significant issue in two of the tests that are intended to verify the removal of a class; they are checking the wrong module, which could lead to a false sense of security. I've provided specific suggestions to correct this.
| from vllm.model_executor.models import config as config_module | ||
|
|
||
| assert not hasattr(config_module, "DeepseekV3ForCausalLM"), ( | ||
| "DeepseekV3ForCausalLM workaround class should be removed" | ||
| ) |
There was a problem hiding this comment.
This test incorrectly checks for the removal of DeepseekV3ForCausalLM from the config module. This class was originally defined in vllm.model_executor.models.deepseek_v2, so the test should check that module to correctly verify its removal. As written, this test does not provide the intended verification.
| from vllm.model_executor.models import config as config_module | |
| assert not hasattr(config_module, "DeepseekV3ForCausalLM"), ( | |
| "DeepseekV3ForCausalLM workaround class should be removed" | |
| ) | |
| from vllm.model_executor.models import deepseek_v2 as models_module | |
| assert not hasattr(models_module, "DeepseekV3ForCausalLM"), ( | |
| "DeepseekV3ForCausalLM workaround class should be removed" | |
| ) |
| from vllm.model_executor.models import config as config_module | ||
|
|
||
| assert not hasattr(config_module, "DeepseekV3ForCausalLM") |
There was a problem hiding this comment.
Similar to the test_deepseek_v3_class_removed test, this check for the removal of DeepseekV3ForCausalLM is targeting the wrong module. The class was defined in vllm.model_executor.models.deepseek_v2, not config. The test should be updated to check the correct module to ensure the test is effective.
| from vllm.model_executor.models import config as config_module | |
| assert not hasattr(config_module, "DeepseekV3ForCausalLM") | |
| from vllm.model_executor.models import deepseek_v2 as models_module | |
| assert not hasattr(models_module, "DeepseekV3ForCausalLM") |
This test aims to cover: #34899
Not sure where this test would be put in the tests folder, open to suggestions from reviewers.
This is part of the NVIDIA effort to add CI to upstream github.