Fix test helper discovery in pytest and sanitizer runner - #446
lucifer1004 wants to merge 2 commits into
Conversation
| return decorator | ||
|
|
||
|
|
||
| test_filter.__test__ = False |
There was a problem hiding this comment.
🔵 suggestion: Consider adding a one-line comment explaining why test_filter.__test__ = False is set (pytest / test_sanitizer.py opt-out marker for a test-prefixed helper). Future readers may otherwise be tempted to remove what looks like a stray attribute assignment.
🤖 v5
| for module_name in [os.path.splitext(f)[0] for f in files if f not in exclude_files] | ||
| for name, obj in inspect.getmembers(importlib.import_module(module_name)) | ||
| if inspect.isfunction(obj) and name.startswith('test') and 'test_filter' not in name | ||
| if inspect.isfunction(obj) and name.startswith('test') and getattr(obj, '__test__', True) |
There was a problem hiding this comment.
🔵 suggestion: With default discovery, the new tests/test_test_discovery.py module (test_pytest_discovery, test_sanitizer_discovery) will now itself be picked up and run under compute-sanitizer for every tool. These are pure-Python CPU tests that spawn pytest subprocesses and mock subprocess.run, so running them under the sanitizer is wasted time (and test_sanitizer_discovery re-executes this runner via runpy inside the sanitized process). Setting __test__ = False on them is not appropriate since pytest should collect them, so adding 'test_test_discovery.py' to exclude_files is the right lever. Not blocking.
🤖 v5
There was a problem hiding this comment.
The whole test has been removed.
|
|
||
|
|
||
| def test_pytest_discovery(): | ||
| with tempfile.TemporaryDirectory() as directory: |
There was a problem hiding this comment.
🔵 suggestion: The 60s timeout for the pytest subprocess may be tight: the sample module imports torch (via deep_gemm/testing/utils.py) in a fresh interpreter twice (collect-only + run). On cold caches or slow CI runners torch import alone can take 10–30s, so this could be flaky. Consider raising the timeout (e.g. 300s); a hang is still caught, just later.
🤖 v5
There was a problem hiding this comment.
The whole test has been removed.
| spec = importlib.util.spec_from_file_location(path.stem, path) | ||
| module = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(module) | ||
| package = ModuleType('deep_gemm') |
There was a problem hiding this comment.
🔵 suggestion: patch.object(os, 'listdir', ...), patch.object(importlib, 'import_module', ...) and patch.object(subprocess, 'run', ...) are global patches held while runpy.run_path executes the runner. This works today because runpy/argparse don't call these, but if the runner later grows other imports or filesystem calls this test could break in confusing ways. A short comment noting the assumption (and that import_module returns the same sample module for any name) would help maintainers.
🤖 v5
There was a problem hiding this comment.
The whole test has been removed.
🤖 ds-review-bot Code Reviewv6v5This MR fixes accidental collection of the v4本 MR 修复了测试助手被 pytest / sanitizer runner 误收集的问题。 标记只作用于装饰器工厂，被装饰的测试函数仍可被发现，条件过滤行为不变；实现简洁、与现有模式一致。未发现正确性、安全性、性能或行为回归问题，建议合并。 Files reviewed: 3 |
FP8 paged MQA logits gains PAGE_KV=32 (BLOCK_KV derived as min(PAGE_KV, 64), mirroring the FP4 sibling). Device-only update: this fork's host launcher still restricts paged FP8 to page 64, so page32 stays inert until host glue opts in (vllm-project#14). Validated on sm_120a: test_sm120_mqa.py + test_sm120_fp8_fp4.py 23/23 passed from a fresh JIT cache (only the pre-existing test_filter collection quirk remains, deepseek-ai#446). Signed-off-by: Zihua Wu <13583761+lucifer1004@users.noreply.github.com>
2 participants
Summary
test_filterdecorator factory with pytest's existing__test__ = Falseopt-out convention.test_filter.Why
Running an entire test module through pytest can collect the imported
test_filter(condition)helper and fail withfixture 'condition' not found. Direct script execution avoids this, while the sanitizer runner currently has a name-specific exclusion. The shared marker handles aliases and explicit opt-outs without accidentally excluding real tests such astest_filter_behavior.The marker applies only to the decorator factory. Decorated test functions remain discoverable, and existing condition-based execution/filtering behavior is unchanged.
Validation
Used a temporary CPU-only verification harness (not included in this PR):
78b6900.getattr(obj, "__test__", True)directly.git diff --check: passed.No GPU kernels, actual Compute Sanitizer execution, or native rebuild were required. This small PR is independent of the SM120/API migration work.