-
-
Notifications
You must be signed in to change notification settings - Fork 11k
fix(tests): Resolve late binding of loop variable in assert message lambda #26249
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(tests): Resolve late binding of loop variable in assert message lambda #26249
Conversation
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 correctly resolves a late binding issue with a loop variable in lambda functions within the test suite, which improves the robustness and debuggability of the tests. The change is well-explained and follows Python best practices. However, the pull request also includes an unrelated file named hq that seems to be the output of a git command. This file should be removed before merging to keep the commit history clean.
db62b25 to
d0eb63d
Compare
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 correctly addresses a latent bug related to the late binding of a loop variable within lambda functions used for assertion messages in tests. By capturing the loop variable i by value using a default argument (i=i), the change ensures that assertion failure messages will be accurate and not misleading. This is a good practice and improves the robustness and debuggability of the tests. The implementation is clean and effectively resolves the issue identified by the Ruff linter (B023).
|
Thanks, let's merge #26247 first though |
4d1cabe to
579ceb4
Compare
579ceb4 to
b97b99a
Compare
|
Please follow these steps to pass pre-commit now that we have changed the linter to
|
8490634 to
5a04735
Compare
|
Please don't change |
…ambda The `msg` parameter for `torch.testing.assert_close` within the `test_mamba_chunk_scan_cont_batch_prefill_chunking` test function used a lambda defined inside a for-loop. This lambda captured the loop variable `i`, triggering the `B023` warning from the Ruff linter. Ruff implements this rule (originally from flake8-bugbear) to detect "late binding" issues, where a lambda captures a reference to a variable, not its value at the time of definition. Although the current test runner likely executes the lambda immediately upon assertion failure, this pattern is a latent bug. Future changes could defer message generation, causing all failure messages to incorrectly display the final value of `i`, which would be misleading for debugging. This commit fixes the issue by using a default argument in the lambda (`lambda x, i=i: ...`) to capture the value of `i` at definition time. This robustly resolves the potential bug and allows for the removal of the `# noqa: B023` suppressions. Signed-off-by: lyd1992 <liuyudong@iscas.ac.cn> Signed-off-by: ihb2032 <1355790728@qq.com
5a04735 to
158751d
Compare
|
I'll override DCO this time |
…ambda (vllm-project#26249) Signed-off-by: lyd1992 <liuyudong@iscas.ac.cn> Signed-off-by: ihb2032 <1355790728@qq.com Signed-off-by: Tomer Asida <57313761+tomeras91@users.noreply.github.com>
…ambda (vllm-project#26249) Signed-off-by: lyd1992 <liuyudong@iscas.ac.cn> Signed-off-by: ihb2032 <1355790728@qq.com Signed-off-by: Karan Goel <3261985+karan@users.noreply.github.com>
…ambda (vllm-project#26249) Signed-off-by: lyd1992 <liuyudong@iscas.ac.cn> Signed-off-by: ihb2032 <1355790728@qq.com
…ambda (vllm-project#26249) Signed-off-by: lyd1992 <liuyudong@iscas.ac.cn> Signed-off-by: ihb2032 <1355790728@qq.com Signed-off-by: xuebwang-amd <xuebwang@amd.com>
…ambda (vllm-project#26249) Signed-off-by: lyd1992 <liuyudong@iscas.ac.cn> Signed-off-by: ihb2032 <1355790728@qq.com
…ambda (vllm-project#26249) Signed-off-by: lyd1992 <liuyudong@iscas.ac.cn> Signed-off-by: ihb2032 <1355790728@qq.com
…ambda (vllm-project#26249) Signed-off-by: lyd1992 <liuyudong@iscas.ac.cn> Signed-off-by: ihb2032 <1355790728@qq.com Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Purpose
This PR resolves a latent bug in the Mamba SSD tests identified by the Ruff linter (
B023).Ruff's
B023rule, which originates fromflake8-bugbear, flagged an issue in thetest_mamba_chunk_scan_cont_batch_prefill_chunkingfunction. Themsgparameter fortorch.testing.assert_closewas assigned alambdafunction created inside aforloop. This lambda captured the loop variableiby reference, not by value, creating a "late binding" issue.While this might not cause incorrect behavior with the current test runner, it posed a significant risk: if the test framework's behavior were to change to defer error message generation, any assertion failure would incorrectly report the final value of
i, making debugging difficult and misleading.This change corrects the variable capture by using a default argument in the lambda (
lambda x, i=i: ...), which ensures the value ofiis captured at definition time. This makes the test code more robust, eliminates the potential for misleading error messages, and allows for the removal of the# noqa: B023suppressions.Test Plan
Test Result