-
Notifications
You must be signed in to change notification settings - Fork 600
[CI] Add mtp_proposer ut #4397
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
base: main
Are you sure you want to change the base?
[CI] Add mtp_proposer ut #4397
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 unit tests for the MtpProposer. The tests cover initialization, model loading, and various helper methods for token generation. I've found several critical issues in the new tests that cause them to be broken or incorrect. One test has an incorrect assertion on an exception message. Another test has a syntax error that prevents it from running, as well as a logical flaw in its assertions. These issues need to be addressed to ensure the tests are reliable and correctly validate the code's behavior.
| with pytest.raises(AssertionError) as excinfo: | ||
| proposer.load_model(mock_model) | ||
|
|
||
| assert str(excinfo.value) == "" |
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.
The assertion assert str(excinfo.value) == "" is incorrect. When pytest catches an AssertionError, its string representation is the assertion expression that failed (e.g., 'assert 0 == 1'), not an empty string. This causes the test to fail incorrectly. To fix this, you should remove the assertion on the exception value if you only care that an AssertionError is raised.
| with pytest.raises(AssertionError) as excinfo: | |
| proposer.load_model(mock_model) | |
| assert str(excinfo.value) == "" | |
| with pytest.raises(AssertionError): | |
| proposer.load_model(mock_model) |
tests/ut/spec_decode/mtp_proposer.py
Outdated
|
|
||
| mock_backup_output = proposer.backup_next_token_ids | ||
|
|
||
| expected_backup_cpu = np.array([1000, 2000, 3000, 4000, 0, 0, ...])[:10] |
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.
This line has a syntax error. The ellipsis ... is not a valid element within a list used to initialize a numpy.array. This will raise a SyntaxError and prevent the test from running.
| expected_backup_cpu = np.array([1000, 2000, 3000, 4000, 0, 0, ...])[:10] | |
| expected_backup_cpu = np.array([1000, 2000, 3000, 4000, 0, 0, 0, 0, 0, 0]) |
tests/ut/spec_decode/mtp_proposer.py
Outdated
| expected_next_tokens = torch.tensor([103, 2, 3, 4], dtype=torch.int32, device="cpu") | ||
| assert torch.equal(next_token_ids, expected_next_tokens) |
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.
The expected value for next_token_ids is incorrect. The backup tokens are calculated from the mocked requests to be [1000, 2000, 3000, 4000]. The method under test correctly calculates the backup tokens and copies them to the GPU buffer. For discarded requests, it should fall back to these values. However, the test asserts against [2, 3, 4], which seems to be based on a stale mock value. The assertion should check against the correctly calculated backup tokens.
| expected_next_tokens = torch.tensor([103, 2, 3, 4], dtype=torch.int32, device="cpu") | |
| assert torch.equal(next_token_ids, expected_next_tokens) | |
| expected_next_tokens = torch.tensor([103, 2000, 3000, 4000], dtype=torch.int32, device="cpu") | |
| assert torch.equal(next_token_ids, expected_next_tokens) |
34c61ee to
560c144
Compare
Signed-off-by: chenmenglong <[email protected]>
560c144 to
397fb64
Compare
What this PR does / why we need it?
Add mtp_proposer ut
Does this PR introduce any user-facing change?
How was this patch tested?