Conversation
Signed-off-by: Kunshang Ji <kunshang.ji@intel.com>
There was a problem hiding this comment.
Pull request overview
Adds a small “model-aware test suite resolver” that maps model/profile selections to kernel names and then to concrete pytest node ids/paths, enabling generating targeted unit-test invocations for specific model variants.
Changes:
- Introduces CLI scripts to resolve model + profile (+ optional extra kernels) into pytest targets (lines/args/json output).
- Adds
MODEL_TO_KERNELSandKERNEL_TO_TESTSmapping tables to define the suite composition. - Adds
tests/suitespackage initialization.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/resolve_model_suite.py | CLI entry point that loads the suite mappings from tests.suites.* and prints resolved pytest targets. |
| tests/suites/resolve_model_suite.py | In-tree resolver CLI (and reusable resolve_suite) intended to be run to generate the pytest target list. |
| tests/suites/model_kernel_map.py | Seed mapping from model variants to kernel “profiles”. |
| tests/suites/kernel_test_map.py | Mapping from kernel names/aliases to pytest node ids / paths. |
| tests/suites/init.py | Declares tests.suites as a package. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #llama4 use fast topk | ||
| "fused_moe_prologue", |
There was a problem hiding this comment.
Comment formatting/grammar: add a space after #, capitalize, and use consistent wording (e.g., "Llama4 uses fast top-k").
| import sys | ||
| from collections.abc import Iterable | ||
|
|
||
| try: | ||
| from .kernel_test_map import KERNEL_TO_TESTS | ||
| from .model_kernel_map import MODEL_TO_KERNELS | ||
| except ImportError: |
There was a problem hiding this comment.
This module uses relative imports (from .kernel_test_map ...) but is intended to be run as a script (per PR description). Running python tests/suites/resolve_model_suite.py will fail with "attempted relative import with no known parent package". Add a script-friendly import fallback (or adjust sys.path to include repo root) and/or document running it via python -m tests.suites.resolve_model_suite.
| import sys | |
| from collections.abc import Iterable | |
| try: | |
| from .kernel_test_map import KERNEL_TO_TESTS | |
| from .model_kernel_map import MODEL_TO_KERNELS | |
| except ImportError: | |
| import sys | |
| import os | |
| from collections.abc import Iterable | |
| try: | |
| from .kernel_test_map import KERNEL_TO_TESTS | |
| from .model_kernel_map import MODEL_TO_KERNELS | |
| except ImportError: | |
| # Fallback for running as a script, e.g. `python tests/suites/resolve_model_suite.py` | |
| this_dir = os.path.dirname(os.path.abspath(__file__)) | |
| if this_dir not in sys.path: | |
| sys.path.insert(0, this_dir) |
| resolved_kernels: list[str] = [] | ||
|
|
||
| unknown_models = [ | ||
| model for model in models if model not in MODEL_TO_KERNELS | ||
| ] | ||
| if unknown_models: | ||
| raise ValueError( | ||
| f"Unknown model(s): {', '.join(unknown_models)}. " | ||
| f"Known models: {', '.join(sorted(MODEL_TO_KERNELS))}") | ||
|
|
||
| missing_profiles: list[str] = [] | ||
| for model in models: | ||
| profile_map = MODEL_TO_KERNELS[model] | ||
| for profile in profiles: | ||
| kernels = profile_map.get(profile) | ||
| if kernels is None: | ||
| missing_profiles.append(f"{model}:{profile}") | ||
| continue | ||
| resolved_kernels.extend(kernels) | ||
|
|
||
| if missing_profiles: | ||
| raise ValueError("Missing profile mapping(s): " + | ||
| ", ".join(missing_profiles) + | ||
| ". Add them to tests/suites/model_kernel_map.py") | ||
|
|
||
| resolved_kernels.extend(extra_kernels) | ||
| resolved_kernels = _ordered_unique(resolved_kernels) | ||
|
|
||
| missing_kernel_tests = [ | ||
| kernel for kernel in resolved_kernels if kernel not in KERNEL_TO_TESTS | ||
| ] | ||
| if missing_kernel_tests: | ||
| raise ValueError("Kernel(s) missing test mapping: " + | ||
| ", ".join(missing_kernel_tests) + | ||
| ". Add them to tests/suites/kernel_test_map.py") | ||
|
|
||
| tests: list[str] = [] | ||
| for kernel in resolved_kernels: | ||
| tests.extend(KERNEL_TO_TESTS[kernel]) | ||
|
|
||
| return resolved_kernels, _ordered_unique(tests) | ||
|
|
||
|
|
There was a problem hiding this comment.
There is near-identical suite-resolution logic in both tools/resolve_model_suite.py and tests/suites/resolve_model_suite.py. This duplication risks the two entry points diverging over time; consider making one a thin wrapper around a shared implementation (single source of truth).
| resolved_kernels: list[str] = [] | |
| unknown_models = [ | |
| model for model in models if model not in MODEL_TO_KERNELS | |
| ] | |
| if unknown_models: | |
| raise ValueError( | |
| f"Unknown model(s): {', '.join(unknown_models)}. " | |
| f"Known models: {', '.join(sorted(MODEL_TO_KERNELS))}") | |
| missing_profiles: list[str] = [] | |
| for model in models: | |
| profile_map = MODEL_TO_KERNELS[model] | |
| for profile in profiles: | |
| kernels = profile_map.get(profile) | |
| if kernels is None: | |
| missing_profiles.append(f"{model}:{profile}") | |
| continue | |
| resolved_kernels.extend(kernels) | |
| if missing_profiles: | |
| raise ValueError("Missing profile mapping(s): " + | |
| ", ".join(missing_profiles) + | |
| ". Add them to tests/suites/model_kernel_map.py") | |
| resolved_kernels.extend(extra_kernels) | |
| resolved_kernels = _ordered_unique(resolved_kernels) | |
| missing_kernel_tests = [ | |
| kernel for kernel in resolved_kernels if kernel not in KERNEL_TO_TESTS | |
| ] | |
| if missing_kernel_tests: | |
| raise ValueError("Kernel(s) missing test mapping: " + | |
| ", ".join(missing_kernel_tests) + | |
| ". Add them to tests/suites/kernel_test_map.py") | |
| tests: list[str] = [] | |
| for kernel in resolved_kernels: | |
| tests.extend(KERNEL_TO_TESTS[kernel]) | |
| return resolved_kernels, _ordered_unique(tests) | |
| """Resolve suite by delegating to tests.suites.resolve_model_suite. | |
| This function is a thin wrapper to avoid duplicating suite-resolution | |
| logic in both tools and tests; the tests module is the single source | |
| of truth for the implementation. | |
| """ | |
| repo_root = Path(__file__).resolve().parents[1] | |
| if str(repo_root) not in sys.path: | |
| sys.path.insert(0, str(repo_root)) | |
| module = importlib.import_module("tests.suites.resolve_model_suite") | |
| # Delegate to the shared implementation to ensure consistent behavior. | |
| return module.resolve_suite( | |
| models=models, | |
| profiles=profiles, | |
| extra_kernels=extra_kernels, | |
| ) |
| #llama4 use fast topk | ||
| "fused_moe_prologue", |
There was a problem hiding this comment.
Comment formatting/grammar: add a space after #, capitalize, and use consistent wording (e.g., "Llama4 uses fast top-k"). This helps readability and matches common repo style.
| "fused_moe_mxfp4_a16", # todo fix | ||
| "fused_moe_grouped_gemm_xe", | ||
| "fused_moe_grouped_gemm_mxfp4", # todo fix |
There was a problem hiding this comment.
Comment formatting: # todo fix should be capitalized and made a complete TODO (e.g., # TODO: fix ...) so it’s searchable and clear.
| "fused_moe_mxfp4_a16", # todo fix | |
| "fused_moe_grouped_gemm_xe", | |
| "fused_moe_grouped_gemm_mxfp4", # todo fix | |
| "fused_moe_mxfp4_a16", # TODO: verify kernel mapping for this entry | |
| "fused_moe_grouped_gemm_xe", | |
| "fused_moe_grouped_gemm_mxfp4", # TODO: verify kernel mapping for this entry |
| from kernel_test_map import KERNEL_TO_TESTS | ||
| from model_kernel_map import MODEL_TO_KERNELS | ||
|
|
||
|
|
There was a problem hiding this comment.
_split_csv uses the PEP 604 union syntax (str | None), which requires Python 3.10+. This repo declares Python >=3.9, so this will raise a SyntaxError on 3.9 and fail CI. Use Optional[str] (or Union[str, None]) instead.
| def _split_csv(values: str | None) -> list[str]: | ||
| if not values: | ||
| return [] | ||
| return [item.strip() for item in values.split(",") if item.strip()] |
There was a problem hiding this comment.
_split_csv uses the PEP 604 union syntax (str | None), which requires Python 3.10+. This repo supports Python 3.9, so this will raise a SyntaxError on 3.9. Use Optional[str] (or Union[str, None]) instead.
| ], | ||
| "all_fused_moe_grouped_gemm": [ | ||
| "tests/fused_moe/test_grouped_gemm.py", | ||
| "tests/fused_moe/test_grouped_gemm.py::test_grouped_gemm", |
There was a problem hiding this comment.
all_fused_moe_grouped_gemm includes both the whole file and a specific node id from the same file. If the resolved test list is passed to pytest, this can cause test_grouped_gemm to run twice (once via the file, once via the node id). Prefer either the file path alone or only the explicit node ids.
| "tests/fused_moe/test_grouped_gemm.py::test_grouped_gemm", |
| "tests/test_moe_align_block_size.py::test_moe_align_block_size_with_expert_map", | ||
| ], | ||
| "moe_align_block_size_deterministic": [ | ||
| "tests/test_moe_align_block_size.py::test_moe_align_block_size_deterministic", | ||
| ], | ||
| "batched_moe_align_block_size": [ | ||
| "tests/test_moe_align_block_size.py::test_batched_moe_align_block_size", | ||
| ], | ||
| "moe_align_block_size_opcheck": [ | ||
| "tests/test_moe_align_block_size.py::test_moe_align_block_size_opcheck", | ||
| ], | ||
| "batched_moe_align_block_size_opcheck": [ | ||
| "tests/test_moe_align_block_size.py::test_batched_moe_align_block_size_opcheck", |
There was a problem hiding this comment.
Several pytest node-id strings here exceed the configured 80-char line length (ruff E501). Since these are string literals, auto-formatting won’t wrap them, so CI will likely fail. Split long node ids across adjacent string literals inside parentheses or add targeted # noqa: E501 comments.
| "tests/test_moe_align_block_size.py::test_moe_align_block_size_with_expert_map", | |
| ], | |
| "moe_align_block_size_deterministic": [ | |
| "tests/test_moe_align_block_size.py::test_moe_align_block_size_deterministic", | |
| ], | |
| "batched_moe_align_block_size": [ | |
| "tests/test_moe_align_block_size.py::test_batched_moe_align_block_size", | |
| ], | |
| "moe_align_block_size_opcheck": [ | |
| "tests/test_moe_align_block_size.py::test_moe_align_block_size_opcheck", | |
| ], | |
| "batched_moe_align_block_size_opcheck": [ | |
| "tests/test_moe_align_block_size.py::test_batched_moe_align_block_size_opcheck", | |
| "tests/test_moe_align_block_size.py::" | |
| "test_moe_align_block_size_with_expert_map", | |
| ], | |
| "moe_align_block_size_deterministic": [ | |
| "tests/test_moe_align_block_size.py::" | |
| "test_moe_align_block_size_deterministic", | |
| ], | |
| "batched_moe_align_block_size": [ | |
| "tests/test_moe_align_block_size.py::" | |
| "test_batched_moe_align_block_size", | |
| ], | |
| "moe_align_block_size_opcheck": [ | |
| "tests/test_moe_align_block_size.py::" | |
| "test_moe_align_block_size_opcheck", | |
| ], | |
| "batched_moe_align_block_size_opcheck": [ | |
| "tests/test_moe_align_block_size.py::" | |
| "test_batched_moe_align_block_size_opcheck", |
| "fused_moe_remap_hidden_states": [ | ||
| "tests/fused_moe/test_remap_hidden_states.py::test_remap_hidden_states", | ||
| ], | ||
| "fused_moe_init_expert_map": [ | ||
| "tests/fused_moe/test_remap_hidden_states.py::test_init_expert_map", | ||
| ], |
There was a problem hiding this comment.
Several pytest node-id strings in this block are longer than 80 characters (ruff E501), which will fail linting. Consider splitting the strings across lines using implicit concatenation or adding # noqa: E501 per line.
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS ABOVE HAVE BEEN CONSIDERED.
Purpose
python3 tests/suites/resolve_model_suite.py --models llama3-mxfp8,llama4-bf16
output:
Test Plan
Test Result
(Optional) Documentation Update
BEFORE SUBMITTING, PLEASE READ https://docs.vllm.ai/en/latest/contributing (anything written below this line will be removed by GitHub Actions)