Repository navigation
update(ci): make the kernel dtype-key checker's scope fail-closed - #2055
Merged
Merged
Conversation
The checker globbed `*.cpp` in two directories and skipped any file without `cuda_kernel(`, so a launch moved into a header, a shared helper or a HIP-only file left the check printing OK over a shrunk scope. It already had: the #1804 ROCm fault probe in `mlx_cxx_bridge.cpp` calls `fast::hip_kernel` and was never checked. The checker now scans every C/C++/CUDA source and header in the repository, counts a file as a launcher when it calls `cuda_kernel(` or `hip_kernel(` outside a comment (the vendored entry-point definitions excluded), reports the in-scope count next to the scanned count, and pins the in-scope set in `EXPECTED_IN_SCOPE`. A pinned file that disappears or stops launching fails with a message saying which, as do an unpinned new launcher and an empty scope. HIP is covered because ROCm's `custom_kernel.cpp` builds the same `name + template_arguments_hash` name and `rocm::get_jit_module` memoises by device index and that name only. `check_kernel_dtype_keys_test.sh` mutates throwaway copies (launch into a header, into a helper, commented out, file deleted, HIP-only launchers, empty tree, plus positive controls) and runs from `make verify-kernel-dtype-keys` and the CI job. Against the previous checker 10 of its 13 cases fail. Closes #1875
Review of the first commit found that `make verify` would break on macOS (the companion test used GNU `sed -i`), that a gitignored `references/mlx` checkout made local runs fail on MLX's own launchers, that `.hip` translation units were not scanned, and that pruning `build`/`site` at any depth could hide a real launcher. In a git work tree the checker now takes its file list from `git ls-files --cached --others --exclude-standard`; other trees (the test's copies) are walked with only top-level build and hidden directories skipped. `.hip` is scanned, and a digit separator such as `1'000` no longer opens a character literal that could hide a launch. The test mutates files through a Python helper that fails when its pattern matches nothing, and gains cases for a `.hip` file in a nested `build/` directory and for ignored versus untracked launchers in a git tree. The docs and docstring now state the two remaining limits (inline `template_args`, launches without a direct call). Refs #1875
inureyes
added
type:enhancement
33 of 37 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1875
check_kernel_dtype_keys.pyscoped itself by globbing*.cppin two directories and skipping files withoutcuda_kernel(, so a launch moved into a header, a helper or a HIP-only file shrank the scope while the check still printed OK. That had already happened once: the #1804 ROCm fault probe inmlx_cxx_bridge.cppcallsfast::hip_kerneland was never in scope.What changes
git ls-files --cached --others --exclude-standard), so gitignoredreferences/checkouts with MLX's own launchers stay out. Non-git trees are walked with only top-level build and hidden directories skipped.cuda_kernel(orhip_kernel(outside a comment. Raw strings and digit separators are lexed correctly, and the vendoredCustomKernelFunction hip_kernel(definitions do not count.custom_kernel.cppbuilds"custom_kernel_" + name + template_arguments_hash(template_args), androcm::get_jit_modulememoises in a process-global map keyed only by device index and that name. The fix(paged): paged_v2 sparse f16 cache decode disagrees completely with its reference #1053 rule therefore applies to HIP unchanged.9 in scope ... of 188 source files scanned. The in-scope set is pinned inEXPECTED_IN_SCOPE, and the check fails on any of these: a pinned file that was deleted, a pinned file that stopped launching (each with its own message), an unpinned new launcher, or an empty scope.check_kernel_dtype_keys_test.shmutates throwaway copies across 16 cases and runs frommake verify-kernel-dtype-keys(somake verifyandmake verify-rocmrun it) and from a new step in thekernel dtype keysCI job. All 11 negative cases fail when run against the previous checker.docs/code-guidelines.md, including the two remaining limits: inlinetemplate_argsand launches without a direct call. InCONTRIBUTING.md, themake verifyprerequisite list was stale; it now includesverify-kernel-port-dispatch.Changes during review
A review pass found that the test used GNU-only
sed -i(it would have brokenmake verifyon macOS), that the checker did not scan.hipfiles, and that it pruned directories by name at any depth. It also found that ignoredreferences/checkouts failed the local run. Commit e3d0150 fixes all four.Verification
On the gfx1151 host:
make verify-kernel-dtype-keys verify-kernel-port-dispatch verify-versions verify-llama-compat verify-fmtpasses.cargo test --features rocm --test dead_doc_pointerspasses (2 tests). The checker also passes on the primary checkout (188 files scanned, 9 in scope).Not verified: Metal and CUDA. Neither is available on this host. The change is CI-script and docs only and touches no Metal or CUDA code path. The test script was made BSD-sed-safe but was not run on macOS.