compass(memory): rename ModelTerms.declared_for_m1 to declared, and let the guard enforce milestone labels - #312
Conversation
The classmethod carried a milestone label in a public name. Its only callers are in tests/compass/, and it is the only path to the declared formulas those tests build ModelTerms from, so it is renamed, not deleted: every term it returns is Basis.DECLARED, from geometry and stated coefficients. The memory package's design-reference guard now matches a milestone label, as `M1` in prose and as an `_m1` identifier suffix, and the KEPT exemption is gone. The pattern no longer captures a group, so a failure prints the forms it found. The remaining M1 mentions in readings.py and graph_pool.py say what they meant instead. Closes #307 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
|
||
| @classmethod | ||
| def declared_for_m1( | ||
| def declared( |
There was a problem hiding this comment.
Blocking (principle 4): declared is already a query in this package, and this makes it a constructor too.
At 306b9b7e8, three classes in atom/compass/memory/ have a member called declared. Measured on node 18 with inspect.getattr_static against the merged tree:
| member | kind | returns |
|---|---|---|
readings.DeviceReadings.declared (this file, :219) |
property |
tuple[str, ...], the names of terms still on a declared basis |
terms.Reading.declared (terms.py:159) |
property |
tuple[str, ...], the same question for one reading |
readings.ModelTerms.declared (here) |
classmethod |
a new ModelTerms |
So x.declared asks "which terms are declared?" on two classes and means "build me declared terms" on the third. On a ModelTerms instance, .declared is a bound method: tuple(model_terms.declared) raises TypeError: 'method' object is not iterable, where the same expression on either sibling returns names.
The two meanings already sit side by side in the same files:
readings.py:224iteratesreading.declared, 113 lines below this definition.test_memory_readings.pycallsModelTerms.declared(...)nine times, and at:324asserts onreadings.peak_torch.declared.
The old name did not collide. This PR is a naming change, so the name is the deliverable.
Fix: use an alternate-constructor name that no query in the package uses. The brief's own example, from_declared_config, works, and so does declared_from_config. That is 11 mechanical sites: this definition, the prose at :42, nine in test_memory_readings.py and one in test_memory_compare.py. Neither name trips _TAGS.
Deleting the method stays wrong, for the reason the dev record gives: 10 test sites would each rebuild three Terms by hand. Keeping it with no production caller is justified. Only the name needs to change.
There was a problem hiding this comment.
Fixed in 40ae3faad. The method is renamed to ModelTerms.from_declared_config, the brief's own example, at all 11 sites: the definition, the module docstring at :42, nine calls in test_memory_readings.py and one in test_memory_compare.py.
DeviceReadings.declared (:219) and Reading.declared (terms.py:159) are untouched, so declared is now only ever the query.
AST check, tip 2565b5f57 against head 40ae3faad, with declared_for_m1 normalised to from_declared_config, run on node 18:
test_memory_readings.pyis identical.readings.pyandgraph_pool.pyare identical once docstrings are masked.
Re-run of mutA with the new name's line (26f1d89a3): `from_declared_config` at :42 becomes `declared_for_m1`. test_no_module_in_the_package_carries_a_design_reference[readings.py] fails, with readings.py carries design references: ['declared_for_m1']. "def declared_for_m1(" stays in the driven list.
| #: prints the forms it found. | ||
| _TAGS = re.compile( | ||
| r"\b[DTW]\d+(\.\d+)?\b|\bP\d+\.\d+\b|\b[A-Z]{2,4}-\d+\b" | ||
| r"\b[DTW]\d+(?:\.\d+)?\b|\bP\d+\.\d+\b|\b[A-Z]{2,4}-\d+\b|\bM\d+\b|\w*_m\d+\b" |
There was a problem hiding this comment.
Non-blocking (principle 6): near variants of the removed form escape. They pass silently, not loudly.
The named result holds for the exact forms: declared_for_m1 and M1 both fail the guard by name at the head (details in the review comment). One step away, they pass. Measured two ways at 306b9b7e8:
- A line-count-preserving edit to
readings.py, tree1bd8572e3. Three prose lines became`declared_for_m1_terms` fills,At m1, with fake models,andpast M1a or declared_for_M1. Result: 109 passed.test_no_module_in_the_package_carries_a_design_reference[readings.py]is green. - Probing this compiled pattern directly (
findall, node 18):
| form | caught? |
|---|---|
declared_for_m1, M1, M1's, M1.2, M01 |
yes |
declared_for_m1_terms, _m1_ |
no. \w*_m\d+\b needs a non-word character after the digits, and _ is a word character. |
declared_for_M1, declaredForM1, m1_declared |
no. The suffix form is lowercase-only and suffix-only, and \bM has no boundary after _. |
m1, at m1, M1a, Milestone1 |
no |
Nothing else in the suite notices either. git grep for any other design-reference guard over this package finds only this file. The whole CPU gate on tree 1bd8572e3 gave 5209 passed, 155 skipped, 3 xfailed, GATE_CPU_RC=0: the same count as the head on its own base.
Ask: a regex cannot close all of these without taking more of ATOM's own names with it (see the next comment). So I am not asking for a wider pattern. Instead, state the escapes beside the benign list, the way the deleted KEPT comment stated its absence. The test file's own standard is that "an absence nobody explained is the same defect one level up."
There was a problem hiding this comment.
Recorded in 40ae3faad; the pattern is not widened. test_what_the_pattern_matches_beside_its_targets_is_on_record asserts that declared_for_m1_terms, At m1, M1a and declared_for_M1 are not matched. Its docstring says why: a pattern wide enough to take them would take more of ATOM's names.
If someone later widens the pattern to catch one of them, this assertion fails. That forces them to update the record, instead of the record going stale.
| # share its shape, which is why it is not a bare letter-and-digit. | ||
| def test_the_widths_and_dtypes_that_share_the_shape_are_not_swept(): | ||
| """The pattern is not a bare letter-and-digit, and this is why.""" | ||
| for benign in ("TP1", "w1_base_bytes", "fp8", "int8", "bf16"): |
There was a problem hiding this comment.
Non-blocking (principle 8): this list claims to cover what shares the pattern's shape, but ATOM's own model names are not in it, and both new alternatives match them.
Measured over the merged tree (d45ecb5b5, tree 149965dee), with this file's compiled _TAGS:
- In the guard's actual scope (
atom/compass/memory/*.py, 6 files, no subdirectories): 0 hits. So there is nothing to fix today, and any future hit fails loudly. - In
atom/outsidecompass/:\bM\d+\bhits 140 times, allM3, across 28 files. Almost all of it is ATOM'sMiniMax-M3, for exampleatom/entrypoints/openai/chat_encoders.py:162.\w*_m\d+\bhits 50 times:minimax_m345 times andminimax_m25 times. These are ATOM's module and model-type names, for exampleatom/config.py:766andatom/model_engine/model_runner.py:124.
- Probes:
- Matched:
MiniMax-M1gives['M1'],MiniMax-M2.5gives['M2'],M128gives['M128'],tile_m128,block_m16,seq_len_m1,n_m1,fp8_m3andint4_m2. - Clear:
float8_e4m3fn,fp8_e5m2,BLOCK_M128andMI308X.
- Matched:
- A line-count-preserving edit to
readings.py(treee7a5729db) putMiniMax-M1,an M128 tileandseq_len_m1 = seq_len - 1into prose. It failstest_no_module_in_the_package_carries_a_design_reference[readings.py]with['M1', 'M128', 'seq_len_m1'].
This package sizes model memory, so a sentence or a key naming one of ATOM's supported models is plausible here. The next developer to write one will be refused, and the obvious reaction is to add an exemption back.
Ask: say it where the next reader looks. Either:
- add
MiniMax-M3andminimax_m3to this test as known collisions, asserting that they are matched, so the trade-off is on record; or - narrow the prose form with a lookbehind (
(?<![-\w])M\d+\bleavesMiniMax-M3alone), then add the model name to the benign loop.
Either way the docstring's claim then matches its assertions.
I measured the lookbehind on node 18:
MiniMax-M3andMiniMax-M1give[].at M1,(M1),M1-era,For M1, withandpast M1.give['M1'].- Its cost: a hyphenated
the-M1would escape.
There was a problem hiding this comment.
Recorded in 40ae3faad, using your first option. The same test now asserts that MiniMax-M3, minimax_m3, M128, tile_m128, seq_len_m1 and fp8_m3 are matched. Its docstring says that none occurs in this package, and that a hit is fixed by rewording the line, not by adding an exemption. I left the lookbehind out, so _TAGS is unchanged this round.
The record also holds the pattern. On mutE, which puts back the tip's _TAGS, this test fails with AssertionError: MiniMax-M3, alongside test_the_guard_catches_the_forms_that_were_actually_removed.
|
Review by an AI agent: the independent reviewer for #307, first cycle. Verdict: REQUEST_CHANGES at One finding blocks: the new name Everything else checks out, measured on node 18:
The two guard comments are non-blocking. I read the eight design principles ( 1. Rename vs delete (principle 3): rename is right; the name is not
2. The new guard patterns (principle 6): sound for the named forms; both edges measured
3. Named result: reproduced
mutE shows that the widening itself is pinned. A developer who reverts the pattern and keeps the prose clean is caught by the driven 4. AST claim: confirmedThe check parsed tip and head on node 18, normalised
5. ponytail-reviewOver the 27+/41- diff:
Nothing is left to delete. Lean already. Ship. Gate: the tree that will land
I also ran the gate once on the false-negative tree What the next cycle should check
|
…d the guard's reach `declared` collided with two queries in the same package: DeviceReadings.declared and Reading.declared are properties that return the names of terms still on a declared basis, so `x.declared` meant a query on two classes and a constructor on the third. The constructor is now `ModelTerms.from_declared_config`. The definition, the module docstring and all ten test call sites are updated. The benign-forms test now records the pattern's reach both ways. Some of ATOM's own names match it (MiniMax-M3, minimax_m3, M128, tile_m128, seq_len_m1, fp8_m3), and none occurs in this package. Some forms one step from the removed ones escape it (declared_for_m1_terms, At m1, M1a, declared_for_M1). The pattern itself is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Developer round 2, head Findings addressed:
Lines against the branch point
Named result, re-verified on node 18 (
The four PR files at the tip AST check, tip against head, with
Gate: the tree that will land.
What the next review should check: the delta |
| # share its shape, which is why it is not a bare letter-and-digit. | ||
| for benign in ("TP1", "w1_base_bytes", "fp8", "int8", "bf16"): | ||
| assert not _TAGS.search(benign), benign | ||
| for collision in ( |
There was a problem hiding this comment.
Non-blocking (principle 8): four of these six strings are not ATOM's names. They occur nowhere in the repo except this test.
The docstring says the pattern "does match some of ATOM's own names". Round 2's comment and the cycle-2 brief go further and call all six "ATOM's colliding names". I counted each one at 40ae3faad with git grep -wF -c over the whole tree:
| string | whole repo | atom/ |
|---|---|---|
MiniMax-M3 |
258 | 120 |
minimax_m3 |
47 | 44 |
M128 |
1 | 0 |
tile_m128 |
1 | 0 |
seq_len_m1 |
1 | 0 |
fp8_m3 |
1 | 0 |
For the last four, the single hit is this line. They are shapes that cycle 1's reviewer invented as probes, not names ATOM carries.
On node 18, the head's \bM\d+\b|\w*_m\d+\b finds exactly three distinct forms across atom/**/*.py (489 files): M3 140 times, minimax_m3 45 times and minimax_m2 5 times.
The asserts are correct either way; only the record's wording is off. One comment line would fix it, for example: "The first two are in ATOM (MiniMax-M3 120 times under atom/); the rest are shapes it could plausibly grow." Otherwise the next reader will believe the four probes were measured in ATOM.
|
Review by an AI agent: the independent reviewer for #307, second cycle. Verdict: APPROVE at Nothing blocks. Cycle 1's blocking finding is fixed. There is one non-blocking note, posted inline on Scope. This is a delta review of 1. The rename (principle 4): no collision left, and the name reads rightAt
2. The record test (principle 8): an honest pin of the pattern's current reachRuling: accepted. It pins a limitation, and it does not block a future fix. A future fix that changes the pattern's reach does fail this test, by design. I measured both directions with line-count-preserving edits to
Why I accept it:
The reason the docstring gives for not widening, measured. On node 18 I ran the head pattern and the mutG widening over the head tree:
So "would take more of ATOM's names with it" is true (+183 hits, mostly Non-blocking, inline on 3. Named result: reproduced on node 18Method:
Each red names the test that holds the property, with the exact form found. None of them is a line-drift guard: the null control shifts nothing and stays green, and mutA and mutB fail on the reinstated text itself. This matches the developer's round-2 table row for row. 4. AST claim: confirmedI parsed both sides on node 18. The only normalisation is the ModelTerms constructor's name: the method inside Tip
Delta
5. ponytail-reviewThis covers the 33+/15- delta. The rename adds no lines. The record test is three Lean already. Ship. Gate: the tree that will land
What the next cycle should checkNothing is outstanding. If the tip moves before landing, recompute |
1 participant
Closes #307
Dev record
Decision: rename
ModelTerms.declared_for_m1toModelTerms.from_declared_config. It is not deleted. Round 1 useddeclared. The first review showed that name collides withDeviceReadings.declaredandReading.declared, both properties that return the names of terms on a declared basis. Round 2 (40ae3faad) renamed it; the round-2 comment has the re-verified gates.Evidence:
93be90841,grep -rn declared_for_m1over the whole tree finds the definition and one prose mention inatom/compass/memory/readings.py, and 10 call sites, all intests/compass/: 9 intest_memory_readings.pyand 1 intest_memory_compare.py. Beyond those, only the guard test intest_memory_compare.pynames it, to assert the name exists. No design doc names it. At the new tipf84b4d778,git grep -l declared_for_m1outside those three files finds nothing.Terms by hand. The tests would then duplicate the formulas instead of exercising them. The brief says to rename in that case.from_declared_configsays what it does. It is an alternate constructor, and every term it returns isBasis.DECLARED, built from config geometry and stated coefficients. It leavesdeclaredto the two existing queries.What the guard needed, which the brief did not say.
_TAGSnever matchedM1ordeclared_for_m1.KEPTonly documented why, and the one test that readKEPTasserted the kept forms did not match. So taking them out ofKEPTalone would have enforced nothing. The pattern gains two forms:\bM\d+\bfor a milestone label in prose;\w*_m\d+\bfor one used as an identifier suffix.It still does not match a bare letter-and-digit, so
TP1,w1_base_bytesand the dtype widths stay clear. That check is kept as its own test. The group(\.\d+)?became non-capturing, and the assertion message now printsfound. Before,findallreturned the group's contents (''), so a failure could not name what it found. Now it can: see the named result.Changes:
readings.py: the method is renamed. ThreeM1mentions in prose now say what they meant:graph_pool.py: "Neither fires at M1 -- no drafter, one data-parallel rank --" becomes "Neither fires with no drafter and one data-parallel rank".test_memory_readings.py: 9 call sites renamed.test_memory_compare.py:KEPTand its comment deleted;_TAGSwidened, as above;removedlist;test_the_kept_forms_are_kept_on_purpose_and_stay_readablereplaced bytest_the_widths_and_dtypes_that_share_the_shape_are_not_swept. It keeps only the benign-forms half, because the other two assertions tested the exemption.Lines: 27 insertions and 41 deletions over 4 files.
readings.py5/5,graph_pool.py2/2).This is within the 20-40 line estimate.
Remaining
M1inatom/compass/memory/: none.grep -nE "\bM[0-9]+\b|_m[0-9]+\b" atom/compass/memory/*.pyis empty at the head. The guard now enforces that over the package glob.Gate 1: ATOM's CPU suite, unmodified, as a delta (round 1, head
306b9b7e8; round 2 is in the PR thread)This ran on node 18
xiaobizh_n18_cpu, using each tree's ownscripts/compass/gate_cpu.sh. Trees were staged bygit archiveplusdocker exec -i tar -xinto/tmp/i307gates/<label>/ATOM, with.compass-commitand.compass-changedstamps. The tarball md5 matched on both ends.atom.__file__resolved under each staged root, for example/tmp/i307gates/head/ATOM/atom/__init__.py.93be90841306b9b7e8f84b4d77809567002d, treecc28111b4Node-id delta, from junit XML: 5367 ids on each side against
93be90841, and 5372 ids on each side againstf84b4d778. The delta is identical on both:tests/compass/test_memory_compare.py::test_the_kept_forms_are_kept_on_purpose_and_stay_readable(passed)tests/compass/test_memory_compare.py::test_the_widths_and_dtypes_that_share_the_shape_are_not_swept(passed)git merge-tree --write-tree:93be90841 306b9b7e8gives64d2ec9c4e31dd888a536bf21a44c604f707aee7, which is306b9b7e8^{tree}.f84b4d778(compass(tools): pin the loose-refusal detector's BROAD and SHARP printouts, argv guard and recursion #304, compass(tests): hold the profiler-reply assertion to the roots REPLY_SURFACE names #302, which touch onlytests/compass/test_detect_loose_refusals.pyandtest_runner_rpc_surface.py). Against it:f84b4d778 306b9b7e8givescc28111b48d5a36656e535e46b1543962fbacd5b, with no conflict. That merged tree was gated above: green, with the same net-0 delta.ruff and black on the four changed files, at control and head:
RUFF_RC=0andBLACK_RC=0on both.Gate 2: CPU-only tests
tests/compass/test_memory_compare.pyandtests/compass/test_memory_readings.pypass 109 of 109 at the head. The renamed method is exercised by the 10 existing call sites, none of which this PR adds. The guard it widens runs over the package glob, which this PR does not add.Gate 3: named result
The mutation trees are commit objects made with
git commit-treeon the head. Each changes one file with its line count preserved. Each ranpytest tests/compass/test_memory_compare.py tests/compass/test_memory_readings.pyon node 18.306b9b7e8c7b71bbddreadings.py:42:`declared` fillsbecomes`declared_for_m1` fillstest_no_module_in_the_package_carries_a_design_reference[readings.py], withAssertionError: readings.py carries design references: ['declared_for_m1']074a1053areadings.py:122: "With fake models," becomes "For M1, with fake models,"...[readings.py], with['M1']366b9d7cagraph_pool.pyreverted to93be90841viagit show...[graph_pool.py], with['M1']80d9a2a23readings.pyreverted to93be90841viagit show, which is the full pre-fix module...[readings.py]with['declared_for_m1', 'M1', 'declared_for_m1', 'M1', 'M1'], plus 31AttributeErrors from callers ofdeclared215bbc73creadings.py:131: "on a real model" becomes "on any real model"93be90841declared_for_m1twice inreadings.py,M1ingraph_pool.py)test_no_module_in_the_package_carries_a_design_reference[readings.py]and[graph_pool.py]both pass. The forms are exempt there.On the tip, the lines mutA, mutB and mutC reinstate are the tip's own lines, so "the same edit on the tip" is the tip itself.
AST comparison, tip against head, with
declared_for_m1normalised todeclaredinFunctionDef.nameandAttribute.attr:tests/compass/test_memory_readings.py: identical.atom/compass/memory/readings.pyandgraph_pool.py: identical once docstrings are masked. The only non-name difference is the docstring prose that removesM1.tests/compass/test_memory_compare.py: differs only in the intended guard changes:_TAGSchanged;KEPTremoved;test_no_module_in_the_package_carries_a_design_referencechanged (the message);test_the_guard_catches_the_forms_that_were_actually_removedchanged (two strings added);test_the_kept_forms_...removed;test_the_widths_and_dtypes_...added.Gate 4
The coordinator dispatches the independent reviewer.
Left undone
Nothing from the brief.
🤖 Generated with Claude Code