[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen - #20728

Merged
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head
Jul 9, 2026
Merged

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen#20728
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Additive coverage for the template engine added in the previous diff.

Key additions (test/test_wgsl_codegen.py, WgslTemplateEngineTest):

  • preprocess$if/$else branch selection + ${...} substitution + guarded-body indentation.
  • the DTYPE x VEC variant matrix and SUFFIX rules ("" suppresses; defaults to str(VALUE)).
  • parse_template_spec — minimal-spec expansion, duplicate-key rejection (via the UniqueKeyLoader), top-level-key validation.
  • the 3 WGSL type-helpers (buffer_scalar_type / buffer_gvec_type / accum_scalar_type).
  • a byte-identity round-trip reproducing both committed rms_norm headers from the template + rms_norm.yaml.
  • a source-of-truth sync-lock asserting the shared $-block transpiler helpers and the UniqueKeyLoader stay character-identical to gen_vulkan_spv.py (guards against silent drift).
    @exported-using-ghexport

Differential Revision: D110660006

Differential Revision: D110660006

[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Jul 5, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20728

Note: Links to docs will display an error until the docs builds have been completed.

❌ 5 New Failures, 2 Pending, 1 Unrelated Failure

As of commit 090cdc3 with merge base f4b01a8 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla

linux-foundation-easyclaBot commented Jul 5, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@JCNTH

JCNTH commented Jul 5, 2026

Copy link
Copy Markdown
ContributorAuthor

@claude throoughly review, check for overengineering, correctness, and areaas for modularization

@claude

claudeBot commented Jul 5, 2026

Copy link
Copy Markdown

Claude finished @JCNTH's task in 2m 40s —— View job


Review: WGSL shader-variant codegen unit tests

  • Read the test file
  • Read the codegen source (gen_wgsl_headers.py) under test
  • Cross-check the sync-lock against gen_vulkan_spv.py
  • Assess correctness, overengineering, modularization

Solid, well-targeted test suite. The names are descriptive, the docstrings capture the why (the DD-009 enable f16 note and the raw-string-terminator case are especially good), and every assertion I traced lines up with the actual API in gen_wgsl_headers.py. I verified the three shared helpers are byte-identical between the two files, and all files the tests reference (rms_norm.wgsl/.json/.h) exist. Verdict: approve with minor suggestions — nothing blocking.

Note: I reviewed by inspection + cross-file diffing; the sandbox blocked me from actually running pytest, so I did not get a green run locally.


Correctness

  • _function_source (lines 36–61) is the one piece with real fragility. Its body-end heuristic — "stop at the next non-blank column-0 line" — is correct for the current layout (escape is followed by a # comment, preprocess by class SPVGenerator:, both at column 0), but it will silently truncate if any helper ever gains a line that starts at column 0 inside the body (e.g. a multi-line string, or a ) closing a call at column 0). For a test whose whole purpose is guarding against drift, the parser guarding it shouldn't itself be position-sensitive. See the modularization note for a sturdier approach.

  • The two-phase signature skip (lines 50–53) correctly handles preprocess's multi-line signature whose ) -> str: sits at column 0 — good catch by the author. Worth noting the loop assumes the first line ending in : is the signature terminator; a default value like x: Dict = {} ending a wrapped signature line would fool it. Not a problem for these three functions, but it's the same class of latent fragility.

Overengineering

  • Reasonable overall — this isn't gold-plated. The single candidate is the sync-lock's hand-rolled text parser: ~26 lines of column heuristics to enforce char-identity of 3 functions. Since gen_wgsl_headers.pyalready imports these functions, the webgpu side can just use inspect.getsource(getattr(g, fn)) and only the (un-importable) Vulkan side needs source extraction — and that extraction is more robustly done with ast than regex/column-counting. That removes the fragility and shrinks the helper.

  • Mild redundancy: test_rms_norm_template_roundtrip_byte_identical (416) overlaps test_committed_headers_match_generator (134) — the committed rms_norm headers are already validated through headers_for_shader, which runs the same template expansion. The round-trip test earns its keep by exercising preprocess + render_headerdirectly (bypassing spec parsing) and pinning the concrete variant names, so I'd keep it — just calling out the overlap.

Modularization

  • Suggested: extract via ast instead of column heuristics.ast.parse(text) + walk for FunctionDef with the target name + ast.get_source_segment(text, node) gives the exact source span with no indentation assumptions, and works identically for both files. This replaces _function_source entirely and is pure stdlib (consistent with the generator's stdlib-only constraint). Fix this →

  • The two test classes (WgslCodegenTest / WgslTemplateEngineTest) and the module-level loader are cleanly separated. _write_spec is an appropriate local helper. No changes needed here.

Coverage gaps (optional, not blocking)

generate_variant_combinations/parse_template_spec are inherited from the Vulkan engine and have branches the tests don't reach. If webgpu will exercise them, worth a line each:

  • The int/uint rejection (gen_wgsl_headers.py:240-243) — a one-line assertRaises(ValueError) test.
  • The RANGE expansion (lines 226-236), including the _1/_2 suffix naming — currently only VALUE is tested.
  • The invalid variant-key assert in parse_template_spec (line 292) — the top-level-key mismatch is covered, but not an unknown key inside a shader_variants entry.
  • The combination/combinationN branch (lines 205-221) is untested. Likely out of scope for webgpu today; a log-style comment noting it's intentionally uncovered would prevent a false sense of completeness.

Nit

  • test_render_header_shape:111 asserts assertNotIn("kUpdateCacheWorkgroupSize ", h) (trailing space) to reject an untyped symbol — clever, but a touch cryptic; a short comment (# no untyped WorkgroupSize symbol, only X/Y/Z) would help the next reader.

None of the above blocks the PR — the suggestions are hardening the drift-guard and closing a few inherited-branch gaps. Nice work on the byte-identity and DD-009 regression cases.
· branch gh/JCNTH/2/head

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]

@SS-JIASS-JIA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@meta-codesync
meta-codesyncBot merged commit 706bce5 into gh/JCNTH/2/baseJul 9, 2026
175 of 183 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/2/head branch July 9, 2026 21:39
JCNTH added a commit that referenced this pull request Jul 9, 2026
Pull Request resolved: #20728
Additive coverage for the template engine added in the previous diff.
Key additions (`test/test_wgsl_codegen.py`, `WgslTemplateEngineTest`):
- `preprocess` `$if`/`$else` branch selection + `${...}` substitution + guarded-body indentation.
- the DTYPE x VEC variant matrix and `SUFFIX` rules (`""` suppresses; defaults to `str(VALUE)`).
- `parse_template_spec` — minimal-spec expansion, duplicate-key rejection (via the `UniqueKeyLoader`), top-level-key validation.
- the 3 WGSL type-helpers (`buffer_scalar_type` / `buffer_gvec_type` / `accum_scalar_type`).
- a byte-identity round-trip reproducing both committed `rms_norm` headers from the template + `rms_norm.yaml`.
- a source-of-truth sync-lock asserting the shared `$`-block transpiler helpers and the `UniqueKeyLoader` stay character-identical to `gen_vulkan_spv.py` (guards against silent drift).
ghstack-source-id: 401515160
@exported-using-ghexport
Differential Revision: [D110660006](https://our.internmc.facebook.com/intern/diff/D110660006/)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@JCNTH@SS-JIA
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen - #20728

Merged
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head
Jul 9, 2026
Merged

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen#20728
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Additive coverage for the template engine added in the previous diff.

Key additions (test/test_wgsl_codegen.py, WgslTemplateEngineTest):

  • preprocess$if/$else branch selection + ${...} substitution + guarded-body indentation.
  • the DTYPE x VEC variant matrix and SUFFIX rules ("" suppresses; defaults to str(VALUE)).
  • parse_template_spec — minimal-spec expansion, duplicate-key rejection (via the UniqueKeyLoader), top-level-key validation.
  • the 3 WGSL type-helpers (buffer_scalar_type / buffer_gvec_type / accum_scalar_type).
  • a byte-identity round-trip reproducing both committed rms_norm headers from the template + rms_norm.yaml.
  • a source-of-truth sync-lock asserting the shared $-block transpiler helpers and the UniqueKeyLoader stay character-identical to gen_vulkan_spv.py (guards against silent drift).
    @exported-using-ghexport

Differential Revision: D110660006

Differential Revision: D110660006

[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Jul 5, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20728

Note: Links to docs will display an error until the docs builds have been completed.

❌ 5 New Failures, 2 Pending, 1 Unrelated Failure

As of commit 090cdc3 with merge base f4b01a8 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla

linux-foundation-easyclaBot commented Jul 5, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@JCNTH

JCNTH commented Jul 5, 2026

Copy link
Copy Markdown
ContributorAuthor

@claude throoughly review, check for overengineering, correctness, and areaas for modularization

@claude

claudeBot commented Jul 5, 2026

Copy link
Copy Markdown

Claude finished @JCNTH's task in 2m 40s —— View job


Review: WGSL shader-variant codegen unit tests

  • Read the test file
  • Read the codegen source (gen_wgsl_headers.py) under test
  • Cross-check the sync-lock against gen_vulkan_spv.py
  • Assess correctness, overengineering, modularization

Solid, well-targeted test suite. The names are descriptive, the docstrings capture the why (the DD-009 enable f16 note and the raw-string-terminator case are especially good), and every assertion I traced lines up with the actual API in gen_wgsl_headers.py. I verified the three shared helpers are byte-identical between the two files, and all files the tests reference (rms_norm.wgsl/.json/.h) exist. Verdict: approve with minor suggestions — nothing blocking.

Note: I reviewed by inspection + cross-file diffing; the sandbox blocked me from actually running pytest, so I did not get a green run locally.


Correctness

  • _function_source (lines 36–61) is the one piece with real fragility. Its body-end heuristic — "stop at the next non-blank column-0 line" — is correct for the current layout (escape is followed by a # comment, preprocess by class SPVGenerator:, both at column 0), but it will silently truncate if any helper ever gains a line that starts at column 0 inside the body (e.g. a multi-line string, or a ) closing a call at column 0). For a test whose whole purpose is guarding against drift, the parser guarding it shouldn't itself be position-sensitive. See the modularization note for a sturdier approach.

  • The two-phase signature skip (lines 50–53) correctly handles preprocess's multi-line signature whose ) -> str: sits at column 0 — good catch by the author. Worth noting the loop assumes the first line ending in : is the signature terminator; a default value like x: Dict = {} ending a wrapped signature line would fool it. Not a problem for these three functions, but it's the same class of latent fragility.

Overengineering

  • Reasonable overall — this isn't gold-plated. The single candidate is the sync-lock's hand-rolled text parser: ~26 lines of column heuristics to enforce char-identity of 3 functions. Since gen_wgsl_headers.pyalready imports these functions, the webgpu side can just use inspect.getsource(getattr(g, fn)) and only the (un-importable) Vulkan side needs source extraction — and that extraction is more robustly done with ast than regex/column-counting. That removes the fragility and shrinks the helper.

  • Mild redundancy: test_rms_norm_template_roundtrip_byte_identical (416) overlaps test_committed_headers_match_generator (134) — the committed rms_norm headers are already validated through headers_for_shader, which runs the same template expansion. The round-trip test earns its keep by exercising preprocess + render_headerdirectly (bypassing spec parsing) and pinning the concrete variant names, so I'd keep it — just calling out the overlap.

Modularization

  • Suggested: extract via ast instead of column heuristics.ast.parse(text) + walk for FunctionDef with the target name + ast.get_source_segment(text, node) gives the exact source span with no indentation assumptions, and works identically for both files. This replaces _function_source entirely and is pure stdlib (consistent with the generator's stdlib-only constraint). Fix this →

  • The two test classes (WgslCodegenTest / WgslTemplateEngineTest) and the module-level loader are cleanly separated. _write_spec is an appropriate local helper. No changes needed here.

Coverage gaps (optional, not blocking)

generate_variant_combinations/parse_template_spec are inherited from the Vulkan engine and have branches the tests don't reach. If webgpu will exercise them, worth a line each:

  • The int/uint rejection (gen_wgsl_headers.py:240-243) — a one-line assertRaises(ValueError) test.
  • The RANGE expansion (lines 226-236), including the _1/_2 suffix naming — currently only VALUE is tested.
  • The invalid variant-key assert in parse_template_spec (line 292) — the top-level-key mismatch is covered, but not an unknown key inside a shader_variants entry.
  • The combination/combinationN branch (lines 205-221) is untested. Likely out of scope for webgpu today; a log-style comment noting it's intentionally uncovered would prevent a false sense of completeness.

Nit

  • test_render_header_shape:111 asserts assertNotIn("kUpdateCacheWorkgroupSize ", h) (trailing space) to reject an untyped symbol — clever, but a touch cryptic; a short comment (# no untyped WorkgroupSize symbol, only X/Y/Z) would help the next reader.

None of the above blocks the PR — the suggestions are hardening the drift-guard and closing a few inherited-branch gaps. Nice work on the byte-identity and DD-009 regression cases.
· branch gh/JCNTH/2/head

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]

@SS-JIASS-JIA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@meta-codesync
meta-codesyncBot merged commit 706bce5 into gh/JCNTH/2/baseJul 9, 2026
175 of 183 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/2/head branch July 9, 2026 21:39
JCNTH added a commit that referenced this pull request Jul 9, 2026
Pull Request resolved: #20728
Additive coverage for the template engine added in the previous diff.
Key additions (`test/test_wgsl_codegen.py`, `WgslTemplateEngineTest`):
- `preprocess` `$if`/`$else` branch selection + `${...}` substitution + guarded-body indentation.
- the DTYPE x VEC variant matrix and `SUFFIX` rules (`""` suppresses; defaults to `str(VALUE)`).
- `parse_template_spec` — minimal-spec expansion, duplicate-key rejection (via the `UniqueKeyLoader`), top-level-key validation.
- the 3 WGSL type-helpers (`buffer_scalar_type` / `buffer_gvec_type` / `accum_scalar_type`).
- a byte-identity round-trip reproducing both committed `rms_norm` headers from the template + `rms_norm.yaml`.
- a source-of-truth sync-lock asserting the shared `$`-block transpiler helpers and the `UniqueKeyLoader` stay character-identical to `gen_vulkan_spv.py` (guards against silent drift).
ghstack-source-id: 401515160
@exported-using-ghexport
Differential Revision: [D110660006](https://our.internmc.facebook.com/intern/diff/D110660006/)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@JCNTH@SS-JIA
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen - #20728

Merged
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head
Jul 9, 2026
Merged

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen#20728
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Additive coverage for the template engine added in the previous diff.

Key additions (test/test_wgsl_codegen.py, WgslTemplateEngineTest):

  • preprocess$if/$else branch selection + ${...} substitution + guarded-body indentation.
  • the DTYPE x VEC variant matrix and SUFFIX rules ("" suppresses; defaults to str(VALUE)).
  • parse_template_spec — minimal-spec expansion, duplicate-key rejection (via the UniqueKeyLoader), top-level-key validation.
  • the 3 WGSL type-helpers (buffer_scalar_type / buffer_gvec_type / accum_scalar_type).
  • a byte-identity round-trip reproducing both committed rms_norm headers from the template + rms_norm.yaml.
  • a source-of-truth sync-lock asserting the shared $-block transpiler helpers and the UniqueKeyLoader stay character-identical to gen_vulkan_spv.py (guards against silent drift).
    @exported-using-ghexport

Differential Revision: D110660006

Differential Revision: D110660006

[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Jul 5, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20728

Note: Links to docs will display an error until the docs builds have been completed.

❌ 5 New Failures, 2 Pending, 1 Unrelated Failure

As of commit 090cdc3 with merge base f4b01a8 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla

linux-foundation-easyclaBot commented Jul 5, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@JCNTH

JCNTH commented Jul 5, 2026

Copy link
Copy Markdown
ContributorAuthor

@claude throoughly review, check for overengineering, correctness, and areaas for modularization

@claude

claudeBot commented Jul 5, 2026

Copy link
Copy Markdown

Claude finished @JCNTH's task in 2m 40s —— View job


Review: WGSL shader-variant codegen unit tests

  • Read the test file
  • Read the codegen source (gen_wgsl_headers.py) under test
  • Cross-check the sync-lock against gen_vulkan_spv.py
  • Assess correctness, overengineering, modularization

Solid, well-targeted test suite. The names are descriptive, the docstrings capture the why (the DD-009 enable f16 note and the raw-string-terminator case are especially good), and every assertion I traced lines up with the actual API in gen_wgsl_headers.py. I verified the three shared helpers are byte-identical between the two files, and all files the tests reference (rms_norm.wgsl/.json/.h) exist. Verdict: approve with minor suggestions — nothing blocking.

Note: I reviewed by inspection + cross-file diffing; the sandbox blocked me from actually running pytest, so I did not get a green run locally.


Correctness

  • _function_source (lines 36–61) is the one piece with real fragility. Its body-end heuristic — "stop at the next non-blank column-0 line" — is correct for the current layout (escape is followed by a # comment, preprocess by class SPVGenerator:, both at column 0), but it will silently truncate if any helper ever gains a line that starts at column 0 inside the body (e.g. a multi-line string, or a ) closing a call at column 0). For a test whose whole purpose is guarding against drift, the parser guarding it shouldn't itself be position-sensitive. See the modularization note for a sturdier approach.

  • The two-phase signature skip (lines 50–53) correctly handles preprocess's multi-line signature whose ) -> str: sits at column 0 — good catch by the author. Worth noting the loop assumes the first line ending in : is the signature terminator; a default value like x: Dict = {} ending a wrapped signature line would fool it. Not a problem for these three functions, but it's the same class of latent fragility.

Overengineering

  • Reasonable overall — this isn't gold-plated. The single candidate is the sync-lock's hand-rolled text parser: ~26 lines of column heuristics to enforce char-identity of 3 functions. Since gen_wgsl_headers.pyalready imports these functions, the webgpu side can just use inspect.getsource(getattr(g, fn)) and only the (un-importable) Vulkan side needs source extraction — and that extraction is more robustly done with ast than regex/column-counting. That removes the fragility and shrinks the helper.

  • Mild redundancy: test_rms_norm_template_roundtrip_byte_identical (416) overlaps test_committed_headers_match_generator (134) — the committed rms_norm headers are already validated through headers_for_shader, which runs the same template expansion. The round-trip test earns its keep by exercising preprocess + render_headerdirectly (bypassing spec parsing) and pinning the concrete variant names, so I'd keep it — just calling out the overlap.

Modularization

  • Suggested: extract via ast instead of column heuristics.ast.parse(text) + walk for FunctionDef with the target name + ast.get_source_segment(text, node) gives the exact source span with no indentation assumptions, and works identically for both files. This replaces _function_source entirely and is pure stdlib (consistent with the generator's stdlib-only constraint). Fix this →

  • The two test classes (WgslCodegenTest / WgslTemplateEngineTest) and the module-level loader are cleanly separated. _write_spec is an appropriate local helper. No changes needed here.

Coverage gaps (optional, not blocking)

generate_variant_combinations/parse_template_spec are inherited from the Vulkan engine and have branches the tests don't reach. If webgpu will exercise them, worth a line each:

  • The int/uint rejection (gen_wgsl_headers.py:240-243) — a one-line assertRaises(ValueError) test.
  • The RANGE expansion (lines 226-236), including the _1/_2 suffix naming — currently only VALUE is tested.
  • The invalid variant-key assert in parse_template_spec (line 292) — the top-level-key mismatch is covered, but not an unknown key inside a shader_variants entry.
  • The combination/combinationN branch (lines 205-221) is untested. Likely out of scope for webgpu today; a log-style comment noting it's intentionally uncovered would prevent a false sense of completeness.

Nit

  • test_render_header_shape:111 asserts assertNotIn("kUpdateCacheWorkgroupSize ", h) (trailing space) to reject an untyped symbol — clever, but a touch cryptic; a short comment (# no untyped WorkgroupSize symbol, only X/Y/Z) would help the next reader.

None of the above blocks the PR — the suggestions are hardening the drift-guard and closing a few inherited-branch gaps. Nice work on the byte-identity and DD-009 regression cases.
· branch gh/JCNTH/2/head

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]

@SS-JIASS-JIA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@meta-codesync
meta-codesyncBot merged commit 706bce5 into gh/JCNTH/2/baseJul 9, 2026
175 of 183 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/2/head branch July 9, 2026 21:39
JCNTH added a commit that referenced this pull request Jul 9, 2026
Pull Request resolved: #20728
Additive coverage for the template engine added in the previous diff.
Key additions (`test/test_wgsl_codegen.py`, `WgslTemplateEngineTest`):
- `preprocess` `$if`/`$else` branch selection + `${...}` substitution + guarded-body indentation.
- the DTYPE x VEC variant matrix and `SUFFIX` rules (`""` suppresses; defaults to `str(VALUE)`).
- `parse_template_spec` — minimal-spec expansion, duplicate-key rejection (via the `UniqueKeyLoader`), top-level-key validation.
- the 3 WGSL type-helpers (`buffer_scalar_type` / `buffer_gvec_type` / `accum_scalar_type`).
- a byte-identity round-trip reproducing both committed `rms_norm` headers from the template + `rms_norm.yaml`.
- a source-of-truth sync-lock asserting the shared `$`-block transpiler helpers and the `UniqueKeyLoader` stay character-identical to `gen_vulkan_spv.py` (guards against silent drift).
ghstack-source-id: 401515160
@exported-using-ghexport
Differential Revision: [D110660006](https://our.internmc.facebook.com/intern/diff/D110660006/)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@JCNTH@SS-JIA
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen - #20728

Merged
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head
Jul 9, 2026
Merged

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen#20728
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Additive coverage for the template engine added in the previous diff.

Key additions (test/test_wgsl_codegen.py, WgslTemplateEngineTest):

  • preprocess$if/$else branch selection + ${...} substitution + guarded-body indentation.
  • the DTYPE x VEC variant matrix and SUFFIX rules ("" suppresses; defaults to str(VALUE)).
  • parse_template_spec — minimal-spec expansion, duplicate-key rejection (via the UniqueKeyLoader), top-level-key validation.
  • the 3 WGSL type-helpers (buffer_scalar_type / buffer_gvec_type / accum_scalar_type).
  • a byte-identity round-trip reproducing both committed rms_norm headers from the template + rms_norm.yaml.
  • a source-of-truth sync-lock asserting the shared $-block transpiler helpers and the UniqueKeyLoader stay character-identical to gen_vulkan_spv.py (guards against silent drift).
    @exported-using-ghexport

Differential Revision: D110660006

Differential Revision: D110660006

[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Jul 5, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20728

Note: Links to docs will display an error until the docs builds have been completed.

❌ 5 New Failures, 2 Pending, 1 Unrelated Failure

As of commit 090cdc3 with merge base f4b01a8 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla

linux-foundation-easyclaBot commented Jul 5, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@JCNTH

JCNTH commented Jul 5, 2026

Copy link
Copy Markdown
ContributorAuthor

@claude throoughly review, check for overengineering, correctness, and areaas for modularization

@claude

claudeBot commented Jul 5, 2026

Copy link
Copy Markdown

Claude finished @JCNTH's task in 2m 40s —— View job


Review: WGSL shader-variant codegen unit tests

  • Read the test file
  • Read the codegen source (gen_wgsl_headers.py) under test
  • Cross-check the sync-lock against gen_vulkan_spv.py
  • Assess correctness, overengineering, modularization

Solid, well-targeted test suite. The names are descriptive, the docstrings capture the why (the DD-009 enable f16 note and the raw-string-terminator case are especially good), and every assertion I traced lines up with the actual API in gen_wgsl_headers.py. I verified the three shared helpers are byte-identical between the two files, and all files the tests reference (rms_norm.wgsl/.json/.h) exist. Verdict: approve with minor suggestions — nothing blocking.

Note: I reviewed by inspection + cross-file diffing; the sandbox blocked me from actually running pytest, so I did not get a green run locally.


Correctness

  • _function_source (lines 36–61) is the one piece with real fragility. Its body-end heuristic — "stop at the next non-blank column-0 line" — is correct for the current layout (escape is followed by a # comment, preprocess by class SPVGenerator:, both at column 0), but it will silently truncate if any helper ever gains a line that starts at column 0 inside the body (e.g. a multi-line string, or a ) closing a call at column 0). For a test whose whole purpose is guarding against drift, the parser guarding it shouldn't itself be position-sensitive. See the modularization note for a sturdier approach.

  • The two-phase signature skip (lines 50–53) correctly handles preprocess's multi-line signature whose ) -> str: sits at column 0 — good catch by the author. Worth noting the loop assumes the first line ending in : is the signature terminator; a default value like x: Dict = {} ending a wrapped signature line would fool it. Not a problem for these three functions, but it's the same class of latent fragility.

Overengineering

  • Reasonable overall — this isn't gold-plated. The single candidate is the sync-lock's hand-rolled text parser: ~26 lines of column heuristics to enforce char-identity of 3 functions. Since gen_wgsl_headers.pyalready imports these functions, the webgpu side can just use inspect.getsource(getattr(g, fn)) and only the (un-importable) Vulkan side needs source extraction — and that extraction is more robustly done with ast than regex/column-counting. That removes the fragility and shrinks the helper.

  • Mild redundancy: test_rms_norm_template_roundtrip_byte_identical (416) overlaps test_committed_headers_match_generator (134) — the committed rms_norm headers are already validated through headers_for_shader, which runs the same template expansion. The round-trip test earns its keep by exercising preprocess + render_headerdirectly (bypassing spec parsing) and pinning the concrete variant names, so I'd keep it — just calling out the overlap.

Modularization

  • Suggested: extract via ast instead of column heuristics.ast.parse(text) + walk for FunctionDef with the target name + ast.get_source_segment(text, node) gives the exact source span with no indentation assumptions, and works identically for both files. This replaces _function_source entirely and is pure stdlib (consistent with the generator's stdlib-only constraint). Fix this →

  • The two test classes (WgslCodegenTest / WgslTemplateEngineTest) and the module-level loader are cleanly separated. _write_spec is an appropriate local helper. No changes needed here.

Coverage gaps (optional, not blocking)

generate_variant_combinations/parse_template_spec are inherited from the Vulkan engine and have branches the tests don't reach. If webgpu will exercise them, worth a line each:

  • The int/uint rejection (gen_wgsl_headers.py:240-243) — a one-line assertRaises(ValueError) test.
  • The RANGE expansion (lines 226-236), including the _1/_2 suffix naming — currently only VALUE is tested.
  • The invalid variant-key assert in parse_template_spec (line 292) — the top-level-key mismatch is covered, but not an unknown key inside a shader_variants entry.
  • The combination/combinationN branch (lines 205-221) is untested. Likely out of scope for webgpu today; a log-style comment noting it's intentionally uncovered would prevent a false sense of completeness.

Nit

  • test_render_header_shape:111 asserts assertNotIn("kUpdateCacheWorkgroupSize ", h) (trailing space) to reject an untyped symbol — clever, but a touch cryptic; a short comment (# no untyped WorkgroupSize symbol, only X/Y/Z) would help the next reader.

None of the above blocks the PR — the suggestions are hardening the drift-guard and closing a few inherited-branch gaps. Nice work on the byte-identity and DD-009 regression cases.
· branch gh/JCNTH/2/head

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]

@SS-JIASS-JIA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@meta-codesync
meta-codesyncBot merged commit 706bce5 into gh/JCNTH/2/baseJul 9, 2026
175 of 183 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/2/head branch July 9, 2026 21:39
JCNTH added a commit that referenced this pull request Jul 9, 2026
Pull Request resolved: #20728
Additive coverage for the template engine added in the previous diff.
Key additions (`test/test_wgsl_codegen.py`, `WgslTemplateEngineTest`):
- `preprocess` `$if`/`$else` branch selection + `${...}` substitution + guarded-body indentation.
- the DTYPE x VEC variant matrix and `SUFFIX` rules (`""` suppresses; defaults to `str(VALUE)`).
- `parse_template_spec` — minimal-spec expansion, duplicate-key rejection (via the `UniqueKeyLoader`), top-level-key validation.
- the 3 WGSL type-helpers (`buffer_scalar_type` / `buffer_gvec_type` / `accum_scalar_type`).
- a byte-identity round-trip reproducing both committed `rms_norm` headers from the template + `rms_norm.yaml`.
- a source-of-truth sync-lock asserting the shared `$`-block transpiler helpers and the `UniqueKeyLoader` stay character-identical to `gen_vulkan_spv.py` (guards against silent drift).
ghstack-source-id: 401515160
@exported-using-ghexport
Differential Revision: [D110660006](https://our.internmc.facebook.com/intern/diff/D110660006/)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@JCNTH@SS-JIA
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen - #20728

Merged
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head
Jul 9, 2026
Merged

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen#20728
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Additive coverage for the template engine added in the previous diff.

Key additions (test/test_wgsl_codegen.py, WgslTemplateEngineTest):

  • preprocess$if/$else branch selection + ${...} substitution + guarded-body indentation.
  • the DTYPE x VEC variant matrix and SUFFIX rules ("" suppresses; defaults to str(VALUE)).
  • parse_template_spec — minimal-spec expansion, duplicate-key rejection (via the UniqueKeyLoader), top-level-key validation.
  • the 3 WGSL type-helpers (buffer_scalar_type / buffer_gvec_type / accum_scalar_type).
  • a byte-identity round-trip reproducing both committed rms_norm headers from the template + rms_norm.yaml.
  • a source-of-truth sync-lock asserting the shared $-block transpiler helpers and the UniqueKeyLoader stay character-identical to gen_vulkan_spv.py (guards against silent drift).
    @exported-using-ghexport

Differential Revision: D110660006

Differential Revision: D110660006

[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Jul 5, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20728

Note: Links to docs will display an error until the docs builds have been completed.

❌ 5 New Failures, 2 Pending, 1 Unrelated Failure

As of commit 090cdc3 with merge base f4b01a8 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla

linux-foundation-easyclaBot commented Jul 5, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@JCNTH

JCNTH commented Jul 5, 2026

Copy link
Copy Markdown
ContributorAuthor

@claude throoughly review, check for overengineering, correctness, and areaas for modularization

@claude

claudeBot commented Jul 5, 2026

Copy link
Copy Markdown

Claude finished @JCNTH's task in 2m 40s —— View job


Review: WGSL shader-variant codegen unit tests

  • Read the test file
  • Read the codegen source (gen_wgsl_headers.py) under test
  • Cross-check the sync-lock against gen_vulkan_spv.py
  • Assess correctness, overengineering, modularization

Solid, well-targeted test suite. The names are descriptive, the docstrings capture the why (the DD-009 enable f16 note and the raw-string-terminator case are especially good), and every assertion I traced lines up with the actual API in gen_wgsl_headers.py. I verified the three shared helpers are byte-identical between the two files, and all files the tests reference (rms_norm.wgsl/.json/.h) exist. Verdict: approve with minor suggestions — nothing blocking.

Note: I reviewed by inspection + cross-file diffing; the sandbox blocked me from actually running pytest, so I did not get a green run locally.


Correctness

  • _function_source (lines 36–61) is the one piece with real fragility. Its body-end heuristic — "stop at the next non-blank column-0 line" — is correct for the current layout (escape is followed by a # comment, preprocess by class SPVGenerator:, both at column 0), but it will silently truncate if any helper ever gains a line that starts at column 0 inside the body (e.g. a multi-line string, or a ) closing a call at column 0). For a test whose whole purpose is guarding against drift, the parser guarding it shouldn't itself be position-sensitive. See the modularization note for a sturdier approach.

  • The two-phase signature skip (lines 50–53) correctly handles preprocess's multi-line signature whose ) -> str: sits at column 0 — good catch by the author. Worth noting the loop assumes the first line ending in : is the signature terminator; a default value like x: Dict = {} ending a wrapped signature line would fool it. Not a problem for these three functions, but it's the same class of latent fragility.

Overengineering

  • Reasonable overall — this isn't gold-plated. The single candidate is the sync-lock's hand-rolled text parser: ~26 lines of column heuristics to enforce char-identity of 3 functions. Since gen_wgsl_headers.pyalready imports these functions, the webgpu side can just use inspect.getsource(getattr(g, fn)) and only the (un-importable) Vulkan side needs source extraction — and that extraction is more robustly done with ast than regex/column-counting. That removes the fragility and shrinks the helper.

  • Mild redundancy: test_rms_norm_template_roundtrip_byte_identical (416) overlaps test_committed_headers_match_generator (134) — the committed rms_norm headers are already validated through headers_for_shader, which runs the same template expansion. The round-trip test earns its keep by exercising preprocess + render_headerdirectly (bypassing spec parsing) and pinning the concrete variant names, so I'd keep it — just calling out the overlap.

Modularization

  • Suggested: extract via ast instead of column heuristics.ast.parse(text) + walk for FunctionDef with the target name + ast.get_source_segment(text, node) gives the exact source span with no indentation assumptions, and works identically for both files. This replaces _function_source entirely and is pure stdlib (consistent with the generator's stdlib-only constraint). Fix this →

  • The two test classes (WgslCodegenTest / WgslTemplateEngineTest) and the module-level loader are cleanly separated. _write_spec is an appropriate local helper. No changes needed here.

Coverage gaps (optional, not blocking)

generate_variant_combinations/parse_template_spec are inherited from the Vulkan engine and have branches the tests don't reach. If webgpu will exercise them, worth a line each:

  • The int/uint rejection (gen_wgsl_headers.py:240-243) — a one-line assertRaises(ValueError) test.
  • The RANGE expansion (lines 226-236), including the _1/_2 suffix naming — currently only VALUE is tested.
  • The invalid variant-key assert in parse_template_spec (line 292) — the top-level-key mismatch is covered, but not an unknown key inside a shader_variants entry.
  • The combination/combinationN branch (lines 205-221) is untested. Likely out of scope for webgpu today; a log-style comment noting it's intentionally uncovered would prevent a false sense of completeness.

Nit

  • test_render_header_shape:111 asserts assertNotIn("kUpdateCacheWorkgroupSize ", h) (trailing space) to reject an untyped symbol — clever, but a touch cryptic; a short comment (# no untyped WorkgroupSize symbol, only X/Y/Z) would help the next reader.

None of the above blocks the PR — the suggestions are hardening the drift-guard and closing a few inherited-branch gaps. Nice work on the byte-identity and DD-009 regression cases.
· branch gh/JCNTH/2/head

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]

@SS-JIASS-JIA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@meta-codesync
meta-codesyncBot merged commit 706bce5 into gh/JCNTH/2/baseJul 9, 2026
175 of 183 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/2/head branch July 9, 2026 21:39
JCNTH added a commit that referenced this pull request Jul 9, 2026
Pull Request resolved: #20728
Additive coverage for the template engine added in the previous diff.
Key additions (`test/test_wgsl_codegen.py`, `WgslTemplateEngineTest`):
- `preprocess` `$if`/`$else` branch selection + `${...}` substitution + guarded-body indentation.
- the DTYPE x VEC variant matrix and `SUFFIX` rules (`""` suppresses; defaults to `str(VALUE)`).
- `parse_template_spec` — minimal-spec expansion, duplicate-key rejection (via the `UniqueKeyLoader`), top-level-key validation.
- the 3 WGSL type-helpers (`buffer_scalar_type` / `buffer_gvec_type` / `accum_scalar_type`).
- a byte-identity round-trip reproducing both committed `rms_norm` headers from the template + `rms_norm.yaml`.
- a source-of-truth sync-lock asserting the shared `$`-block transpiler helpers and the `UniqueKeyLoader` stay character-identical to `gen_vulkan_spv.py` (guards against silent drift).
ghstack-source-id: 401515160
@exported-using-ghexport
Differential Revision: [D110660006](https://our.internmc.facebook.com/intern/diff/D110660006/)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@JCNTH@SS-JIA
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen - #20728

Merged
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head
Jul 9, 2026
Merged

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen#20728
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Additive coverage for the template engine added in the previous diff.

Key additions (test/test_wgsl_codegen.py, WgslTemplateEngineTest):

  • preprocess$if/$else branch selection + ${...} substitution + guarded-body indentation.
  • the DTYPE x VEC variant matrix and SUFFIX rules ("" suppresses; defaults to str(VALUE)).
  • parse_template_spec — minimal-spec expansion, duplicate-key rejection (via the UniqueKeyLoader), top-level-key validation.
  • the 3 WGSL type-helpers (buffer_scalar_type / buffer_gvec_type / accum_scalar_type).
  • a byte-identity round-trip reproducing both committed rms_norm headers from the template + rms_norm.yaml.
  • a source-of-truth sync-lock asserting the shared $-block transpiler helpers and the UniqueKeyLoader stay character-identical to gen_vulkan_spv.py (guards against silent drift).
    @exported-using-ghexport

Differential Revision: D110660006

Differential Revision: D110660006

[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Jul 5, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20728

Note: Links to docs will display an error until the docs builds have been completed.

❌ 5 New Failures, 2 Pending, 1 Unrelated Failure

As of commit 090cdc3 with merge base f4b01a8 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla

linux-foundation-easyclaBot commented Jul 5, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@JCNTH

JCNTH commented Jul 5, 2026

Copy link
Copy Markdown
ContributorAuthor

@claude throoughly review, check for overengineering, correctness, and areaas for modularization

@claude

claudeBot commented Jul 5, 2026

Copy link
Copy Markdown

Claude finished @JCNTH's task in 2m 40s —— View job


Review: WGSL shader-variant codegen unit tests

  • Read the test file
  • Read the codegen source (gen_wgsl_headers.py) under test
  • Cross-check the sync-lock against gen_vulkan_spv.py
  • Assess correctness, overengineering, modularization

Solid, well-targeted test suite. The names are descriptive, the docstrings capture the why (the DD-009 enable f16 note and the raw-string-terminator case are especially good), and every assertion I traced lines up with the actual API in gen_wgsl_headers.py. I verified the three shared helpers are byte-identical between the two files, and all files the tests reference (rms_norm.wgsl/.json/.h) exist. Verdict: approve with minor suggestions — nothing blocking.

Note: I reviewed by inspection + cross-file diffing; the sandbox blocked me from actually running pytest, so I did not get a green run locally.


Correctness

  • _function_source (lines 36–61) is the one piece with real fragility. Its body-end heuristic — "stop at the next non-blank column-0 line" — is correct for the current layout (escape is followed by a # comment, preprocess by class SPVGenerator:, both at column 0), but it will silently truncate if any helper ever gains a line that starts at column 0 inside the body (e.g. a multi-line string, or a ) closing a call at column 0). For a test whose whole purpose is guarding against drift, the parser guarding it shouldn't itself be position-sensitive. See the modularization note for a sturdier approach.

  • The two-phase signature skip (lines 50–53) correctly handles preprocess's multi-line signature whose ) -> str: sits at column 0 — good catch by the author. Worth noting the loop assumes the first line ending in : is the signature terminator; a default value like x: Dict = {} ending a wrapped signature line would fool it. Not a problem for these three functions, but it's the same class of latent fragility.

Overengineering

  • Reasonable overall — this isn't gold-plated. The single candidate is the sync-lock's hand-rolled text parser: ~26 lines of column heuristics to enforce char-identity of 3 functions. Since gen_wgsl_headers.pyalready imports these functions, the webgpu side can just use inspect.getsource(getattr(g, fn)) and only the (un-importable) Vulkan side needs source extraction — and that extraction is more robustly done with ast than regex/column-counting. That removes the fragility and shrinks the helper.

  • Mild redundancy: test_rms_norm_template_roundtrip_byte_identical (416) overlaps test_committed_headers_match_generator (134) — the committed rms_norm headers are already validated through headers_for_shader, which runs the same template expansion. The round-trip test earns its keep by exercising preprocess + render_headerdirectly (bypassing spec parsing) and pinning the concrete variant names, so I'd keep it — just calling out the overlap.

Modularization

  • Suggested: extract via ast instead of column heuristics.ast.parse(text) + walk for FunctionDef with the target name + ast.get_source_segment(text, node) gives the exact source span with no indentation assumptions, and works identically for both files. This replaces _function_source entirely and is pure stdlib (consistent with the generator's stdlib-only constraint). Fix this →

  • The two test classes (WgslCodegenTest / WgslTemplateEngineTest) and the module-level loader are cleanly separated. _write_spec is an appropriate local helper. No changes needed here.

Coverage gaps (optional, not blocking)

generate_variant_combinations/parse_template_spec are inherited from the Vulkan engine and have branches the tests don't reach. If webgpu will exercise them, worth a line each:

  • The int/uint rejection (gen_wgsl_headers.py:240-243) — a one-line assertRaises(ValueError) test.
  • The RANGE expansion (lines 226-236), including the _1/_2 suffix naming — currently only VALUE is tested.
  • The invalid variant-key assert in parse_template_spec (line 292) — the top-level-key mismatch is covered, but not an unknown key inside a shader_variants entry.
  • The combination/combinationN branch (lines 205-221) is untested. Likely out of scope for webgpu today; a log-style comment noting it's intentionally uncovered would prevent a false sense of completeness.

Nit

  • test_render_header_shape:111 asserts assertNotIn("kUpdateCacheWorkgroupSize ", h) (trailing space) to reject an untyped symbol — clever, but a touch cryptic; a short comment (# no untyped WorkgroupSize symbol, only X/Y/Z) would help the next reader.

None of the above blocks the PR — the suggestions are hardening the drift-guard and closing a few inherited-branch gaps. Nice work on the byte-identity and DD-009 regression cases.
· branch gh/JCNTH/2/head

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]

@SS-JIASS-JIA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@meta-codesync
meta-codesyncBot merged commit 706bce5 into gh/JCNTH/2/baseJul 9, 2026
175 of 183 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/2/head branch July 9, 2026 21:39
JCNTH added a commit that referenced this pull request Jul 9, 2026
Pull Request resolved: #20728
Additive coverage for the template engine added in the previous diff.
Key additions (`test/test_wgsl_codegen.py`, `WgslTemplateEngineTest`):
- `preprocess` `$if`/`$else` branch selection + `${...}` substitution + guarded-body indentation.
- the DTYPE x VEC variant matrix and `SUFFIX` rules (`""` suppresses; defaults to `str(VALUE)`).
- `parse_template_spec` — minimal-spec expansion, duplicate-key rejection (via the `UniqueKeyLoader`), top-level-key validation.
- the 3 WGSL type-helpers (`buffer_scalar_type` / `buffer_gvec_type` / `accum_scalar_type`).
- a byte-identity round-trip reproducing both committed `rms_norm` headers from the template + `rms_norm.yaml`.
- a source-of-truth sync-lock asserting the shared `$`-block transpiler helpers and the `UniqueKeyLoader` stay character-identical to `gen_vulkan_spv.py` (guards against silent drift).
ghstack-source-id: 401515160
@exported-using-ghexport
Differential Revision: [D110660006](https://our.internmc.facebook.com/intern/diff/D110660006/)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@JCNTH@SS-JIA
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen - #20728

Merged
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head
Jul 9, 2026
Merged

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen#20728
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Additive coverage for the template engine added in the previous diff.

Key additions (test/test_wgsl_codegen.py, WgslTemplateEngineTest):

  • preprocess$if/$else branch selection + ${...} substitution + guarded-body indentation.
  • the DTYPE x VEC variant matrix and SUFFIX rules ("" suppresses; defaults to str(VALUE)).
  • parse_template_spec — minimal-spec expansion, duplicate-key rejection (via the UniqueKeyLoader), top-level-key validation.
  • the 3 WGSL type-helpers (buffer_scalar_type / buffer_gvec_type / accum_scalar_type).
  • a byte-identity round-trip reproducing both committed rms_norm headers from the template + rms_norm.yaml.
  • a source-of-truth sync-lock asserting the shared $-block transpiler helpers and the UniqueKeyLoader stay character-identical to gen_vulkan_spv.py (guards against silent drift).
    @exported-using-ghexport

Differential Revision: D110660006

Differential Revision: D110660006

[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Jul 5, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20728

Note: Links to docs will display an error until the docs builds have been completed.

❌ 5 New Failures, 2 Pending, 1 Unrelated Failure

As of commit 090cdc3 with merge base f4b01a8 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla

linux-foundation-easyclaBot commented Jul 5, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@JCNTH

JCNTH commented Jul 5, 2026

Copy link
Copy Markdown
ContributorAuthor

@claude throoughly review, check for overengineering, correctness, and areaas for modularization

@claude

claudeBot commented Jul 5, 2026

Copy link
Copy Markdown

Claude finished @JCNTH's task in 2m 40s —— View job


Review: WGSL shader-variant codegen unit tests

  • Read the test file
  • Read the codegen source (gen_wgsl_headers.py) under test
  • Cross-check the sync-lock against gen_vulkan_spv.py
  • Assess correctness, overengineering, modularization

Solid, well-targeted test suite. The names are descriptive, the docstrings capture the why (the DD-009 enable f16 note and the raw-string-terminator case are especially good), and every assertion I traced lines up with the actual API in gen_wgsl_headers.py. I verified the three shared helpers are byte-identical between the two files, and all files the tests reference (rms_norm.wgsl/.json/.h) exist. Verdict: approve with minor suggestions — nothing blocking.

Note: I reviewed by inspection + cross-file diffing; the sandbox blocked me from actually running pytest, so I did not get a green run locally.


Correctness

  • _function_source (lines 36–61) is the one piece with real fragility. Its body-end heuristic — "stop at the next non-blank column-0 line" — is correct for the current layout (escape is followed by a # comment, preprocess by class SPVGenerator:, both at column 0), but it will silently truncate if any helper ever gains a line that starts at column 0 inside the body (e.g. a multi-line string, or a ) closing a call at column 0). For a test whose whole purpose is guarding against drift, the parser guarding it shouldn't itself be position-sensitive. See the modularization note for a sturdier approach.

  • The two-phase signature skip (lines 50–53) correctly handles preprocess's multi-line signature whose ) -> str: sits at column 0 — good catch by the author. Worth noting the loop assumes the first line ending in : is the signature terminator; a default value like x: Dict = {} ending a wrapped signature line would fool it. Not a problem for these three functions, but it's the same class of latent fragility.

Overengineering

  • Reasonable overall — this isn't gold-plated. The single candidate is the sync-lock's hand-rolled text parser: ~26 lines of column heuristics to enforce char-identity of 3 functions. Since gen_wgsl_headers.pyalready imports these functions, the webgpu side can just use inspect.getsource(getattr(g, fn)) and only the (un-importable) Vulkan side needs source extraction — and that extraction is more robustly done with ast than regex/column-counting. That removes the fragility and shrinks the helper.

  • Mild redundancy: test_rms_norm_template_roundtrip_byte_identical (416) overlaps test_committed_headers_match_generator (134) — the committed rms_norm headers are already validated through headers_for_shader, which runs the same template expansion. The round-trip test earns its keep by exercising preprocess + render_headerdirectly (bypassing spec parsing) and pinning the concrete variant names, so I'd keep it — just calling out the overlap.

Modularization

  • Suggested: extract via ast instead of column heuristics.ast.parse(text) + walk for FunctionDef with the target name + ast.get_source_segment(text, node) gives the exact source span with no indentation assumptions, and works identically for both files. This replaces _function_source entirely and is pure stdlib (consistent with the generator's stdlib-only constraint). Fix this →

  • The two test classes (WgslCodegenTest / WgslTemplateEngineTest) and the module-level loader are cleanly separated. _write_spec is an appropriate local helper. No changes needed here.

Coverage gaps (optional, not blocking)

generate_variant_combinations/parse_template_spec are inherited from the Vulkan engine and have branches the tests don't reach. If webgpu will exercise them, worth a line each:

  • The int/uint rejection (gen_wgsl_headers.py:240-243) — a one-line assertRaises(ValueError) test.
  • The RANGE expansion (lines 226-236), including the _1/_2 suffix naming — currently only VALUE is tested.
  • The invalid variant-key assert in parse_template_spec (line 292) — the top-level-key mismatch is covered, but not an unknown key inside a shader_variants entry.
  • The combination/combinationN branch (lines 205-221) is untested. Likely out of scope for webgpu today; a log-style comment noting it's intentionally uncovered would prevent a false sense of completeness.

Nit

  • test_render_header_shape:111 asserts assertNotIn("kUpdateCacheWorkgroupSize ", h) (trailing space) to reject an untyped symbol — clever, but a touch cryptic; a short comment (# no untyped WorkgroupSize symbol, only X/Y/Z) would help the next reader.

None of the above blocks the PR — the suggestions are hardening the drift-guard and closing a few inherited-branch gaps. Nice work on the byte-identity and DD-009 regression cases.
· branch gh/JCNTH/2/head

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]

@SS-JIASS-JIA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@meta-codesync
meta-codesyncBot merged commit 706bce5 into gh/JCNTH/2/baseJul 9, 2026
175 of 183 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/2/head branch July 9, 2026 21:39
JCNTH added a commit that referenced this pull request Jul 9, 2026
Pull Request resolved: #20728
Additive coverage for the template engine added in the previous diff.
Key additions (`test/test_wgsl_codegen.py`, `WgslTemplateEngineTest`):
- `preprocess` `$if`/`$else` branch selection + `${...}` substitution + guarded-body indentation.
- the DTYPE x VEC variant matrix and `SUFFIX` rules (`""` suppresses; defaults to `str(VALUE)`).
- `parse_template_spec` — minimal-spec expansion, duplicate-key rejection (via the `UniqueKeyLoader`), top-level-key validation.
- the 3 WGSL type-helpers (`buffer_scalar_type` / `buffer_gvec_type` / `accum_scalar_type`).
- a byte-identity round-trip reproducing both committed `rms_norm` headers from the template + `rms_norm.yaml`.
- a source-of-truth sync-lock asserting the shared `$`-block transpiler helpers and the `UniqueKeyLoader` stay character-identical to `gen_vulkan_spv.py` (guards against silent drift).
ghstack-source-id: 401515160
@exported-using-ghexport
Differential Revision: [D110660006](https://our.internmc.facebook.com/intern/diff/D110660006/)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@JCNTH@SS-JIA
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen - #20728

Merged
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head
Jul 9, 2026
Merged

[ExecuTorch][WebGPU] Unit tests for the WGSL shader-variant codegen#20728
meta-codesync[bot] merged 10 commits into
gh/JCNTH/2/basefrom
gh/JCNTH/2/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Additive coverage for the template engine added in the previous diff.

Key additions (test/test_wgsl_codegen.py, WgslTemplateEngineTest):

  • preprocess$if/$else branch selection + ${...} substitution + guarded-body indentation.
  • the DTYPE x VEC variant matrix and SUFFIX rules ("" suppresses; defaults to str(VALUE)).
  • parse_template_spec — minimal-spec expansion, duplicate-key rejection (via the UniqueKeyLoader), top-level-key validation.
  • the 3 WGSL type-helpers (buffer_scalar_type / buffer_gvec_type / accum_scalar_type).
  • a byte-identity round-trip reproducing both committed rms_norm headers from the template + rms_norm.yaml.
  • a source-of-truth sync-lock asserting the shared $-block transpiler helpers and the UniqueKeyLoader stay character-identical to gen_vulkan_spv.py (guards against silent drift).
    @exported-using-ghexport

Differential Revision: D110660006

Differential Revision: D110660006

[ghstack-poisoned]
@pytorch-bot

pytorch-botBot commented Jul 5, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20728

Note: Links to docs will display an error until the docs builds have been completed.

❌ 5 New Failures, 2 Pending, 1 Unrelated Failure

As of commit 090cdc3 with merge base f4b01a8 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla

linux-foundation-easyclaBot commented Jul 5, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@JCNTH

JCNTH commented Jul 5, 2026

Copy link
Copy Markdown
ContributorAuthor

@claude throoughly review, check for overengineering, correctness, and areaas for modularization

@claude

claudeBot commented Jul 5, 2026

Copy link
Copy Markdown

Claude finished @JCNTH's task in 2m 40s —— View job


Review: WGSL shader-variant codegen unit tests

  • Read the test file
  • Read the codegen source (gen_wgsl_headers.py) under test
  • Cross-check the sync-lock against gen_vulkan_spv.py
  • Assess correctness, overengineering, modularization

Solid, well-targeted test suite. The names are descriptive, the docstrings capture the why (the DD-009 enable f16 note and the raw-string-terminator case are especially good), and every assertion I traced lines up with the actual API in gen_wgsl_headers.py. I verified the three shared helpers are byte-identical between the two files, and all files the tests reference (rms_norm.wgsl/.json/.h) exist. Verdict: approve with minor suggestions — nothing blocking.

Note: I reviewed by inspection + cross-file diffing; the sandbox blocked me from actually running pytest, so I did not get a green run locally.


Correctness

  • _function_source (lines 36–61) is the one piece with real fragility. Its body-end heuristic — "stop at the next non-blank column-0 line" — is correct for the current layout (escape is followed by a # comment, preprocess by class SPVGenerator:, both at column 0), but it will silently truncate if any helper ever gains a line that starts at column 0 inside the body (e.g. a multi-line string, or a ) closing a call at column 0). For a test whose whole purpose is guarding against drift, the parser guarding it shouldn't itself be position-sensitive. See the modularization note for a sturdier approach.

  • The two-phase signature skip (lines 50–53) correctly handles preprocess's multi-line signature whose ) -> str: sits at column 0 — good catch by the author. Worth noting the loop assumes the first line ending in : is the signature terminator; a default value like x: Dict = {} ending a wrapped signature line would fool it. Not a problem for these three functions, but it's the same class of latent fragility.

Overengineering

  • Reasonable overall — this isn't gold-plated. The single candidate is the sync-lock's hand-rolled text parser: ~26 lines of column heuristics to enforce char-identity of 3 functions. Since gen_wgsl_headers.pyalready imports these functions, the webgpu side can just use inspect.getsource(getattr(g, fn)) and only the (un-importable) Vulkan side needs source extraction — and that extraction is more robustly done with ast than regex/column-counting. That removes the fragility and shrinks the helper.

  • Mild redundancy: test_rms_norm_template_roundtrip_byte_identical (416) overlaps test_committed_headers_match_generator (134) — the committed rms_norm headers are already validated through headers_for_shader, which runs the same template expansion. The round-trip test earns its keep by exercising preprocess + render_headerdirectly (bypassing spec parsing) and pinning the concrete variant names, so I'd keep it — just calling out the overlap.

Modularization

  • Suggested: extract via ast instead of column heuristics.ast.parse(text) + walk for FunctionDef with the target name + ast.get_source_segment(text, node) gives the exact source span with no indentation assumptions, and works identically for both files. This replaces _function_source entirely and is pure stdlib (consistent with the generator's stdlib-only constraint). Fix this →

  • The two test classes (WgslCodegenTest / WgslTemplateEngineTest) and the module-level loader are cleanly separated. _write_spec is an appropriate local helper. No changes needed here.

Coverage gaps (optional, not blocking)

generate_variant_combinations/parse_template_spec are inherited from the Vulkan engine and have branches the tests don't reach. If webgpu will exercise them, worth a line each:

  • The int/uint rejection (gen_wgsl_headers.py:240-243) — a one-line assertRaises(ValueError) test.
  • The RANGE expansion (lines 226-236), including the _1/_2 suffix naming — currently only VALUE is tested.
  • The invalid variant-key assert in parse_template_spec (line 292) — the top-level-key mismatch is covered, but not an unknown key inside a shader_variants entry.
  • The combination/combinationN branch (lines 205-221) is untested. Likely out of scope for webgpu today; a log-style comment noting it's intentionally uncovered would prevent a false sense of completeness.

Nit

  • test_render_header_shape:111 asserts assertNotIn("kUpdateCacheWorkgroupSize ", h) (trailing space) to reject an untyped symbol — clever, but a touch cryptic; a short comment (# no untyped WorkgroupSize symbol, only X/Y/Z) would help the next reader.

None of the above blocks the PR — the suggestions are hardening the drift-guard and closing a few inherited-branch gaps. Nice work on the byte-identity and DD-009 regression cases.
· branch gh/JCNTH/2/head

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]

@SS-JIASS-JIA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@meta-codesync
meta-codesyncBot merged commit 706bce5 into gh/JCNTH/2/baseJul 9, 2026
175 of 183 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/2/head branch July 9, 2026 21:39
JCNTH added a commit that referenced this pull request Jul 9, 2026
Pull Request resolved: #20728
Additive coverage for the template engine added in the previous diff.
Key additions (`test/test_wgsl_codegen.py`, `WgslTemplateEngineTest`):
- `preprocess` `$if`/`$else` branch selection + `${...}` substitution + guarded-body indentation.
- the DTYPE x VEC variant matrix and `SUFFIX` rules (`""` suppresses; defaults to `str(VALUE)`).
- `parse_template_spec` — minimal-spec expansion, duplicate-key rejection (via the `UniqueKeyLoader`), top-level-key validation.
- the 3 WGSL type-helpers (`buffer_scalar_type` / `buffer_gvec_type` / `accum_scalar_type`).
- a byte-identity round-trip reproducing both committed `rms_norm` headers from the template + `rms_norm.yaml`.
- a source-of-truth sync-lock asserting the shared `$`-block transpiler helpers and the `UniqueKeyLoader` stay character-identical to `gen_vulkan_spv.py` (guards against silent drift).
ghstack-source-id: 401515160
@exported-using-ghexport
Differential Revision: [D110660006](https://our.internmc.facebook.com/intern/diff/D110660006/)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@JCNTH@SS-JIA