Skip to content

PR 03: attention planner ownership and backend support contract - #915

Draft
kalebbroo wants to merge 5 commits into
codex/attention-semantic-correctness-pr02from
codex/attention-contracts-pr03
Draft

kalebbroo wants to merge 5 commits into
codex/attention-semantic-correctness-pr02from
codex/attention-contracts-pr03

Conversation

@kalebbroo

@kalebbroo kalebbroo commented Nov 15, 2026 •

Copy link
Copy Markdown
Contributor

Core attention capability declarations, execution, plan descriptions and logging shared one source file. Separate those responsibilities so subsequent validation/planner work has clear ownership, while preserving public type names, namespaces, enum values and method bodies. The new support contract defines required common semantics, optional backend admission, precision expectations and current implementation gaps.

  • Start with a pure git mv commit, then extract the unchanged row/plan/logging/planner declarations.
  • Keep related capability flags beside the capability declaration. This follows the user's explicit attention-plan amendment to organize by ownership and drop “one public type per file” as a universal requirement; it is an intentional scoped grouping.
  • Preserve arithmetic, row order, predicates, fallback/lowering, public signatures and kernel locations/artifacts.
  • Add six planner contract tests for planning without launch, exclusion before admission, recoverable priority, exceptions without retry, unchanged output on unsupported, and absence of per-call allocation growth.
  • Define the mandatory/optional matrix without claiming it already conforms. A read-only probe reproduced the existing CPU dense fully-excluded-row NaN gap; it is recorded on PR 02: attention semantic correctness (in progress) #910 for semantic follow-up. No PR 02 test pin changed.

Validation:

  • net8.0/net10.0 builds pass. Normalized compiled Core signatures/constants/IL/local/exception/method-flag snapshots match the parent: 9,536 entries on net8.0 and 9,611 on net10.0. Managed DLL byte hashes can differ through metadata ordering.
  • 396 staged PTX/SPIR-V artifacts match byte for byte; no tracked artifact or backend implementation changed. Checkout newline differences were harmonized only for runtime comparison.
  • Full CPU lane across 13 projects: 3,225 reported passes, two known B0 failures, one skip. The failures remain API mesh-operation inventory and Windows MiniMax-H3 VAE path casing. All 178 Core CPU tests pass after the allocation-test review adjustment.
  • Full GPU categories ran sequentially on the RTX 3060: Core 19 passes; LLM 101/19 skips; Diffusion 18/2 skips; Engine 12/10 skips; no failures. Four guarded two/eight-GPU reported passes and missing real-weight cases remain unvalidated.
  • CUDA F32/F16 and Vulkan F32 mechanical smokes retain exact output hashes, routes, numerical settings and diagnostic observation arrays. Timing is mixed, including a slower F32 public cell; no statistical non-inferiority, speedup or primary-generation claim is made.
  • net8.0 is temporary SwarmUI compatibility pending its net10.0 migration. Both targets remain intact; no framework-specific abstraction was added.

Support contract · Detailed verification and timing

Stacked on #910 under the user's authorization to proceed while the GPU agent tests pinned PR 01/02 revisions. Those pins are unchanged. Keep draft pending parent evidence, matrix review and readiness concerns; this does not establish Bfix or approve later optimizations. Raw evidence remains under artifacts/attention-pr03 in this worktree.

@kalebbroo

Copy link
Copy Markdown
Contributor Author

@claude Review PR

@claude

claude Bot commented Nov 15, 2026 •

Copy link
Copy Markdown

Claude finished @kalebbroo's task in 1m 36s —— View job


PR 03 review: attention planner split

Verdict: The split is behavior-preserving. I found nothing that blocks merging, only one style deviation and two nits.

  • Gather context (AGENTS.md, CODE_STYLE.md)
  • Check that the move is content-identical
  • Review the new contract tests and run them
  • Post review

Is the move really a pure move?

Yes.

  • 560c250 is a 0/0 rename from AttentionPlanner.cs to AttentionKernelCapability.cs.
  • For 5b0a1c9, I took the base AttentionPlanner.cs (394 lines) and the five resulting files. I removed using, namespace and blank lines and sorted both sides. The line multisets are identical: no member, predicate, preference loop or Describe() format changed.
  • Each new file imports only what it uses:
    • System.Text is in AttentionPlanner.cs and AttentionPlan.cs.
    • Concurrent, Numerics and Logging are only in AttentionPlanLog.cs.
    • AttentionKernelCapability.cs imports only Tensors.
  • The namespace and the public type names are unchanged. No docs link to AttentionPlanner.cs, so the move breaks no links.

Tests

dotnet test tests/HartsyInference.Core.Tests -f net10.0 --filter AttentionPlanner passed 6 of 6 tests. I didn't run the full CPU regression or net8.0 here.

The six tests cover the contract boundaries that matter:

  • Planning never launches a kernel.
  • An excluded row never reaches its Admits. The test uses throwing delegates to prove this, which is a nice approach.
  • A recoverable decline (Run returns false) moves on to the next row.
  • A thrown launch failure is not retried on another row.
  • An unsupported call leaves the output buffer untouched.
  • Run doesn't allocate on the success path.

Findings

  1. Style: one public type per file. docs/CODE_STYLE.md:5 says "one public type per file". AttentionKernelCapability.cs still holds six public types:

    • AttentionDTypes (L7)
    • AttentionLayouts (L33)
    • AttentionMaskSupport (L50)
    • AttentionLowering (L73)
    • AttentionKernelCapability (L102)
    • AttentionExclusion (L218)

    The original file had the same problem, so this is inherited. But this PR exists to give each type its owning file, so it's the natural place to move the five enums into their own files. If you'd rather keep the diff small, say in the PR body that this is deferred. Fix this →

  2. Nit: the allocation test may be flaky. SuccessfulExecutionDoesNotAllocatePerCall uses GC.GetAllocatedBytesForCurrentThread and requires exact equality. That works here, but tiered JIT, OSR or a coverage or profiler hook could add bytes on the thread, and the test would then fail intermittently. Two options:

    • Use a small tolerance, or require that bytes do not grow with the iteration count (compare 1,000 runs against 10,000).
    • Mark the test as non-coverage.
  3. Nit: the title promises more than the diff. The title says "backend support contract", and the body mentions mandatory common subset and optional capability definitions. None of those are in the diff yet. The body does say they "will follow separately", but a reader of the title may assume they're included. Consider a scope line such as "this revision: file split + planner contract tests only" until those commits land.

Not covered by this review

  • Compiled-method and IL identity, and the full CPU regression on both TFMs. Your validation list covers these.
  • PR 01/02 gates. This PR doesn't touch them, and no kernel, PTX or SPIR-V files changed.

TL;DR: The move is clean and content-identical, and the new tests pass. The one real deviation is the five enums still sharing AttentionKernelCapability.cs, which breaks the one-public-type-per-file rule.

Copy link
Copy Markdown
Contributor Author

The user has authorized PR 03 in parallel with the GPU agent's pinned PR 01/02 validation. Production changes are limited to extracting unchanged capability/planning/diagnostic declarations. The public signatures and normalized compiled method bodies match the parent on both net8.0 and net10.0; six planner contract tests pass. The proposed common/optional support contract explicitly distinguishes current gaps from conformance. It does not repair arithmetic/admission or claim that all mandatory cases already pass. The additional CPU dense all-excluded-row gap is recorded on #910 without changing the active hardware-test pin.

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 15, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-15T07:24:31.310421Z 73bd606 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 9d5f764469

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown
Contributor Author

Addressed the review in e1d47ed and the updated PR description.

  1. Grouping is intentional under the user's explicit plan amendment: “Drop ‘one public type per file’ as a universal requirement; use it where it improves ownership.” The related capability flags remain with their capability declaration; planner execution, row contract, description and diagnostics now have their own owners. No namespace/public API changed. This scoped user instruction takes precedence over the generic style rule; it does not change the repository-wide policy.
  2. The allocation test now checks 1,000 versus 10,000 calls with a fixed one-KiB allowance for instrumentation noise, rather than requiring exact counter equality. A per-call allocation grows well beyond that fixed allowance. All 178 Core CPU tests pass after the adjustment; numerical tolerances are unchanged.
  3. The support contract is now included in docs/ATTENTION_SUPPORT.md and linked from the documentation map and mathematical contract. It separates mandatory common semantics, optional unsupported-before-write behavior, precision-specific gates and current source/runtime gaps. The title now matches the complete diff.

Full CPU results: 3,225 reported passes, the same two known B0 failures and one skip. Full sequential GPU categories: Core 19, LLM 101, Diffusion 18, Engine 12 reported passes, no failures; resource gaps remain unvalidated. Both library targets build and normalized compiled snapshots match the parent. Exact operation output/route/policy/diagnostic identity is checked on CUDA/Vulkan; mixed timing is preserved with no speed/non-inferiority claim. Parent GPU-agent pins remain unchanged, and the PR stays draft.

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: e1d47ed7de

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown
Contributor Author

Imported the parent evidence-tool correction by normal merge in 73bd606. No production source, kernel or arithmetic changed relative to e1d47ed. Core CPU regression passes 198 cases on each of net8.0 and net10.0, including the new fixture semantic rejection cases. The GPU agent's immutable P1/P2 production pins remain unchanged; the corrected H harness and invalidation rules are in the updated handoff. PR 03 remains draft pending the existing parent gates.

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 73bd606c44

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant