Skip to content

compass(tests): size the tied head at a 4-byte element size as well as a 2-byte one - #290

Merged
jgong5 merged 1 commit into
feature/atomcompass_newfrom
compass/issue-284
Sep 23, 2026
Merged

jgong5 merged 1 commit into
feature/atomcompass_newfrom
compass/issue-284

Conversation

@jgong5

@jgong5 jgong5 commented Sep 23, 2026

Copy link
Copy Markdown
Owner

Closes #284

What changed

One test in tests/compass/test_memory_compare.py. It calls tied_lm_head_bytes on the tied Qwen3-0.6B geometry at dtype_bytes=4 and checks the exact byte count: 151936 * 1024 * 4 = 622,329,856. Before this, every call in the file passed a 2-byte element size. That meant a function which ignored dtype_bytes and always multiplied by 2 still passed the whole file.

Lines changed: production 0, tests +7 / -0.

Named result (node 18, xiaobizh_n18_cpu, each side on its own git archive copy, atom.__file__ checked under it)

T2 is return vocab * hidden * int(dtype_bytes) changed to return vocab * hidden * 2, one line for one line. The null control keeps the original line and adds a trailing comment to it. Both runs used pytest tests/compass/test_memory_compare.py.

tree unmutated T2 null
base 3c8bca938 53 passed 53 passed 53 passed
head dfed2cb3d 54 passed 1 failed, 53 passed 54 passed

The test that fails under T2: tests/compass/test_memory_compare.py::test_the_tied_head_scales_with_a_4_byte_element_size.

The base now has 53 tests rather than the 51 the brief measured, because #283 landed two parametrised cases in this file. I rebased onto that tip before gating.

Gate 1: ATOM CPU suite, unmodified, against a control measured on the same tip

I ran scripts/compass/gate_cpu.sh from each tree's own copy, with the .compass-commit and .compass-changed stamps written.

tree passed skipped xfailed failed GATE_CPU_RC
control 3c8bca938 5162 155 3 0 0
head dfed2cb3d 5163 155 3 0 0

The difference in collected node ids is exactly one, the added test above.

git merge-tree --write-tree 99fc506f9 dfed2cb3d gives b4b0fe4a977ece102ae94bfe5f1ad4e031fcc4eb (clean). 99fc506f9 is the integration tip when this PR was opened. It is 3c8bca938 plus #287, which changes only design markdown files and no code or tests. For that reason I did not restack or re-gate the branch onto it.

Gate 2

The new test is CPU-only, lives in tests/compass/, and runs under the device-reading autouse fixture already in the file. On this file, ruff check and ruff format --check are clean on both sides.

Dev record

Nothing surprising came up. #283 landed on the same file while this branch was being gated. The two changes touch separate regions, and the rebase was clean.

🤖 Generated with Claude Code

…s a 2-byte one

Every call to tied_lm_head_bytes in test_memory_compare.py passed a 2-byte
element size, so a function that ignored dtype_bytes and multiplied by 2
passed the whole file. Add one assertion on the tied 0.6B geometry at
dtype_bytes=4: 151936 * 1024 * 4 = 622,329,856 bytes.

Closes #284

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment on lines +320 to +321
assert nbytes == 151_936 * 1_024 * 4
assert nbytes == 622_329_856

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

shrink: (principle 3, non-blocking) L320-321 assert the same value twice, once as the product and once as its literal. One line carries both: assert nbytes == 151_936 * 1_024 * 4 == 622_329_856. The T2 red keeps its message under the chained form: pytest still prints assert 311164928 == ((151936 * 1024) * 4). net: -1 line.

@jgong5

jgong5 commented Sep 23, 2026

Copy link
Copy Markdown
Owner Author

Review 1: APPROVE at dfed2cb3d2e73f2849e6113641da44907f8c23a0

No blocking issues. There is one non-blocking shrink: note, posted inline on L320.

I read the eight design principles in atom/compass/design/README.md and AI_DEV_RULES.md at this head before reviewing.

Named result: reproduced (principle 8)

I ran this on node 18, container xiaobizh_n18_cpu. Each side ran on its own git archive copy at /tmp/pr290rev/<side>/ATOM, and atom.__file__ was asserted under that root and printed. Only one line changed per mutation, and the file's line count stayed the same (1139). The command was pytest tests/compass/test_memory_compare.py.

mutation base 3c8bca938 head dfed2cb3d
none 53 passed 54 passed
T2: return vocab * hidden * int(dtype_bytes) → return vocab * hidden * 2 53 passed 1 failed, 53 passed
null: the same line with a trailing # null control 53 passed 54 passed
M3 (reviewer's own): in atom/compass/backends/geometry.py, "float32": 4, → "float32": 2, 53 passed 54 passed

The T2 red at the head:

  • FAILED tests/compass/test_memory_compare.py::test_the_tied_head_scales_with_a_4_byte_element_size
  • assert 311164928 == ((151936 * 1024) * 4)

This matches the developer's table exactly.

M3 is the caller-side mechanism: the function reads dtype_bytes correctly, but a caller hands it the wrong size. It is out of scope for this PR.

  • tied_lm_head_bytes has no production caller. Its only callers are the six in this test file.
  • The new test passes the literal 4, so it pins the function's arithmetic and nothing upstream of it. That is exactly what compass(tests): tied_lm_head_bytes is never asked for a 4-byte element size #284 asked for.
  • M3 also survives all of tests/compass (pytest rc=0 at both head and base).
  • The float32 row of _DTYPE_BYTES is reached only through a string spelling. An HF config delivers a torch.dtype, and dtype_bytes answers that from .itemsize (see compass: delete the torch_dtype spelling no production config can deliver #285).
  • This is recorded here as an observation, not a finding. Whether any production path passes the string "float32" has not been established, and that should be settled before anyone files it.

Is the expected value derived or pinned? (principle 8)

It is pinned, and the pin is sourced.

  • 151_936 and 1_024 restate the qwen_0_6b() fixture. That fixture matches the published Qwen/Qwen3-0.6B config.json (snapshot c1899de2): vocab_size 151936, hidden_size 1024, tie_word_embeddings true.
  • 151936 × 1024 × 4 = 622,329,856. The head passing both asserts confirms the arithmetic.
  • The pin is deliberately not derived from config.vocab_size: that would feed the function's own inputs back into its expected value.

Design-doc references

The PR adds none. I checked over the whole of tests/compass/test_memory_compare.py at the head, not only the added lines. The only matches are the pre-existing strings at L1055-1069, which are data driven through the design-reference guard's regex, not citations.

ponytail-review

  • shrink: L320-321 assert one value twice. Use assert nbytes == 151_936 * 1_024 * 4 == 622_329_856. (principle 3, non-blocking, inline)

net: -1 lines possible.

Gate 1: the tree that will land

  • I re-read both refs before gating. The tip fork/feature/atomcompass_new = 99fc506f9, and the PR head = dfed2cb3d.
  • git merge-tree --write-tree 99fc506f9 dfed2cb3d = b4b0fe4a977ece102ae94bfe5f1ad4e031fcc4eb. The merge was clean, and it matches the developer's value.
  • 3c8bca938..99fc506f9 touches only eight atom/compass/design/*.md files.

I gated it once on node 18, using the tree's own scripts/compass/gate_cpu.sh, with the stamps .compass-commit=b4b0fe4a9 and .compass-changed=tests/compass/test_memory_compare.py:

tree passed skipped xfailed failed GATE_CPU_RC
merged b4b0fe4a9 5163 155 3 0 0
  • The gate printed commit: b4b0fe4a9 (stamp) and gpu: not required.
  • 5163 = the developer's control of 5162 at 3c8bca938, plus the one added test. The design-markdown delta adds no tests.

Gates 2 and 3

  • Gate 2: the new test is CPU-only and lives in tests/compass/. It exercises tied_lm_head_bytes, which this PR did not add. It runs under the file's autouse device-reading fixture.
  • Gate 3: the named result from the brief was reproduced as stated, T2 red by name at the head and green at the base, with the null control green on both sides.

Verdict: APPROVE, head dfed2cb3d2e73f2849e6113641da44907f8c23a0, tree to land b4b0fe4a977ece102ae94bfe5f1ad4e031fcc4eb. No blocking issues.

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