[Bugfix][Triton] Set the _get_config memo only after the config JSON loads - #4447
Closed
Ragua1 wants to merge 1 commit into
Closed
[Bugfix][Triton] Set the _get_config memo only after the config JSON loads#4447Ragua1 wants to merge 1 commit into
Ragua1 wants to merge 1 commit into
Conversation
…loads The memo attribute was created before the config file was opened, so a failed load left it behind, empty. functools.lru_cache does not cache exceptions, so the next call re-entered the function, found the attribute present, skipped the load and raised a KeyError on an internal key name instead of naming the missing file. In mla_decode_rope it raised nothing at all and returned an empty config. Five of the seven sites already assign the whole dict after the load, so the earlier assignment to an empty dict was dead code on the success path; those lines are removed. mha.py and gluon/gemm_a8w8_blockscale.py build into the dict, so the two statements fold into one. Adds op_tests/triton_tests/test_config_load_failure.py, which points the configs path at a temporary directory and asserts that two consecutive calls raise the same FileNotFoundError. It needs no tuned config and no particular GPU. Signed-off-by: Martin Domanský <ragua@email.cz>
Contributor
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
|
Contributor
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.
Motivation
When a Triton op has no config JSON for the running architecture, the first call reports the real error and every call after it reports a misleading one.
The second message is the one users see and report, because retries, later layers and test suites rarely stop at the first exception. It names an internal dict key instead of the missing file, so it sends people looking in the wrong place.
This is not specific to one architecture or to a missing file: it happens on any arch, for any op whose config file cannot be read or parsed.
Technical Details
Seven
_get_confighelpers memoize their parsed config on the function object and guard the load withhasattr:functools.lru_cachedoes not cache exceptions, so the next call re-enters the body — buthasattris nowTrue, the load block is skipped, and the caller reads an empty dict.Affected sites and what the second call reports today:
aiter/ops/triton/_triton_kernels/attention/mha.pyKeyError: 'default'aiter/ops/triton/_triton_kernels/attention/mha_fused_bwd.pyKeyError: 'bkwd_fused'aiter/ops/triton/_triton_kernels/attention/mha_onekernel_bwd.pyKeyError: 'bkwd_onekernel'aiter/ops/triton/_triton_kernels/attention/extend_attention.pyKeyError: 'default'aiter/ops/triton/_triton_kernels/attention/mla_decode_rope.py{}aiter/ops/triton/_triton_kernels/moe/moe_routing_sigmoid_top1_fused.pyKeyError: 'N16'aiter/ops/triton/gluon/gemm_a8w8_blockscale.pyKeyError: 'default'mla_decode_ropeis the one worth a second look: it hands an empty config back to the caller without raising, so the failure surfaces later as a missing kernel parameter.The fix publishes the memo only once there is something to publish. Two shapes,
+2/-9in total:_get_config._config_dict = config), which means the earlier= {}is dead code on the success path — it is overwritten three lines later. Those lines are simply removed.mha.pyandgluon/gemm_a8w8_blockscale.pybuild into the dict, so the two statements fold into one:_get_config._config_dict = {"default": config}.No behaviour changes on any path where the config loads.
This is the same family as #2169 (merged), which stopped
get_gemm_configfrom leaking cached state into callers. That one was about a caller mutating the cached dict; this one is about an exception leaving a half-built memo behind.Test Plan
New test:
op_tests/triton_tests/test_config_load_failure.py, parametrised over the six sites whose failure path is arch-independent. For each one it pointsAITER_TRITON_CONFIGS_PATHat pytest'stmp_pathand asserts that two consecutive calls raise the sameFileNotFoundError.It needs no tuned config and no particular GPU — the load fails for whichever architecture is running, so it exercises the fix on CDNA as well. It also clears the memo in a
finallyblock so it cannot affect later tests in the same session.gluon/gemm_a8w8_blockscale.pyis fixed but not covered by the test: its load sits behind agfx < 950check, so whether the failure path is reachable depends on the running architecture, and a test whose outcome depends on that does not belong in the suite.Also run: a standalone reproducer that calls each of the seven
_get_confighelpers twice in a fresh subprocess, with an option to override the reported architecture so the missing-config condition can be produced on any GPU, including ones that ship every config.Test Result
Environment: Radeon RX 7800 XT (gfx1101), Windows,
torch 2.10.0+rocm7.14.0a20260611,triton 3.7.1,AITER_TRITON_ONLY(implicit on win32). CDNA hardware was not available to me.New test, with the fix:
Same test with the fix reverted — 5 sites raise the wrong error, 1 raises none:
Reproducer, all seven sites, before and after.
--archoverrides the reported architecture; thegfx942run is the control, since those config files do exist:End to end through the public API,
flash_attn_funccalled twice in one process:Style checks on the changed files:
black --checkclean (and clean repo-wide, 972 files, with black 26.5.1);ruff checkwith the CI-pinned 0.16.0 reportsAll checks passed!.I could not run the existing MHA op_tests on this platform:
op_tests/triton_tests/attention/test_mha.pyimportsaiter.test_mha_common, which doesfrom aiter import dtypes, and that attribute is not bound on theAITER_TRITON_ONLYpath. Supplying it from outside then fails on the missingaiter.jit.module_aiter_coreprebuilt, which sends architecture detection torocminfo. That is why the new test is written not to depend on those helpers.Submission Checklist