[executorch][vulkan] Add payload-bounded constant sharding - #21404

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

[executorch][vulkan] Add payload-bounded constant sharding#21404
meta-codesync[bot] merged 2 commits into
gh/JCNTH/194/basefrom
gh/JCNTH/194/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Enable deterministic payload-bounded Vulkan constant sharding

Problem
Large Vulkan exports can make inline PTE named data exceed downstream artifact limits, but NamedDataStore cannot externalize that data into bounded groups.

Solution

  • Before: Vulkan constants remain inline with no payload-bounded externalization option.
  • After: an opt-in uint64 raw-data cap externalizes constants after graph construction into deterministic shards.

Implementation

  • NamedDataStore.externalize_pte_data groups aliases by backing buffer, preserves existing tags, and rejects oversize buffers or collisions atomically.
  • parse_compile_options and parse_compile_spec transport external_constants_max_data_bytes as a strict positive uint64.
  • VulkanBackend.preprocess applies sharding only when the option is present.

Constraints
The default path is unchanged; buffers are not copied or split. The cap bounds unique raw buffer bytes per external tag, not final FlatTensor file size including alignment and header overhead.

Co-authored-with: Claude Code.
@exported-using-ghexport

Differential Revision: D113608555

Differential Revision: D113608555

cc @SS-JIA@manuelcandales@digantdesai@cbilgin

[ghstack-poisoned]
@pytorch-botpytorch-botBot added the module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/ label Jul 27, 2026
@pytorch-bot

pytorch-botBot commented Jul 27, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure, 6 Unrelated Failures

As of commit d0e8ae3 with merge base 2439920 (image):

NEW FAILURE - The following job has failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@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

Copy link
Copy Markdown
ContributorAuthor

@claude Thoroughly review for redundancy or areas of modularization or any high value changes

@claude

claudeBot commented Jul 27, 2026

Copy link
Copy Markdown

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


Review: payload-bounded Vulkan constant sharding

Thorough pass focused on redundancy, modularization, and high-value changes. Overall this is clean, well-tested (atomicity, idempotency, aliased-buffer-counted-once, order-independence are all covered), and the default path is genuinely untouched. Notes below, roughly in priority order.

1. externalize_pte_data externalizes allpte_data, but Vulkan labels it vulkan_constants

backends/vulkan/vulkan_preprocess.py:264-268 calls the method right after build_graph() and tags everything vulkan_constants. The method itself (_named_data_store.py:227-267) moves the entirepte_data map, not just constants. This is correct only if pte_data contains nothing but constants at that point. If any non-constant named data can land in pte_data before this call, it would be silently mislabeled as a constant shard. Worth a one-line assertion or comment documenting that invariant, since the method name (pte_data) and the Vulkan tag (constants) disagree.

2. The positive-uint64 check is validated in three places

  • parse_compile_optionsvulkan_partitioner.py:291-299 (rejects bool / non-int / <=0 / >= 1<<64)
  • _parse_external_constants_max_data_bytesvulkan_preprocess.py:93-99 (decode: length-8 + <=0)
  • externalize_pte_data_named_data_store.py:218-223 (bool / non-int / <=0)

Each with a slightly different message ("positive uint64" / "encoded as uint64" / "positive integer"), and each asserted by a separate test. Defense-in-depth across the encode → CompileSpec-bytes → decode → store boundaries is defensible, but the encode and decode validators live in the two Vulkan files and could share one helper. At minimum, the duplication is worth a comment explaining it's intentional boundary validation rather than an oversight. Fix this →

3. Greedy packing is key-sorted, not size-sorted

_named_data_store.py:234-251 does first-fit in sorted-key order. This is deterministic (good, and the reason for the sort) but not size-optimal — it can produce more shards than a first-fit-decreasing pass would. Given the PR explicitly documents "buffers are not copied or split" and prioritizes determinism, this is a reasonable trade-off, but a brief comment on why key-order over size-order (reproducibility) would help the next reader who is tempted to "optimize" it.

4. Test coupling to internal structure

test_vulkan_compile_options.py:_preprocess_named_data patches four internal symbols by name (unsafe_remove_auto_functionalized_pass, apply_passes, VkGraphBuilder, serialize_vulkan_graph). This verifies the wiring well, but it's tightly coupled to the current internals of preprocess and will break on unrelated refactors. Acceptable given a real graph would be heavyweight, but flagging as a maintenance cost.

Minor / positive

  • Good: placing externalize_pte_data on the generic NamedDataStore rather than in Vulkan makes it reusable by other backends — nice modularization.
  • Good: the external_data shallow-copy at _named_data_store.py:253-255 before mutation gives the atomic-on-collision guarantee the tests assert. Note the oversize check runs earlier (line 239) so that path is atomic too.
  • _parse_external_constants_max_data_bytes is a single-use helper, which CLAUDE.md discourages, but here it cleanly separates two distinct error messages (encoding vs. value) — I'd keep it.
  • Content-addressed tag naming (sha256 of the joined keys) is a solid choice for reproducibility and the collision guard is correct.

No blocking correctness bugs found. The main thing I'd want confirmed before merge is #1 — the pte_data-is-only-constants invariant at the call site.
· branch gh/JCNTH/194/head

@meta-clameta-claBot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 27, 2026
[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.

@meta-codesync
meta-codesyncBot merged commit 41760ec into gh/JCNTH/194/baseJul 29, 2026
188 of 197 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/194/head branch July 29, 2026 17:44
JCNTH added a commit that referenced this pull request Jul 29, 2026
This PR was created by the merge bot to help merge the original PR into
the main branch.
ghstack PR number: #21404 by
@JCNTH
^ Please use this as the source of truth for the PR details, comments,
and reviews
ghstack PR base:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/base
ghstack PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/main
Merge bot PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/orig
@diff-train-skip-merge
cc @SS-JIA@manuelcandales@digantdesai@cbilgin
---------
Co-authored-by: Julian Ng-Thow-Hing <juliannth@meta.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Julian Ng-Thow-Hing <107437036+JCNTH@users.noreply.github.com>
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-exportedmodule: vulkanIssues related to the Vulkan delegate and code under backends/vulkan/

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][vulkan] Add payload-bounded constant sharding - #21404

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

[executorch][vulkan] Add payload-bounded constant sharding#21404
meta-codesync[bot] merged 2 commits into
gh/JCNTH/194/basefrom
gh/JCNTH/194/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Enable deterministic payload-bounded Vulkan constant sharding

Problem
Large Vulkan exports can make inline PTE named data exceed downstream artifact limits, but NamedDataStore cannot externalize that data into bounded groups.

Solution

  • Before: Vulkan constants remain inline with no payload-bounded externalization option.
  • After: an opt-in uint64 raw-data cap externalizes constants after graph construction into deterministic shards.

Implementation

  • NamedDataStore.externalize_pte_data groups aliases by backing buffer, preserves existing tags, and rejects oversize buffers or collisions atomically.
  • parse_compile_options and parse_compile_spec transport external_constants_max_data_bytes as a strict positive uint64.
  • VulkanBackend.preprocess applies sharding only when the option is present.

Constraints
The default path is unchanged; buffers are not copied or split. The cap bounds unique raw buffer bytes per external tag, not final FlatTensor file size including alignment and header overhead.

Co-authored-with: Claude Code.
@exported-using-ghexport

Differential Revision: D113608555

Differential Revision: D113608555

cc @SS-JIA@manuelcandales@digantdesai@cbilgin

[ghstack-poisoned]
@pytorch-botpytorch-botBot added the module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/ label Jul 27, 2026
@pytorch-bot

pytorch-botBot commented Jul 27, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure, 6 Unrelated Failures

As of commit d0e8ae3 with merge base 2439920 (image):

NEW FAILURE - The following job has failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@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

Copy link
Copy Markdown
ContributorAuthor

@claude Thoroughly review for redundancy or areas of modularization or any high value changes

@claude

claudeBot commented Jul 27, 2026

Copy link
Copy Markdown

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


Review: payload-bounded Vulkan constant sharding

Thorough pass focused on redundancy, modularization, and high-value changes. Overall this is clean, well-tested (atomicity, idempotency, aliased-buffer-counted-once, order-independence are all covered), and the default path is genuinely untouched. Notes below, roughly in priority order.

1. externalize_pte_data externalizes allpte_data, but Vulkan labels it vulkan_constants

backends/vulkan/vulkan_preprocess.py:264-268 calls the method right after build_graph() and tags everything vulkan_constants. The method itself (_named_data_store.py:227-267) moves the entirepte_data map, not just constants. This is correct only if pte_data contains nothing but constants at that point. If any non-constant named data can land in pte_data before this call, it would be silently mislabeled as a constant shard. Worth a one-line assertion or comment documenting that invariant, since the method name (pte_data) and the Vulkan tag (constants) disagree.

2. The positive-uint64 check is validated in three places

  • parse_compile_optionsvulkan_partitioner.py:291-299 (rejects bool / non-int / <=0 / >= 1<<64)
  • _parse_external_constants_max_data_bytesvulkan_preprocess.py:93-99 (decode: length-8 + <=0)
  • externalize_pte_data_named_data_store.py:218-223 (bool / non-int / <=0)

Each with a slightly different message ("positive uint64" / "encoded as uint64" / "positive integer"), and each asserted by a separate test. Defense-in-depth across the encode → CompileSpec-bytes → decode → store boundaries is defensible, but the encode and decode validators live in the two Vulkan files and could share one helper. At minimum, the duplication is worth a comment explaining it's intentional boundary validation rather than an oversight. Fix this →

3. Greedy packing is key-sorted, not size-sorted

_named_data_store.py:234-251 does first-fit in sorted-key order. This is deterministic (good, and the reason for the sort) but not size-optimal — it can produce more shards than a first-fit-decreasing pass would. Given the PR explicitly documents "buffers are not copied or split" and prioritizes determinism, this is a reasonable trade-off, but a brief comment on why key-order over size-order (reproducibility) would help the next reader who is tempted to "optimize" it.

4. Test coupling to internal structure

test_vulkan_compile_options.py:_preprocess_named_data patches four internal symbols by name (unsafe_remove_auto_functionalized_pass, apply_passes, VkGraphBuilder, serialize_vulkan_graph). This verifies the wiring well, but it's tightly coupled to the current internals of preprocess and will break on unrelated refactors. Acceptable given a real graph would be heavyweight, but flagging as a maintenance cost.

Minor / positive

  • Good: placing externalize_pte_data on the generic NamedDataStore rather than in Vulkan makes it reusable by other backends — nice modularization.
  • Good: the external_data shallow-copy at _named_data_store.py:253-255 before mutation gives the atomic-on-collision guarantee the tests assert. Note the oversize check runs earlier (line 239) so that path is atomic too.
  • _parse_external_constants_max_data_bytes is a single-use helper, which CLAUDE.md discourages, but here it cleanly separates two distinct error messages (encoding vs. value) — I'd keep it.
  • Content-addressed tag naming (sha256 of the joined keys) is a solid choice for reproducibility and the collision guard is correct.

No blocking correctness bugs found. The main thing I'd want confirmed before merge is #1 — the pte_data-is-only-constants invariant at the call site.
· branch gh/JCNTH/194/head

@meta-clameta-claBot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 27, 2026
[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.

@meta-codesync
meta-codesyncBot merged commit 41760ec into gh/JCNTH/194/baseJul 29, 2026
188 of 197 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/194/head branch July 29, 2026 17:44
JCNTH added a commit that referenced this pull request Jul 29, 2026
This PR was created by the merge bot to help merge the original PR into
the main branch.
ghstack PR number: #21404 by
@JCNTH
^ Please use this as the source of truth for the PR details, comments,
and reviews
ghstack PR base:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/base
ghstack PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/main
Merge bot PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/orig
@diff-train-skip-merge
cc @SS-JIA@manuelcandales@digantdesai@cbilgin
---------
Co-authored-by: Julian Ng-Thow-Hing <juliannth@meta.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Julian Ng-Thow-Hing <107437036+JCNTH@users.noreply.github.com>
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-exportedmodule: vulkanIssues related to the Vulkan delegate and code under backends/vulkan/

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][vulkan] Add payload-bounded constant sharding - #21404

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

[executorch][vulkan] Add payload-bounded constant sharding#21404
meta-codesync[bot] merged 2 commits into
gh/JCNTH/194/basefrom
gh/JCNTH/194/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Enable deterministic payload-bounded Vulkan constant sharding

Problem
Large Vulkan exports can make inline PTE named data exceed downstream artifact limits, but NamedDataStore cannot externalize that data into bounded groups.

Solution

  • Before: Vulkan constants remain inline with no payload-bounded externalization option.
  • After: an opt-in uint64 raw-data cap externalizes constants after graph construction into deterministic shards.

Implementation

  • NamedDataStore.externalize_pte_data groups aliases by backing buffer, preserves existing tags, and rejects oversize buffers or collisions atomically.
  • parse_compile_options and parse_compile_spec transport external_constants_max_data_bytes as a strict positive uint64.
  • VulkanBackend.preprocess applies sharding only when the option is present.

Constraints
The default path is unchanged; buffers are not copied or split. The cap bounds unique raw buffer bytes per external tag, not final FlatTensor file size including alignment and header overhead.

Co-authored-with: Claude Code.
@exported-using-ghexport

Differential Revision: D113608555

Differential Revision: D113608555

cc @SS-JIA@manuelcandales@digantdesai@cbilgin

[ghstack-poisoned]
@pytorch-botpytorch-botBot added the module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/ label Jul 27, 2026
@pytorch-bot

pytorch-botBot commented Jul 27, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure, 6 Unrelated Failures

As of commit d0e8ae3 with merge base 2439920 (image):

NEW FAILURE - The following job has failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@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

Copy link
Copy Markdown
ContributorAuthor

@claude Thoroughly review for redundancy or areas of modularization or any high value changes

@claude

claudeBot commented Jul 27, 2026

Copy link
Copy Markdown

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


Review: payload-bounded Vulkan constant sharding

Thorough pass focused on redundancy, modularization, and high-value changes. Overall this is clean, well-tested (atomicity, idempotency, aliased-buffer-counted-once, order-independence are all covered), and the default path is genuinely untouched. Notes below, roughly in priority order.

1. externalize_pte_data externalizes allpte_data, but Vulkan labels it vulkan_constants

backends/vulkan/vulkan_preprocess.py:264-268 calls the method right after build_graph() and tags everything vulkan_constants. The method itself (_named_data_store.py:227-267) moves the entirepte_data map, not just constants. This is correct only if pte_data contains nothing but constants at that point. If any non-constant named data can land in pte_data before this call, it would be silently mislabeled as a constant shard. Worth a one-line assertion or comment documenting that invariant, since the method name (pte_data) and the Vulkan tag (constants) disagree.

2. The positive-uint64 check is validated in three places

  • parse_compile_optionsvulkan_partitioner.py:291-299 (rejects bool / non-int / <=0 / >= 1<<64)
  • _parse_external_constants_max_data_bytesvulkan_preprocess.py:93-99 (decode: length-8 + <=0)
  • externalize_pte_data_named_data_store.py:218-223 (bool / non-int / <=0)

Each with a slightly different message ("positive uint64" / "encoded as uint64" / "positive integer"), and each asserted by a separate test. Defense-in-depth across the encode → CompileSpec-bytes → decode → store boundaries is defensible, but the encode and decode validators live in the two Vulkan files and could share one helper. At minimum, the duplication is worth a comment explaining it's intentional boundary validation rather than an oversight. Fix this →

3. Greedy packing is key-sorted, not size-sorted

_named_data_store.py:234-251 does first-fit in sorted-key order. This is deterministic (good, and the reason for the sort) but not size-optimal — it can produce more shards than a first-fit-decreasing pass would. Given the PR explicitly documents "buffers are not copied or split" and prioritizes determinism, this is a reasonable trade-off, but a brief comment on why key-order over size-order (reproducibility) would help the next reader who is tempted to "optimize" it.

4. Test coupling to internal structure

test_vulkan_compile_options.py:_preprocess_named_data patches four internal symbols by name (unsafe_remove_auto_functionalized_pass, apply_passes, VkGraphBuilder, serialize_vulkan_graph). This verifies the wiring well, but it's tightly coupled to the current internals of preprocess and will break on unrelated refactors. Acceptable given a real graph would be heavyweight, but flagging as a maintenance cost.

Minor / positive

  • Good: placing externalize_pte_data on the generic NamedDataStore rather than in Vulkan makes it reusable by other backends — nice modularization.
  • Good: the external_data shallow-copy at _named_data_store.py:253-255 before mutation gives the atomic-on-collision guarantee the tests assert. Note the oversize check runs earlier (line 239) so that path is atomic too.
  • _parse_external_constants_max_data_bytes is a single-use helper, which CLAUDE.md discourages, but here it cleanly separates two distinct error messages (encoding vs. value) — I'd keep it.
  • Content-addressed tag naming (sha256 of the joined keys) is a solid choice for reproducibility and the collision guard is correct.

No blocking correctness bugs found. The main thing I'd want confirmed before merge is #1 — the pte_data-is-only-constants invariant at the call site.
· branch gh/JCNTH/194/head

@meta-clameta-claBot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 27, 2026
[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.

@meta-codesync
meta-codesyncBot merged commit 41760ec into gh/JCNTH/194/baseJul 29, 2026
188 of 197 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/194/head branch July 29, 2026 17:44
JCNTH added a commit that referenced this pull request Jul 29, 2026
This PR was created by the merge bot to help merge the original PR into
the main branch.
ghstack PR number: #21404 by
@JCNTH
^ Please use this as the source of truth for the PR details, comments,
and reviews
ghstack PR base:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/base
ghstack PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/main
Merge bot PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/orig
@diff-train-skip-merge
cc @SS-JIA@manuelcandales@digantdesai@cbilgin
---------
Co-authored-by: Julian Ng-Thow-Hing <juliannth@meta.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Julian Ng-Thow-Hing <107437036+JCNTH@users.noreply.github.com>
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-exportedmodule: vulkanIssues related to the Vulkan delegate and code under backends/vulkan/

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][vulkan] Add payload-bounded constant sharding - #21404

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

[executorch][vulkan] Add payload-bounded constant sharding#21404
meta-codesync[bot] merged 2 commits into
gh/JCNTH/194/basefrom
gh/JCNTH/194/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Enable deterministic payload-bounded Vulkan constant sharding

Problem
Large Vulkan exports can make inline PTE named data exceed downstream artifact limits, but NamedDataStore cannot externalize that data into bounded groups.

Solution

  • Before: Vulkan constants remain inline with no payload-bounded externalization option.
  • After: an opt-in uint64 raw-data cap externalizes constants after graph construction into deterministic shards.

Implementation

  • NamedDataStore.externalize_pte_data groups aliases by backing buffer, preserves existing tags, and rejects oversize buffers or collisions atomically.
  • parse_compile_options and parse_compile_spec transport external_constants_max_data_bytes as a strict positive uint64.
  • VulkanBackend.preprocess applies sharding only when the option is present.

Constraints
The default path is unchanged; buffers are not copied or split. The cap bounds unique raw buffer bytes per external tag, not final FlatTensor file size including alignment and header overhead.

Co-authored-with: Claude Code.
@exported-using-ghexport

Differential Revision: D113608555

Differential Revision: D113608555

cc @SS-JIA@manuelcandales@digantdesai@cbilgin

[ghstack-poisoned]
@pytorch-botpytorch-botBot added the module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/ label Jul 27, 2026
@pytorch-bot

pytorch-botBot commented Jul 27, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure, 6 Unrelated Failures

As of commit d0e8ae3 with merge base 2439920 (image):

NEW FAILURE - The following job has failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@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

Copy link
Copy Markdown
ContributorAuthor

@claude Thoroughly review for redundancy or areas of modularization or any high value changes

@claude

claudeBot commented Jul 27, 2026

Copy link
Copy Markdown

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


Review: payload-bounded Vulkan constant sharding

Thorough pass focused on redundancy, modularization, and high-value changes. Overall this is clean, well-tested (atomicity, idempotency, aliased-buffer-counted-once, order-independence are all covered), and the default path is genuinely untouched. Notes below, roughly in priority order.

1. externalize_pte_data externalizes allpte_data, but Vulkan labels it vulkan_constants

backends/vulkan/vulkan_preprocess.py:264-268 calls the method right after build_graph() and tags everything vulkan_constants. The method itself (_named_data_store.py:227-267) moves the entirepte_data map, not just constants. This is correct only if pte_data contains nothing but constants at that point. If any non-constant named data can land in pte_data before this call, it would be silently mislabeled as a constant shard. Worth a one-line assertion or comment documenting that invariant, since the method name (pte_data) and the Vulkan tag (constants) disagree.

2. The positive-uint64 check is validated in three places

  • parse_compile_optionsvulkan_partitioner.py:291-299 (rejects bool / non-int / <=0 / >= 1<<64)
  • _parse_external_constants_max_data_bytesvulkan_preprocess.py:93-99 (decode: length-8 + <=0)
  • externalize_pte_data_named_data_store.py:218-223 (bool / non-int / <=0)

Each with a slightly different message ("positive uint64" / "encoded as uint64" / "positive integer"), and each asserted by a separate test. Defense-in-depth across the encode → CompileSpec-bytes → decode → store boundaries is defensible, but the encode and decode validators live in the two Vulkan files and could share one helper. At minimum, the duplication is worth a comment explaining it's intentional boundary validation rather than an oversight. Fix this →

3. Greedy packing is key-sorted, not size-sorted

_named_data_store.py:234-251 does first-fit in sorted-key order. This is deterministic (good, and the reason for the sort) but not size-optimal — it can produce more shards than a first-fit-decreasing pass would. Given the PR explicitly documents "buffers are not copied or split" and prioritizes determinism, this is a reasonable trade-off, but a brief comment on why key-order over size-order (reproducibility) would help the next reader who is tempted to "optimize" it.

4. Test coupling to internal structure

test_vulkan_compile_options.py:_preprocess_named_data patches four internal symbols by name (unsafe_remove_auto_functionalized_pass, apply_passes, VkGraphBuilder, serialize_vulkan_graph). This verifies the wiring well, but it's tightly coupled to the current internals of preprocess and will break on unrelated refactors. Acceptable given a real graph would be heavyweight, but flagging as a maintenance cost.

Minor / positive

  • Good: placing externalize_pte_data on the generic NamedDataStore rather than in Vulkan makes it reusable by other backends — nice modularization.
  • Good: the external_data shallow-copy at _named_data_store.py:253-255 before mutation gives the atomic-on-collision guarantee the tests assert. Note the oversize check runs earlier (line 239) so that path is atomic too.
  • _parse_external_constants_max_data_bytes is a single-use helper, which CLAUDE.md discourages, but here it cleanly separates two distinct error messages (encoding vs. value) — I'd keep it.
  • Content-addressed tag naming (sha256 of the joined keys) is a solid choice for reproducibility and the collision guard is correct.

No blocking correctness bugs found. The main thing I'd want confirmed before merge is #1 — the pte_data-is-only-constants invariant at the call site.
· branch gh/JCNTH/194/head

@meta-clameta-claBot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 27, 2026
[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.

@meta-codesync
meta-codesyncBot merged commit 41760ec into gh/JCNTH/194/baseJul 29, 2026
188 of 197 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/194/head branch July 29, 2026 17:44
JCNTH added a commit that referenced this pull request Jul 29, 2026
This PR was created by the merge bot to help merge the original PR into
the main branch.
ghstack PR number: #21404 by
@JCNTH
^ Please use this as the source of truth for the PR details, comments,
and reviews
ghstack PR base:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/base
ghstack PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/main
Merge bot PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/orig
@diff-train-skip-merge
cc @SS-JIA@manuelcandales@digantdesai@cbilgin
---------
Co-authored-by: Julian Ng-Thow-Hing <juliannth@meta.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Julian Ng-Thow-Hing <107437036+JCNTH@users.noreply.github.com>
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-exportedmodule: vulkanIssues related to the Vulkan delegate and code under backends/vulkan/

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][vulkan] Add payload-bounded constant sharding - #21404

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

[executorch][vulkan] Add payload-bounded constant sharding#21404
meta-codesync[bot] merged 2 commits into
gh/JCNTH/194/basefrom
gh/JCNTH/194/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Enable deterministic payload-bounded Vulkan constant sharding

Problem
Large Vulkan exports can make inline PTE named data exceed downstream artifact limits, but NamedDataStore cannot externalize that data into bounded groups.

Solution

  • Before: Vulkan constants remain inline with no payload-bounded externalization option.
  • After: an opt-in uint64 raw-data cap externalizes constants after graph construction into deterministic shards.

Implementation

  • NamedDataStore.externalize_pte_data groups aliases by backing buffer, preserves existing tags, and rejects oversize buffers or collisions atomically.
  • parse_compile_options and parse_compile_spec transport external_constants_max_data_bytes as a strict positive uint64.
  • VulkanBackend.preprocess applies sharding only when the option is present.

Constraints
The default path is unchanged; buffers are not copied or split. The cap bounds unique raw buffer bytes per external tag, not final FlatTensor file size including alignment and header overhead.

Co-authored-with: Claude Code.
@exported-using-ghexport

Differential Revision: D113608555

Differential Revision: D113608555

cc @SS-JIA@manuelcandales@digantdesai@cbilgin

[ghstack-poisoned]
@pytorch-botpytorch-botBot added the module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/ label Jul 27, 2026
@pytorch-bot

pytorch-botBot commented Jul 27, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure, 6 Unrelated Failures

As of commit d0e8ae3 with merge base 2439920 (image):

NEW FAILURE - The following job has failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@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

Copy link
Copy Markdown
ContributorAuthor

@claude Thoroughly review for redundancy or areas of modularization or any high value changes

@claude

claudeBot commented Jul 27, 2026

Copy link
Copy Markdown

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


Review: payload-bounded Vulkan constant sharding

Thorough pass focused on redundancy, modularization, and high-value changes. Overall this is clean, well-tested (atomicity, idempotency, aliased-buffer-counted-once, order-independence are all covered), and the default path is genuinely untouched. Notes below, roughly in priority order.

1. externalize_pte_data externalizes allpte_data, but Vulkan labels it vulkan_constants

backends/vulkan/vulkan_preprocess.py:264-268 calls the method right after build_graph() and tags everything vulkan_constants. The method itself (_named_data_store.py:227-267) moves the entirepte_data map, not just constants. This is correct only if pte_data contains nothing but constants at that point. If any non-constant named data can land in pte_data before this call, it would be silently mislabeled as a constant shard. Worth a one-line assertion or comment documenting that invariant, since the method name (pte_data) and the Vulkan tag (constants) disagree.

2. The positive-uint64 check is validated in three places

  • parse_compile_optionsvulkan_partitioner.py:291-299 (rejects bool / non-int / <=0 / >= 1<<64)
  • _parse_external_constants_max_data_bytesvulkan_preprocess.py:93-99 (decode: length-8 + <=0)
  • externalize_pte_data_named_data_store.py:218-223 (bool / non-int / <=0)

Each with a slightly different message ("positive uint64" / "encoded as uint64" / "positive integer"), and each asserted by a separate test. Defense-in-depth across the encode → CompileSpec-bytes → decode → store boundaries is defensible, but the encode and decode validators live in the two Vulkan files and could share one helper. At minimum, the duplication is worth a comment explaining it's intentional boundary validation rather than an oversight. Fix this →

3. Greedy packing is key-sorted, not size-sorted

_named_data_store.py:234-251 does first-fit in sorted-key order. This is deterministic (good, and the reason for the sort) but not size-optimal — it can produce more shards than a first-fit-decreasing pass would. Given the PR explicitly documents "buffers are not copied or split" and prioritizes determinism, this is a reasonable trade-off, but a brief comment on why key-order over size-order (reproducibility) would help the next reader who is tempted to "optimize" it.

4. Test coupling to internal structure

test_vulkan_compile_options.py:_preprocess_named_data patches four internal symbols by name (unsafe_remove_auto_functionalized_pass, apply_passes, VkGraphBuilder, serialize_vulkan_graph). This verifies the wiring well, but it's tightly coupled to the current internals of preprocess and will break on unrelated refactors. Acceptable given a real graph would be heavyweight, but flagging as a maintenance cost.

Minor / positive

  • Good: placing externalize_pte_data on the generic NamedDataStore rather than in Vulkan makes it reusable by other backends — nice modularization.
  • Good: the external_data shallow-copy at _named_data_store.py:253-255 before mutation gives the atomic-on-collision guarantee the tests assert. Note the oversize check runs earlier (line 239) so that path is atomic too.
  • _parse_external_constants_max_data_bytes is a single-use helper, which CLAUDE.md discourages, but here it cleanly separates two distinct error messages (encoding vs. value) — I'd keep it.
  • Content-addressed tag naming (sha256 of the joined keys) is a solid choice for reproducibility and the collision guard is correct.

No blocking correctness bugs found. The main thing I'd want confirmed before merge is #1 — the pte_data-is-only-constants invariant at the call site.
· branch gh/JCNTH/194/head

@meta-clameta-claBot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 27, 2026
[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.

@meta-codesync
meta-codesyncBot merged commit 41760ec into gh/JCNTH/194/baseJul 29, 2026
188 of 197 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/194/head branch July 29, 2026 17:44
JCNTH added a commit that referenced this pull request Jul 29, 2026
This PR was created by the merge bot to help merge the original PR into
the main branch.
ghstack PR number: #21404 by
@JCNTH
^ Please use this as the source of truth for the PR details, comments,
and reviews
ghstack PR base:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/base
ghstack PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/main
Merge bot PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/orig
@diff-train-skip-merge
cc @SS-JIA@manuelcandales@digantdesai@cbilgin
---------
Co-authored-by: Julian Ng-Thow-Hing <juliannth@meta.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Julian Ng-Thow-Hing <107437036+JCNTH@users.noreply.github.com>
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-exportedmodule: vulkanIssues related to the Vulkan delegate and code under backends/vulkan/

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][vulkan] Add payload-bounded constant sharding - #21404

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

[executorch][vulkan] Add payload-bounded constant sharding#21404
meta-codesync[bot] merged 2 commits into
gh/JCNTH/194/basefrom
gh/JCNTH/194/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Enable deterministic payload-bounded Vulkan constant sharding

Problem
Large Vulkan exports can make inline PTE named data exceed downstream artifact limits, but NamedDataStore cannot externalize that data into bounded groups.

Solution

  • Before: Vulkan constants remain inline with no payload-bounded externalization option.
  • After: an opt-in uint64 raw-data cap externalizes constants after graph construction into deterministic shards.

Implementation

  • NamedDataStore.externalize_pte_data groups aliases by backing buffer, preserves existing tags, and rejects oversize buffers or collisions atomically.
  • parse_compile_options and parse_compile_spec transport external_constants_max_data_bytes as a strict positive uint64.
  • VulkanBackend.preprocess applies sharding only when the option is present.

Constraints
The default path is unchanged; buffers are not copied or split. The cap bounds unique raw buffer bytes per external tag, not final FlatTensor file size including alignment and header overhead.

Co-authored-with: Claude Code.
@exported-using-ghexport

Differential Revision: D113608555

Differential Revision: D113608555

cc @SS-JIA@manuelcandales@digantdesai@cbilgin

[ghstack-poisoned]
@pytorch-botpytorch-botBot added the module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/ label Jul 27, 2026
@pytorch-bot

pytorch-botBot commented Jul 27, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure, 6 Unrelated Failures

As of commit d0e8ae3 with merge base 2439920 (image):

NEW FAILURE - The following job has failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@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

Copy link
Copy Markdown
ContributorAuthor

@claude Thoroughly review for redundancy or areas of modularization or any high value changes

@claude

claudeBot commented Jul 27, 2026

Copy link
Copy Markdown

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


Review: payload-bounded Vulkan constant sharding

Thorough pass focused on redundancy, modularization, and high-value changes. Overall this is clean, well-tested (atomicity, idempotency, aliased-buffer-counted-once, order-independence are all covered), and the default path is genuinely untouched. Notes below, roughly in priority order.

1. externalize_pte_data externalizes allpte_data, but Vulkan labels it vulkan_constants

backends/vulkan/vulkan_preprocess.py:264-268 calls the method right after build_graph() and tags everything vulkan_constants. The method itself (_named_data_store.py:227-267) moves the entirepte_data map, not just constants. This is correct only if pte_data contains nothing but constants at that point. If any non-constant named data can land in pte_data before this call, it would be silently mislabeled as a constant shard. Worth a one-line assertion or comment documenting that invariant, since the method name (pte_data) and the Vulkan tag (constants) disagree.

2. The positive-uint64 check is validated in three places

  • parse_compile_optionsvulkan_partitioner.py:291-299 (rejects bool / non-int / <=0 / >= 1<<64)
  • _parse_external_constants_max_data_bytesvulkan_preprocess.py:93-99 (decode: length-8 + <=0)
  • externalize_pte_data_named_data_store.py:218-223 (bool / non-int / <=0)

Each with a slightly different message ("positive uint64" / "encoded as uint64" / "positive integer"), and each asserted by a separate test. Defense-in-depth across the encode → CompileSpec-bytes → decode → store boundaries is defensible, but the encode and decode validators live in the two Vulkan files and could share one helper. At minimum, the duplication is worth a comment explaining it's intentional boundary validation rather than an oversight. Fix this →

3. Greedy packing is key-sorted, not size-sorted

_named_data_store.py:234-251 does first-fit in sorted-key order. This is deterministic (good, and the reason for the sort) but not size-optimal — it can produce more shards than a first-fit-decreasing pass would. Given the PR explicitly documents "buffers are not copied or split" and prioritizes determinism, this is a reasonable trade-off, but a brief comment on why key-order over size-order (reproducibility) would help the next reader who is tempted to "optimize" it.

4. Test coupling to internal structure

test_vulkan_compile_options.py:_preprocess_named_data patches four internal symbols by name (unsafe_remove_auto_functionalized_pass, apply_passes, VkGraphBuilder, serialize_vulkan_graph). This verifies the wiring well, but it's tightly coupled to the current internals of preprocess and will break on unrelated refactors. Acceptable given a real graph would be heavyweight, but flagging as a maintenance cost.

Minor / positive

  • Good: placing externalize_pte_data on the generic NamedDataStore rather than in Vulkan makes it reusable by other backends — nice modularization.
  • Good: the external_data shallow-copy at _named_data_store.py:253-255 before mutation gives the atomic-on-collision guarantee the tests assert. Note the oversize check runs earlier (line 239) so that path is atomic too.
  • _parse_external_constants_max_data_bytes is a single-use helper, which CLAUDE.md discourages, but here it cleanly separates two distinct error messages (encoding vs. value) — I'd keep it.
  • Content-addressed tag naming (sha256 of the joined keys) is a solid choice for reproducibility and the collision guard is correct.

No blocking correctness bugs found. The main thing I'd want confirmed before merge is #1 — the pte_data-is-only-constants invariant at the call site.
· branch gh/JCNTH/194/head

@meta-clameta-claBot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 27, 2026
[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.

@meta-codesync
meta-codesyncBot merged commit 41760ec into gh/JCNTH/194/baseJul 29, 2026
188 of 197 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/194/head branch July 29, 2026 17:44
JCNTH added a commit that referenced this pull request Jul 29, 2026
This PR was created by the merge bot to help merge the original PR into
the main branch.
ghstack PR number: #21404 by
@JCNTH
^ Please use this as the source of truth for the PR details, comments,
and reviews
ghstack PR base:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/base
ghstack PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/main
Merge bot PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/orig
@diff-train-skip-merge
cc @SS-JIA@manuelcandales@digantdesai@cbilgin
---------
Co-authored-by: Julian Ng-Thow-Hing <juliannth@meta.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Julian Ng-Thow-Hing <107437036+JCNTH@users.noreply.github.com>
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-exportedmodule: vulkanIssues related to the Vulkan delegate and code under backends/vulkan/

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][vulkan] Add payload-bounded constant sharding - #21404

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

[executorch][vulkan] Add payload-bounded constant sharding#21404
meta-codesync[bot] merged 2 commits into
gh/JCNTH/194/basefrom
gh/JCNTH/194/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Enable deterministic payload-bounded Vulkan constant sharding

Problem
Large Vulkan exports can make inline PTE named data exceed downstream artifact limits, but NamedDataStore cannot externalize that data into bounded groups.

Solution

  • Before: Vulkan constants remain inline with no payload-bounded externalization option.
  • After: an opt-in uint64 raw-data cap externalizes constants after graph construction into deterministic shards.

Implementation

  • NamedDataStore.externalize_pte_data groups aliases by backing buffer, preserves existing tags, and rejects oversize buffers or collisions atomically.
  • parse_compile_options and parse_compile_spec transport external_constants_max_data_bytes as a strict positive uint64.
  • VulkanBackend.preprocess applies sharding only when the option is present.

Constraints
The default path is unchanged; buffers are not copied or split. The cap bounds unique raw buffer bytes per external tag, not final FlatTensor file size including alignment and header overhead.

Co-authored-with: Claude Code.
@exported-using-ghexport

Differential Revision: D113608555

Differential Revision: D113608555

cc @SS-JIA@manuelcandales@digantdesai@cbilgin

[ghstack-poisoned]
@pytorch-botpytorch-botBot added the module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/ label Jul 27, 2026
@pytorch-bot

pytorch-botBot commented Jul 27, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure, 6 Unrelated Failures

As of commit d0e8ae3 with merge base 2439920 (image):

NEW FAILURE - The following job has failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@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

Copy link
Copy Markdown
ContributorAuthor

@claude Thoroughly review for redundancy or areas of modularization or any high value changes

@claude

claudeBot commented Jul 27, 2026

Copy link
Copy Markdown

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


Review: payload-bounded Vulkan constant sharding

Thorough pass focused on redundancy, modularization, and high-value changes. Overall this is clean, well-tested (atomicity, idempotency, aliased-buffer-counted-once, order-independence are all covered), and the default path is genuinely untouched. Notes below, roughly in priority order.

1. externalize_pte_data externalizes allpte_data, but Vulkan labels it vulkan_constants

backends/vulkan/vulkan_preprocess.py:264-268 calls the method right after build_graph() and tags everything vulkan_constants. The method itself (_named_data_store.py:227-267) moves the entirepte_data map, not just constants. This is correct only if pte_data contains nothing but constants at that point. If any non-constant named data can land in pte_data before this call, it would be silently mislabeled as a constant shard. Worth a one-line assertion or comment documenting that invariant, since the method name (pte_data) and the Vulkan tag (constants) disagree.

2. The positive-uint64 check is validated in three places

  • parse_compile_optionsvulkan_partitioner.py:291-299 (rejects bool / non-int / <=0 / >= 1<<64)
  • _parse_external_constants_max_data_bytesvulkan_preprocess.py:93-99 (decode: length-8 + <=0)
  • externalize_pte_data_named_data_store.py:218-223 (bool / non-int / <=0)

Each with a slightly different message ("positive uint64" / "encoded as uint64" / "positive integer"), and each asserted by a separate test. Defense-in-depth across the encode → CompileSpec-bytes → decode → store boundaries is defensible, but the encode and decode validators live in the two Vulkan files and could share one helper. At minimum, the duplication is worth a comment explaining it's intentional boundary validation rather than an oversight. Fix this →

3. Greedy packing is key-sorted, not size-sorted

_named_data_store.py:234-251 does first-fit in sorted-key order. This is deterministic (good, and the reason for the sort) but not size-optimal — it can produce more shards than a first-fit-decreasing pass would. Given the PR explicitly documents "buffers are not copied or split" and prioritizes determinism, this is a reasonable trade-off, but a brief comment on why key-order over size-order (reproducibility) would help the next reader who is tempted to "optimize" it.

4. Test coupling to internal structure

test_vulkan_compile_options.py:_preprocess_named_data patches four internal symbols by name (unsafe_remove_auto_functionalized_pass, apply_passes, VkGraphBuilder, serialize_vulkan_graph). This verifies the wiring well, but it's tightly coupled to the current internals of preprocess and will break on unrelated refactors. Acceptable given a real graph would be heavyweight, but flagging as a maintenance cost.

Minor / positive

  • Good: placing externalize_pte_data on the generic NamedDataStore rather than in Vulkan makes it reusable by other backends — nice modularization.
  • Good: the external_data shallow-copy at _named_data_store.py:253-255 before mutation gives the atomic-on-collision guarantee the tests assert. Note the oversize check runs earlier (line 239) so that path is atomic too.
  • _parse_external_constants_max_data_bytes is a single-use helper, which CLAUDE.md discourages, but here it cleanly separates two distinct error messages (encoding vs. value) — I'd keep it.
  • Content-addressed tag naming (sha256 of the joined keys) is a solid choice for reproducibility and the collision guard is correct.

No blocking correctness bugs found. The main thing I'd want confirmed before merge is #1 — the pte_data-is-only-constants invariant at the call site.
· branch gh/JCNTH/194/head

@meta-clameta-claBot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 27, 2026
[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.

@meta-codesync
meta-codesyncBot merged commit 41760ec into gh/JCNTH/194/baseJul 29, 2026
188 of 197 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/194/head branch July 29, 2026 17:44
JCNTH added a commit that referenced this pull request Jul 29, 2026
This PR was created by the merge bot to help merge the original PR into
the main branch.
ghstack PR number: #21404 by
@JCNTH
^ Please use this as the source of truth for the PR details, comments,
and reviews
ghstack PR base:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/base
ghstack PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/main
Merge bot PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/orig
@diff-train-skip-merge
cc @SS-JIA@manuelcandales@digantdesai@cbilgin
---------
Co-authored-by: Julian Ng-Thow-Hing <juliannth@meta.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Julian Ng-Thow-Hing <107437036+JCNTH@users.noreply.github.com>
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-exportedmodule: vulkanIssues related to the Vulkan delegate and code under backends/vulkan/

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][vulkan] Add payload-bounded constant sharding - #21404

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

[executorch][vulkan] Add payload-bounded constant sharding#21404
meta-codesync[bot] merged 2 commits into
gh/JCNTH/194/basefrom
gh/JCNTH/194/head

Conversation

@JCNTH

@JCNTHJCNTH commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Enable deterministic payload-bounded Vulkan constant sharding

Problem
Large Vulkan exports can make inline PTE named data exceed downstream artifact limits, but NamedDataStore cannot externalize that data into bounded groups.

Solution

  • Before: Vulkan constants remain inline with no payload-bounded externalization option.
  • After: an opt-in uint64 raw-data cap externalizes constants after graph construction into deterministic shards.

Implementation

  • NamedDataStore.externalize_pte_data groups aliases by backing buffer, preserves existing tags, and rejects oversize buffers or collisions atomically.
  • parse_compile_options and parse_compile_spec transport external_constants_max_data_bytes as a strict positive uint64.
  • VulkanBackend.preprocess applies sharding only when the option is present.

Constraints
The default path is unchanged; buffers are not copied or split. The cap bounds unique raw buffer bytes per external tag, not final FlatTensor file size including alignment and header overhead.

Co-authored-with: Claude Code.
@exported-using-ghexport

Differential Revision: D113608555

Differential Revision: D113608555

cc @SS-JIA@manuelcandales@digantdesai@cbilgin

[ghstack-poisoned]
@pytorch-botpytorch-botBot added the module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/ label Jul 27, 2026
@pytorch-bot

pytorch-botBot commented Jul 27, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure, 6 Unrelated Failures

As of commit d0e8ae3 with merge base 2439920 (image):

NEW FAILURE - The following job has failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@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

Copy link
Copy Markdown
ContributorAuthor

@claude Thoroughly review for redundancy or areas of modularization or any high value changes

@claude

claudeBot commented Jul 27, 2026

Copy link
Copy Markdown

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


Review: payload-bounded Vulkan constant sharding

Thorough pass focused on redundancy, modularization, and high-value changes. Overall this is clean, well-tested (atomicity, idempotency, aliased-buffer-counted-once, order-independence are all covered), and the default path is genuinely untouched. Notes below, roughly in priority order.

1. externalize_pte_data externalizes allpte_data, but Vulkan labels it vulkan_constants

backends/vulkan/vulkan_preprocess.py:264-268 calls the method right after build_graph() and tags everything vulkan_constants. The method itself (_named_data_store.py:227-267) moves the entirepte_data map, not just constants. This is correct only if pte_data contains nothing but constants at that point. If any non-constant named data can land in pte_data before this call, it would be silently mislabeled as a constant shard. Worth a one-line assertion or comment documenting that invariant, since the method name (pte_data) and the Vulkan tag (constants) disagree.

2. The positive-uint64 check is validated in three places

  • parse_compile_optionsvulkan_partitioner.py:291-299 (rejects bool / non-int / <=0 / >= 1<<64)
  • _parse_external_constants_max_data_bytesvulkan_preprocess.py:93-99 (decode: length-8 + <=0)
  • externalize_pte_data_named_data_store.py:218-223 (bool / non-int / <=0)

Each with a slightly different message ("positive uint64" / "encoded as uint64" / "positive integer"), and each asserted by a separate test. Defense-in-depth across the encode → CompileSpec-bytes → decode → store boundaries is defensible, but the encode and decode validators live in the two Vulkan files and could share one helper. At minimum, the duplication is worth a comment explaining it's intentional boundary validation rather than an oversight. Fix this →

3. Greedy packing is key-sorted, not size-sorted

_named_data_store.py:234-251 does first-fit in sorted-key order. This is deterministic (good, and the reason for the sort) but not size-optimal — it can produce more shards than a first-fit-decreasing pass would. Given the PR explicitly documents "buffers are not copied or split" and prioritizes determinism, this is a reasonable trade-off, but a brief comment on why key-order over size-order (reproducibility) would help the next reader who is tempted to "optimize" it.

4. Test coupling to internal structure

test_vulkan_compile_options.py:_preprocess_named_data patches four internal symbols by name (unsafe_remove_auto_functionalized_pass, apply_passes, VkGraphBuilder, serialize_vulkan_graph). This verifies the wiring well, but it's tightly coupled to the current internals of preprocess and will break on unrelated refactors. Acceptable given a real graph would be heavyweight, but flagging as a maintenance cost.

Minor / positive

  • Good: placing externalize_pte_data on the generic NamedDataStore rather than in Vulkan makes it reusable by other backends — nice modularization.
  • Good: the external_data shallow-copy at _named_data_store.py:253-255 before mutation gives the atomic-on-collision guarantee the tests assert. Note the oversize check runs earlier (line 239) so that path is atomic too.
  • _parse_external_constants_max_data_bytes is a single-use helper, which CLAUDE.md discourages, but here it cleanly separates two distinct error messages (encoding vs. value) — I'd keep it.
  • Content-addressed tag naming (sha256 of the joined keys) is a solid choice for reproducibility and the collision guard is correct.

No blocking correctness bugs found. The main thing I'd want confirmed before merge is #1 — the pte_data-is-only-constants invariant at the call site.
· branch gh/JCNTH/194/head

@meta-clameta-claBot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 27, 2026
[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.

@meta-codesync
meta-codesyncBot merged commit 41760ec into gh/JCNTH/194/baseJul 29, 2026
188 of 197 checks passed
@meta-codesync
meta-codesyncBot deleted the gh/JCNTH/194/head branch July 29, 2026 17:44
JCNTH added a commit that referenced this pull request Jul 29, 2026
This PR was created by the merge bot to help merge the original PR into
the main branch.
ghstack PR number: #21404 by
@JCNTH
^ Please use this as the source of truth for the PR details, comments,
and reviews
ghstack PR base:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/base
ghstack PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/main
Merge bot PR head:
https://github.com/pytorch/executorch/tree/gh/JCNTH/194/orig
@diff-train-skip-merge
cc @SS-JIA@manuelcandales@digantdesai@cbilgin
---------
Co-authored-by: Julian Ng-Thow-Hing <juliannth@meta.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Julian Ng-Thow-Hing <107437036+JCNTH@users.noreply.github.com>
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-exportedmodule: vulkanIssues related to the Vulkan delegate and code under backends/vulkan/

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@JCNTH@SS-JIA