compass(ir): the cost graph data model (#54) - #60
Conversation
A forward pass recorded flat is one node per dispatched operator: 2,999 of them for a 27B model, priced at ~39 ms per step against a 32.7 ms modelled step, and valid only for the batch shape it was traced at. This adds the tree that replaces that list, and nothing that walks one. Four regions compose: an operator, a sequence, a repeat over a named index, and an overlap with a join policy. A graph pairs one region with a statement of where its record is valid; the statement's slot is defined here, the evaluator is not. Three things the types carry rather than ask a caller to remember: * a repeat body is typed as a region, so it may be a sequence or another repeat at any depth -- the nested, non-contiguous form a real layer stack needs; * a repeat's index binding is mandatory, and a body that rebinds the same name is refused, because the inner binding would shadow the outer one and the node reading it would be priced at the wrong instance; * a repeat's grouping evidence is mandatory, naming either the signature the instances share or the two prices that were compared, so collapsing a sequence is a claim somebody made rather than a default. Shapes stay symbolic and are never compared or converted here, since either installs a guard and would mutate the record being read. Attributes refuse to hold a copy of a per-step ambient reading -- a block table, a slot mapping, a maximum sequence length -- because a copy is whatever the tracing forward happened to see; those are reached through the node's context reference when a price is asked for. For the same reason no price key is offered: a key over shapes and static attributes alone moved 64 of 2,439 operator signatures between two equally valid allocations of one step. 70 new CPU-only tests in tests/compass/test_ir_data_model.py. Issue: #54 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| raise ValueError(f"a concrete dimension cannot be negative, got {dim}") | ||
| return dim | ||
| try: | ||
| hash(dim) |
There was a problem hiding this comment.
The hashability gate refuses every torch.SymInt. Measured on node 18, xiaobizh_n18_cpu, torch 2.10.0+rocm7.2.4.git3d3aa833, against this branch's tree:
backed symint: s52 SymInt is_symbolic: True
hash(s52) -> TypeError: unhashable type: non-nested SymInt
as_shape([s52, 4096]) -> TypeError: a symbolic dimension must be hashable,
because the node holding it is a value that is
compared and keyed; SymInt is not
unbacked u0 -> same, both ways
torch.SymInt.__hash__ raises for every non-nested symint, backed or unbacked (the else branch of its definition, with a comment saying constant symints could be supported and are not). So the one class the capture side is most likely to hand this module — a FakeTensorMode trace's dynamic dimension — is refused at as_dim, by name, with this module's own message.
The test does not catch it because _Tripwire defines __hash__ returning 17 and reprs itself as s52: the stand-in differs from the real thing in exactly the one property this line gates on.
I am not asking for SymInt support — "what carries a symbol stays the capture side's choice" is a reasonable position. I am asking that the choice be stated and pinned, because IR-2 and IR-3 are being written against this interface now and both will key on nodes:
- Say in the docstring that a raw
torch.SymIntis not an acceptable dimension and the capture side must wrap it in a hashable value object (the symbol's string form is the obvious one:str(s52) == 's52', hashable, and comparison-free). - Add a second stand-in to
test_a_symbolic_dimension_is_never_compared_or_convertedwhose__hash__raises the waySymInt's does, asserting the refusal — so the constraint is a test rather than an assumption.
Taken with the __eq__ finding on nodes.py, this is one decision, not two: what is the canonical, hashable, comparison-free form of a symbolic dimension. It belongs in this PR.
| raise NotImplementedError | ||
|
|
||
|
|
||
| @dataclass(frozen=True, slots=True) |
There was a problem hiding this comment.
The no-guard property survives hashing and does not survive equality — and equality is the operation this module advertises. Same tripwire as the test (__eq__/__lt__/__int__/__bool__ all raise), but exercising the paths the test does not. Measured in both containers, identical results:
OK hash(op) repr(op), str(op) OK
GUARD!! op == op2 (two distinct symbolic objects, same symbol)
GUARD!! shapes tuple eq
GUARD!! Seq(...) == Seq(...) GUARD!! Repeat(...) == Repeat(...)
GUARD!! {op, op2} (hash collision falls through to __eq__)
GUARD!! op2 in {op: 1} GUARD!! op in [op2]
@dataclass(frozen=True) generates __eq__ over the field tuple, so comparing two nodes compares their dimensions elementwise. Tuple comparison short-circuits on identity, which is why op == op (literally the same dimension object) is clean and why nothing in the suite notices; two separate accesses of a traced shape, or two layer bodies, are not the same object.
That matters because it is precisely the advertised use. The module docstring says node equality "is structural identity -- useful for deciding that two blocks are interchangeable", and D19's detection algorithm is run-length encoding over a canonical block signature, i.e. deciding that two blocks are equal. So the module hands IR-3 an operation that, on a symbolic shape, installs the guard this design forbids — and on a real torch.SymInt a comparison between two different symbols is exactly what shape_env records.
Direction, one decision shared with the shapes.py hash finding: compare and hash a dimension through a canonical guard-free key rather than through the object. Whatever the capture side wraps a symbol in must define __eq__ as symbol-identity, or this module must derive the key itself.
For the PR body: the claim "goes through shape construction, node construction and hashing untouched" is accurate and I reproduced it. It is the sentence after it — that no other path touches a symbolic dimension — that does not hold.
| grouping is always a claim somebody made about a specific comparison. | ||
| """ | ||
|
|
||
| __slots__ = () |
There was a problem hiding this comment.
The mandatory evidence can be satisfied by the marker base itself. GroupingEvidence is a plain class, so GroupingEvidence() instantiates and passes the isinstance check in Repeat.__post_init__:
ACCEPTED GroupingEvidence() as evidence: 'GroupingEvidence'
Rule 3's claim — "you cannot pass a string or a flag" — is true, and I verified the string/flag/None/float refusals. But the cheapest satisfying value is not IdenticalStructure(...), it is the contentless base, which names no comparison at all and costs one more character than the flag the rule refuses. abc.ABC plus one @abstractmethod (or a __new__ that refuses the base) closes it and keeps everything else as it is.
Same defect, same fix, in graph.py: Applicability() instantiates and Graph(applicability=Applicability(), region=...) is accepted. That object is the "graph claiming to apply everywhere" that the module docstring says the field exists to prevent.
Two smaller ones while you are here: EqualPrice(-1.0, -1.0) and EqualPrice(0.0, 0.0) are both accepted — equal, finite, and not durations.
On the split itself, since I am the last reader before IR-3 depends on it: the split is right. A single evidence type carrying two prices would force a detector that has no prices to fabricate a pair, which is the accident the rule exists to prevent, installed by the rule. Keep IdenticalStructure and EqualPrice.
What the split buys is narrower than the PR body implies, and the handoff should say so: a mandatory, unverified evidence field buys a named claim in the artifact and a slot for IR-4's verdict. It does not buy truth, and IdenticalStructure("x") is accepted on any body whatsoever. It becomes ceremony unless IR-4 recomputes the signature from repeat.body and rejects a mismatch. That obligation should be written into #57's brief now, not discovered there.
| non-shape leaves no guard behind and the key is what covers it. | ||
| """ | ||
|
|
||
| __slots__ = () |
There was a problem hiding this comment.
Applicability() instantiates, so this refusal has a hole. abc.ABC only blocks instantiation when there is an abstract method, and there is none here — deliberately, since the signature belongs to #55. Measured:
ACCEPTED Applicability() as graph applicability: 'Applicability'
The docstring above says None "is not an empty predicate; it is a graph claiming to apply everywhere, which is the failure this field exists to prevent". A bare Applicability() is that graph, spelled differently and accepted. One @abc.abstractmethod — even def __str__ or a no-argument describe() that #55 has to implement anyway — restores the property without deciding #55's signature. Same shape of fix as GroupingEvidence in nodes.py.
| #: Readings that decide an operator's cost and change every step. They are | ||
| #: reached through the context reference when a price is asked for; a copy taken | ||
| #: at trace time is a value from whichever forward did the tracing. | ||
| AMBIENT_READINGS = frozenset( |
There was a problem hiding this comment.
The refusal list is the right kind of rule with an incomplete list. I checked it against what ATOM actually reads off the forward context. Counting attribute reads of attn_metadata.* across the tree:
slot_mapping 49 | block_tables 38 | max_seqlen_q 35 | context_lens 32 |
cu_seqlens_q 30 | max_seqlen_k 24 | cu_seqlens_k 11 <- all refused
seq_lens 18 | query_start_loc 17 | num_actual_tokens 15 | max_query_len 15 |
kv_indptr 18 | kv_indices 15 | kv_last_page_lens 14 | block_table_tensor 13 |
max_seq_len 12 | num_reqs 11 | batch_id_per_token 10 <- all accepted
Measured, on this branch:
ACCEPTED attr named block_table_tensor ACCEPTED attr named seq_lens
ACCEPTED attr named query_start_loc ACCEPTED attr named max_seq_len
ACCEPTED attr named MAX_SEQLEN_Q ACCEPTED attr named attn_metadata.max_seqlen_q
Two of those are one-character-off spellings of names on the list — block_table_tensor for block_tables, max_seq_len for max_seqlen_k — and they are live names in this repository, not hypotheticals. GDNAttentionMetadata adds nums_dict, batch_ptr, token_chunk_offset_ptr, which D20 names explicitly; they are recomputed rather than recorded, so excluding them is defensible, but query_start_loc — what they are recomputed from — is per-step and is accepted.
A tensor value does not save you either: torch.Tensor hashes by identity, so the hashability check below passes a recorded tensor straight through.
So: right instinct, list not yet earned. Either derive the names from the metadata classes (GDNAttentionMetadata and the backends' builders), or refuse on the shape of the value — a per-request sequence is what a per-step reading looks like, whatever it is called — and keep the name list as a fast path. At minimum add the accepted names above and normalise case, and say in the docstring that the list is a blacklist and therefore a floor, not a guarantee.
The rest of D20 checks out and I am not asking for changes to it: context_ref optional is right (a tuned GEMM's cost really is a function of its shapes, and requiring a key would refuse that node), and offering no price key on a node is right and carries its measurement (64 of 2,439 signatures moved; 32.667 ms vs 28.360 ms).
| def __post_init__(self) -> None: | ||
| if not isinstance(self.key, str): | ||
| raise TypeError(f"a context key is a str, got {type(self.key).__name__}") | ||
| if not self.key.strip(): |
There was a problem hiding this comment.
ContextRef accepts keys it can never bind, and fails at a distance. Measured:
ACCEPTED ContextRef('model.layers.{')
REFUSED ...its .index_names -> ValueError: Single '{' encountered in format string
ACCEPTED ContextRef('{}').index_names -> ()
REFUSED ContextRef('{}').bind() -> IndexError: Replacement index 0 out of range
ACCEPTED ContextRef('{0}').index_names -> ('0',)
REFUSED ContextRef('{0}').bind(**{'0': 1}) -> IndexError
ACCEPTED ContextRef('a.{x.y}').index_names -> ('x.y',)
Three things are wrong with that, all cheap here and expensive in IR-2/IR-3's pricing loop:
- An unbalanced key constructs fine and blows up later, in a property, with a stdlib message that does not name the key or say what to do — the one refusal in these four files that carries no reason of its own.
{0}reports an index name of"0", which noIndexBindingcan ever carry:IndexBinding.__post_init__requiresisidentifier(). A key that cannot be bound by any legal binding is accepted as a key.{x.y}likewise.
__post_init__ already validates the key is a non-empty str; one call to index_names there plus an isidentifier() check per placeholder, mirroring IndexBinding's own rule, refuses all three where the mistake is made.
Related, and for IR-2/IR-3 rather than for this line: nothing relates a body's placeholders to the bindings above it. Both of these are accepted —
Op(context_ref=ContextRef('model.layers.{layer}')) with no repeat at all
Repeat(index=IndexBinding('period'), body=<that op>) binds 'period', body names 'layer'
The PR body says context_ref "is what makes rule 2 operational rather than decorative"; the half that is enforced is that an inner repeat cannot shadow an outer name, and the half that is not is that a name a node reads is bound by something. A free_indices property composed bottom-up exactly as bound_indices is — no traversal, same shape, same cost — would let Graph refuse a region with an unbound placeholder. Adding it now is an hour; adding it after IR-2 and IR-3 consume this interface is a change to their call sites.
| "is not, and the node holding it is a value" | ||
| ) from None | ||
| pairs.append((key, value)) | ||
| return tuple(sorted(pairs, key=lambda pair: pair[0])) |
There was a problem hiding this comment.
Sorting does not make the recording order irrelevant when a name repeats. The docstring's stated invariant is that two recordings of one operator compare equal whatever order the tracer visited the arguments in. Measured:
ACCEPTED duplicate attr key: (('dtype', 'bf16'), ('dtype', 'fp8'))
same pairs, other order, == first? False
sorted is stable and keys only, so two recordings of the same operator that disagree about which dtype came first are two different nodes — and a duplicate name is a defect in its own right, since nothing says which value governs. Three lines: refuse a name that appears twice, naming it.
One more from the same function: _as_attrs refuses a str for the whole attrs argument but not for an item, so attrs=['ab'] is accepted and unpacks into ('a', 'b'). And in_shapes={(4, 8): 'x'} silently iterates the dict's keys and yields ((4, 8),).
| "a repeat carries what was compared before the grouping was " | ||
| f"taken, got {type(self.evidence).__name__}" | ||
| ) | ||
| if self.index.name in self.body.bound_indices: |
There was a problem hiding this comment.
Checked and correct: the bottom-up composition holds at depth. This is the check the brief asked me to make independently, so recording what I ran rather than just agreeing. Built a four-level tree mixing all four region types —
Seq( Op, Repeat(index='mid', body=Seq( Op, Par( Repeat(index='layer', body=Op), Op ))), Op )
bound_indices -> ['layer', 'mid']
— and then tried to rebind each name from outside:
REFUSED outer rebinds 'layer' (bound three levels down, inside a Par, inside a Repeat)
REFUSED outer rebinds 'mid'
ACCEPTED outer binds a fresh name -> ['layer', 'mid', 'outer']
REFUSED outer rebinds a name bound by two same-named siblings in a Seq
ACCEPTED two Par branches each binding 'layer' (disjoint scopes)
The shadowed-name-three-levels-down case is refused, and refusal does not depend on where in the tree the inner binding sits. Sibling scopes are disjoint and are allowed to reuse a name, which is right, and an outer repeat over such siblings is then refused, which is conservative in the safe direction.
Two notes, neither a change request. A Region subclass that returns an empty bound_indices defeats this check, but it has to override a base property that raises NotImplementedError to do it — out of scope here. And bound_indices is recomputed on each access and walks the subtree, so a detector that calls it per candidate is quadratic in the tree; the 5x replay gate makes that worth knowing before IR-3 writes its inner loop.
Review record — IR-1, round 1, agent-authoredVerdict: REQUEST CHANGES. Two interface defects, both cheap today and not cheap once #55/#56 are written against this package; four small refusal holes; everything else I checked stands, including the three gate numbers and the effort arithmetic, which I reproduced rather than read. Eight findings are inline. This comment carries the verdict, the re-measurements, and what the next two tasks should watch. The two blocking findingsThey are one decision, not two: what the canonical, hashable, comparison-free form of a symbolic dimension is. That decision belongs in this PR, because both of the consumers now in flight will key on and compare nodes.
Did the no-guard property survive?Partly, and the PR body's own sentence is the accurate one. Construction, node construction, Rulings the brief asked for
ScopeClean. Nothing here walks a tree to detect or price. Gates and effort, re-measuredIndependent staging:
Effort, by the same AST protocol, reproduces to the line: nodes 186, shapes 27, graph 14, What #55, #56 and #57 should watch
Accepted with reservation
Not checked
|
…oles The data model could not hold a real symbolic size. On torch 2.10 a SymInt cannot be hashed at all -- `hash()` raises `TypeError: unhashable type: non-nested SymInt`, measured here for a backed symbol and an unbacked one alike -- so the hashability gate refused every one of them with the package's own message. The suite missed it because the stand-in defined `__hash__`, differing from the real type in exactly the property the gate read. The same defect had a second face. A node has to be hashable and comparable, because deciding that two blocks are interchangeable is how repetition gets found, and the generated equality compares dimensions: two nodes, two sequences, `in` over a list, a set, a dict key all reach one. Tuple comparison short-circuits on identity, so comparing a node with itself stayed clean and nothing noticed. Both are one decision. A size that is not known yet is now canonicalised on the way in, into `SymDim`, which holds the text the expression renders to and nothing else. Rendering is the one safe operation -- measured, `str()` and `repr()` of `s26`, of `2*s26 + 1` and of `u0` left the guard list untouched -- so no live symbolic object is ever stored, and no comparison can reach one. The stand-in now refuses to hash and raises on every comparison, and a test against a real SymInt runs the same path. Also refused, each of which was constructible before: * a bare `GroupingEvidence()` and a bare `Applicability()`, both cheaper to write than the flag the mandatory field exists to refuse. Both types are now abstract on one method that states what they compared or where they hold -- not on deciding a step, which is still the guard evaluator's to shape. * twelve more per-step metadata names in the attribute denylist, two of them one character from names already there, and any tensor-shaped value whatever it is called. The name list is documented as a tripwire rather than a proof; the tensor check is what carries the guarantee. * a context key naming an index nothing binds. Every region now reports `free_indices` the way it reports `bound_indices`, composed from immediate children, and a graph refuses a region with any. * a duplicate attribute name, which made the sorted-order invariant a coin toss; an attribute that is not a name/value pair; a context key that does not parse, or whose placeholder is automatically numbered or dotted, both of which no index binding could ever carry; and a negative duration. Issue: #54 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Revision pushed as Both blocking findings were one defect and have one fix. Reproduced on node
The stand-in now refuses to hash and raises on every comparison and conversion, The four refusal holes:
On the caveat you attached to the evidence split: the constraint that Your caveat on my metric is right and is now in the body: Gates re-run side by side on node 18, container
4190 − 4084 = 106, exactly the tests in the new file. Production is now 295 AST |
| "context reference when a price is asked for." | ||
| ) | ||
| try: | ||
| hash(value) |
There was a problem hiding this comment.
BLOCKING — a live symbolic object still reaches a node, through this line, and this line is what specialises it. The fix closed the dimension path. attrs is the neighbouring field and it is untouched, and hash(value) here is exactly the gate that round 1 proved does not work on a symbolic value. Measured on node 18, xiaobizh_n18_cpu, torch 2.10.0+rocm7.2.4, against this tree, with a real ShapeEnv:
hash(SymBool 's26 > 100') -> 0 guards 0->1 GUARD: s26 <= 100
Op(attrs=(('causal', s27 > 100),)) -> (('causal', s27 > 100),)
guards 1->2 GUARD: s27 <= 100
...type of the stored value -> SymBool (live object, held on the node)
Op(attrs=(('scale', 1.5*ToFloat(s26)),)) -> (('scale', 1.5*ToFloat(s26)),)
guards 2->3 GUARD: Eq(1.5*ToFloat(s26), 25.5)
Op(attrs=(('n', (s26 > 100, 4)),)) -> accepted, SymBool inside a tuple
Op(attrs=(('n', frozenset({s26 > 100}),)) -> accepted
Op(attrs=(('n', SymInt s26),)) -> TypeError: attribute 'n' must be hashable (the loud half)
Op(attrs=(('c', u0 > 1),)) unbacked -> GuardOnDataDependentSymNode, uncaught
Three things follow.
SymInt.__hash__raises;SymBool.__hash__silently specialises.torch/__init__.pydoes, in its own comment,# Force specialization->hash(builtins.bool(self)). One gate, opposite failures, and the silent one is the dangerous one.SymFloatis worse still: the guard isEq(1.5*ToFloat(s26), 25.5), a full specialisation of the symbol to its trace-time hint — the artifact now says the graph is valid only at T=17.- The node stores the live object. So the dataclass
__eq__onOp,SeqandRepeatreaches aSymBoolthroughattrson every comparison — the same reachability round-1 finding 2 was about, on the field the canonicalisation did not cover. The PR body, the commit message andshapes.py's docstring all say "no live symbolic object is ever stored, so no generated__eq__can reach one; every comparison and hash on a node touches only ints and strings." As measured that sentence is not true. - Nothing in the suite looks.
_Unhashableis used for dimensions only; no test passes a symbolic value as an attribute. That is the round-1 miss repeated one field over: the stand-in differs from the real type in exactly the property the code reads.
causal, is_causal, scale, softmax_scale, window_size are what an attention op's non-tensor arguments are called, and under FakeTensorMode a flag derived from a shape comparison is a SymBool, not a bool.
Direction, and it is the decision this PR already made, applied once more. The lesson the project recorded from round 1 is to guard a symbolic boundary with a positive type allowlist, never with "does it hash". as_dim does that. _as_attrs does not. Either canonicalise an attribute value the same way a dimension is — render it and store the text — or refuse a value whose type is not on a stated allowlist of what an attribute may be, with this module's own named reason instead of torch's GuardOnDataDependentSymNode. Three lines here; a change to #55/#56/#57 call sites after they are written against attrs being part of node identity.
Whichever you pick, pin it with a test that builds a real SymBool under a live ShapeEnv and asserts len(shape_env.guards) does not move — the same shape as test_a_real_symbolic_size_survives_as_a_node, which is the test that makes this package's central claim checkable rather than assumed.
| return dim | ||
| if isinstance(dim, SymDim): | ||
| return dim | ||
| return SymDim.of(dim) |
There was a problem hiding this comment.
as_dim has no positive test for a symbolic size, and now that the text is the whole of the identity that is load-bearing. This line renders anything that is not an int and not one of the six refused types. Measured on this tree:
as_dim(np.int64(4096)) -> SymDim(text='4096') ; == 4096 -> False
as_dim(np.int32(4096)) -> SymDim(text='4096')
Op(in_shapes=[[np.int64(4096), 8]]) == Op(in_shapes=[[4096, 8]]) -> False
as_dim(torch.tensor(7)) -> SymDim(text='tensor(7)')
as_dim([1, 2]) -> SymDim(text='[1, 2]')
as_dim(range(4)) -> SymDim(text='range(0, 4)')
as_dim(o1), as_dim(o2) -> SymDim('<object object at 0x7fc1530b1eb0>')
SymDim('<object object at 0x7fc1530b1e70>') ; equal -> False
Three consequences, in order of how much they cost:
- A numpy integer becomes a symbolic dimension that never equals the same concrete size. One recording of a shape via
np.int64and another viaintare two different nodes for one operator — and node equality is the operationRepeatdetection is built out of. This is the quiet one: nothing raises, the tree just fails to group. - An object with the default
__repr__renders to its address, so every recording of it is a unique dimension. A node holding one can never equal another node, in any run. - A tensor is refused as an attribute value, by name, with a reason — and accepted as a dimension.
_as_attrsalready has the check (hasattr(value, "shape") and hasattr(value, "dtype"));as_dimdoes not consult it.
The module cannot import torch, and should not. It does not have to: two library-free checks cover all three. Refuse a value whose type(dim).__str__ is object.__str__ — "it renders to its address" is a refusal with a real reason — and either refuse or fold back a rendering that is a plain decimal integer, since a concrete size that arrived by this door is a concrete size. Reusing the two-attribute tensor check costs one line and closes 3.
This is the flip side of the ruling in the docstring: once the text is the identity, "what renders" is the whole contract, and right now the contract accepts anything with a __str__.
| constructible and testable on a machine with no device runtime. It needs only | ||
| that an open size can render itself, and the text it renders to is its identity | ||
| from then on. Rendering has to be the tracer's one convention: two spellings of | ||
| one expression are two dimensions here. |
There was a problem hiding this comment.
The ruling is stated here, which is the right place — and it is only half the rule. "Rendering has to be the tracer's one convention: two spellings of one expression are two dimensions here" is exactly the sentence a capture author needs, and they will meet it: it sits in the module whose as_dim/as_shape they have to call. I checked 04's D18/D19 and the convention is not there, so this docstring is the only statement of it — which I think is fine, because the doc predates the decision and the code is where it binds.
The half that is missing is that a symbol's text is scoped to the ShapeEnv that produced it. Measured, two entirely independent captures on this tree:
capture A (17 tokens) backed s26 unbacked u0
capture B (512 tokens) backed s26 unbacked u0
as_dim(A.backed) == as_dim(B.backed) -> True
as_dim(A.unbacked) == as_dim(B.unbacked) -> True
Op(from capture A) == Op(from capture B) -> True hash equal -> True
s26 and u0 are counters in a ShapeEnv, not names of sizes. Two captures of two different models, at two different hints, both say s26. So node equality is sound within one graph and is a false positive across two — and an artifact holding several graphs is the normal case, which is what Graph exists for.
This is not a regression: before the fix the comparison installed a guard or raised, so it was not usable across captures either. It is a property the chosen canonical form now has, and it is the kind of thing that reads as a detector bug six weeks from now. Two sentences here would settle it: a SymDim's text identifies a size only within the capture that produced it, and comparing nodes from two graphs is comparing two namespaces.
For #56 specifically: D19's detector runs bottom-up within one trace, so it is safe as specified. The unsafe call is any cross-graph dedup or price cache keyed on a node — and hash(op) is right there and now works, which makes it an attractive nuisance in a way it was not before.
| #: reached through the context reference when a price is asked for; a copy taken | ||
| #: at trace time is a value from whichever forward did the tracing. A tripwire | ||
| #: for the names seen so far, not a proof -- see the module docstring. | ||
| AMBIENT_READINGS = frozenset( |
There was a problem hiding this comment.
The list grew to 20 and all twelve names from round 1 are in — but the sentence in the module docstring that the tensor check "carries the guarantee" is the wrong way round for most of these names. Measured on this tree:
attr seq_lens=512 -> REFUSED attr max_seq_len=512 -> REFUSED
attr query_start_loc=512 -> REFUSED attr block_table_tensor=512 -> REFUSED
attr MAX_SEQLEN_Q=512 -> ACCEPTED attr attn_metadata.max_seqlen_q=512 -> ACCEPTED
attr seq_lens_tensor=512 -> ACCEPTED attr num_scheduled_tokens=512 -> ACCEPTED
attr query_lens=512 -> ACCEPTED attr max_num_blocks_per_req=512 -> ACCEPTED
attr num_computed_tokens=512 -> ACCEPTED
The reason the tensor rule does not rescue these is that six of the twenty names on this list are int in ATOM, not tensors:
atom/model_ops/attentions/aiter_attention.py:790 max_seqlen_q: int
atom/model_ops/attentions/aiter_attention.py:791 max_seqlen_k: int
atom/model_ops/attention_mha.py:1234 max_query_len: int
atom/model_ops/attention_mha.py:1235 max_seq_len: int
atom/model_engine/model_runner.py:457 max_seqlen_q: int
max_seqlen_q, max_seqlen_k, max_query_len, max_seq_len, num_reqs and num_actual_tokens are scalars, so hasattr(value, "shape") does nothing for them and the case-sensitive exact-name list is the only defence. And it is the maximum sequence length from a warm-up dummy — a scalar — that the docstring names as the failure that priced attention at 163.6 us against a true 23.0 us. The one reading the list exists because of is precisely the one the general rule cannot see.
Two small things, both of which round 1 asked for and neither of which is the mechanism change:
key.strip().lower()before the membership test.MAX_SEQLEN_Qis one line of ATOM away.- Four live names in this repository are absent:
num_scheduled_tokens(93 uses),query_lens(48),max_num_blocks_per_req(19),num_computed_tokens(17).
And the docstring sentence at the top of nodes.py — "refuses such a snapshot two ways, and only the first is a guarantee: no value that looks like a tensor" — should say which readings each way actually covers. Right now it tells the next person the list is the weak half, and for the scalar readings it is the only half.
| tensor inputs has two of them, and flattening the two into one list would | ||
| lose which dimension belonged to which operand. | ||
| """ | ||
| if isinstance(shapes, (str, bytes)) or not isinstance(shapes, Iterable): |
There was a problem hiding this comment.
Round 1's trailing remark on this function is unfixed and unmentioned. The half about attrs was fixed — attrs=['ab'], [('dtype',)], [('a',1,2)], [3] are all refused now, with named reasons and a test. The other half was about shapes and still stands:
Op(..., in_shapes={(4, 8): 'x'}, out_shapes=[]).in_shapes -> ((4, 8),)
A dict passed where one shape per operand was meant iterates its keys, so the values vanish and the node is built from half the argument without a word. It is a caller mistake rather than a soundness hole, and it is the same three lines as the attrs fix: a mapping is not a sequence of shapes, refuse it by name.
I am flagging it mainly because it was raised in round 1 and neither fixed nor answered. I did not treat that as the review loop's stop — see the verdict comment for why — but a third round should.
|
|
||
| __slots__ = () | ||
|
|
||
| @abc.abstractmethod |
There was a problem hiding this comment.
Checked and closed: Applicability() is genuinely no longer instantiable. IR-2's reviewer measured Graph(applicability=Applicability(), ...) accepted on IR-2's base, so I re-ran it against this head rather than reading the diff:
Applicability() -> TypeError: Can't instantiate abstract class
Applicability without an implementation for
abstract method 'describe'
Graph(Applicability(), op) -> same, refused at the argument
class Sub(Applicability): pass; Sub() -> TypeError (a subclass that does not implement it)
class Ok(Applicability): describe -> str -> accepted, Graph built
GroupingEvidence() -> TypeError, same shape
class BadEv(GroupingEvidence): pass; BadEv() -> TypeError
Both bases are closed, and closed the way that does not decide #55's signature — describe() is a record-or-refusal line, not a deciding method. That is the right call and I would keep it exactly as it is.
free_indices also holds at depth, which is the other half of this file's new refusal. I built a four-level tree mixing all four region types and tried it both ways:
Seq( Op, Par( Repeat(index='layer', body=Op(ctx='model.layers.{layer}')), Op(ctx='x.{other}') ) )
free_indices -> ['other']
Graph(Ok(), that) -> REFUSED: nothing binds ['other'], named by a
context key in this graph
Repeat(index='other', body=that).free_indices -> [] (binding it above clears it)
Graph(Ok(), Op(ctx='{layer}')) -> REFUSED, names 'layer'
Graph(Ok(), Repeat(index='period', body=Op(ctx='{layer}')))
-> REFUSED, names 'layer' (the shadow case)
Graph(Ok(), Repeat(index='layer', body=Op(ctx='{layer}')))
-> accepted
Composed from immediate children, no traversal, subtraction at the binding repeat, and the refusal names the index. That is the property round 1 asked for and it is worth saying it landed as specified.
Review record — IR-1, round 2, agent-authoredVerdict: REQUEST CHANGES. One blocking finding, and it is the mirror image of round 1's. Both round-1 blockers are genuinely fixed — I reproduced the fix against real Six findings are inline. This comment carries the verdict, the re-measurements, the loop-stop call, and what #55/#56/#57 should watch. No round-1 finding survived unfixed, so I have not applied The central fix: verified, against the real thing
Every path round 1 named as broken — The stand-in is also fixed the way it needed to be: Past it: where a live symbolic object can still get inThat is what I spent the round on. Every field, every default, every
Everything closes on a positive The Two more that follow from the same decision, both inline, neither blocking:
The ruling you asked for: is the rendering convention stated where a capture author will find it?Yes, and it is the right place. What is missing is the second half of the rule, the The four refusal holes: all closed, re-measured
I tested the general rule rather than the list, as asked, and it does not carry the readings the list exists for. A tensor value is refused whatever it is called — I confirmed that with a real The loop stop, and why I did not apply
|
| commit | passed | skipped | xfailed | failed | GATE_CPU_RC |
|
|---|---|---|---|---|---|---|
| control (current integration) | 5e767898f |
4152 | 149 | 3 | 0 | 0 |
| branch | eb9c41bc1 |
4190 | 149 | 3 | 0 | 0 |
| merged (branch into control) | 562ef949e |
4258 | 149 | 3 | 0 | 0 |
- Merged − control = +106, exactly the new file:
pytest tests/compass/test_ir_data_model.pyalone reports106 passed in 0.40s. The merged tree is green. - Control − the PR's stated control = 4152 − 4084 = +68, exactly W1.5's tests. The arithmetic in the PR body is right; the baseline under it is stale.
4190 − 4084 = 106happens to give the same delta, but the branch does not contain W1.5, so it is not the number that predicts the merge. - The merge is clean, no conflicts. W1.5's package-wide glob (
test_the_package_imports_nothing_from_the_engine) is parametrised overatom/compass/backends/**only, and IR-1 addsatom/compass/ir/, so the two do not intersect — but that is a fact I had to check, not one the branch gate could have told anyone. - Not the one-in-four flake: that moves one test between passed and skipped, and
skippedis 149 on all three runs. ruff checkandruff format --checkclean on all five files. No design-doc references in code or in emitted data — I grepped the five files forD19/D20/T6/P0.x/W1.5/principle/doc filenames, including the refusal strings, and found none.
Effort, and the caveat you asked me to rule on
The AST count reproduces to the line, and so does the raise count, which tells me we are running the same protocol:
| nodes | shapes | graph | __init__ |
production | tests | raise |
|
|---|---|---|---|---|---|---|---|
| author | 232 | 39 | 20 | 4 | 295 | 267 | 49 |
| this review | 232 | 39 | 20 | 4 | 295 | 267 | 49 |
Does the ast.unparse caveat materially change 1.48x? Yes, at one end.
- Two ratios, not one: 1.48x against 200 and 1.97x against 150. The estimate is a range, and against its lower end this is one line short of the 2x halt. The PR body quotes only the first.
- The undercount is not marginal. Counting physical source lines excluding blanks, comments and docstrings, production is 456 — nodes 338, shapes 49, graph 28,
__init__41. That is 2.28x against 200 and 3.04x against 150, both past the halt. - The gap is 161 lines and it is where the caveat says it is:
__init__.pyreads 4 for 41, and the 49 multi-line refusal messages each unparse to one line.
I am not calling a halt on it, because 295 is what the project's protocol measures and round 1 accepted the same protocol. I am saying the metric now differs from a like-for-like line count by 1.5x, so "inside the threshold" is a statement about the protocol and not about the code. #55, #56 and #57 will all report against the same estimate; it is cheaper to settle which number the threshold applies to now than to discover it when one of them reads 1.9x by one measure and 3.1x by the other.
Accepted with reservation
EqualPrice(0.0, 0.0)is accepted — equal, finite, not negative. Fine, and round 1 said so.IndexBinding(start=0, step=-1)still yields(0, -1, -2), formatting tomodel.layers.-1.Repeathas both the binding and the count somin(index_values()) >= 0is available to it. Not asking, again.ContextRef('{layer!r}')and('{layer:>5}')are accepted and report('layer',);bindthen applies the conversion or the padding to the index value. Harmless, and arguably useful.'{{layer}}'reports no index and binds to the literal{layer}, which a secondContextRefwould then read as a placeholder. Neither is worth a line of code.- A
Regionsubclass that returns an emptybound_indicesorfree_indicesdefeats both checks, but has to override a base property that raisesNotImplementedErrorto do it. Out of scope, as in round 1. SymExpr = AnyandShape = tuplestill carry no checking. With the text now being the whole of the identity, the cost of that has moved fromhash/__eq__(fixed) toas_dim's open fallback (finding 2). Same choice, next bill.
What #55, #56 and #57 should watch
- IR-3 — the nested Repeat detector over canonical block signatures #56 (repeat detector). Node equality and
hash(op)now work and are guard-free on a symbolic shape — that is the thing this PR bought you. Two limits: they are only meaningful within one capture (s26is aShapeEnvcounter), and they are not guard-free if the blocking finding lands unfixed and any op carries aSymBoolattribute. Also,bound_indicesand nowfree_indicesboth recompute and walk the subtree on every access; calling either per candidate is quadratic, and the 5x replay gate is real. - IR-4 — the provably-free grouping rule and its flat-versus-grouped validation #57 (grouping validator).
IdenticalStructure's docstring now states that the signature must be recomputable fromrepeat.body. That is a precondition you can lean on and an obligation you have to discharge: recompute it and reject a mismatch, or the mandatory evidence field is ceremony. Nothing today ties evidence to the repeat it sits on. - IR-2 — the applicability predicate: discrete key, symbolic domain, and refusal that names its guard #55 (applicability).
Applicabilityis abstract ondescribe()and nothing else, so the deciding signature is still yours to shape — the hole is closed without your decision being made for you. D19's rule that a domain is evaluated withsympy.subsorsize_hint()and neverint()or a Python comparison is the same rule this PR just learned the hard way on a neighbouring field. - All three. If you add a field that can hold a value from the tracer, gate it with a positive type allowlist, not with "does it hash" and not with "does it compare". That is the whole lesson of both rounds, and
attrsis the field where it has not been applied yet.
Not checked
- Nothing on a GPU. The gate reported
gpu: not requiredfrom the stamp on all three trees. - No end-to-end capture of a real ATOM forward under
FakeTensorMode. My symbolic objects come frommode.from_tensor(..., static_shapes=False)andcreate_unbacked_symint(), which is what the new test does. So "does ATOM's capture actually hand aSymBooltoattrs" is argued from the types and from the names ATOM's attention operators carry, not observed. That is the one assumption behind the blocking finding, and a compass: the cost backend interface, its breakdown and its ladder (W1.5, #23) #58 author with a real trace could overturn it — though not, I think, the argument that the gate is the wrong kind. str.format_map, unicode and\N{...}placeholder forms beyond the cases listed.- Whether any of this changes under a torch version other than
2.10.0+rocm7.2.4.git3d3aa833.
…apture A live symbolic value still reached a node, one field over from where the last one did. `attrs` asked whether a value could be hashed, which is the same question `as_dim` had just stopped asking, and the question is not safe: measured on torch 2.10.0+rocm7.2.4, hashing refuses a SymInt loudly, accepts a SymBool and a SymFloat silently -- specialising both, and pinning the SymFloat to its trace-time hint with a guard nobody asked for -- and raises GuardOnDataDependentSymNode on an unbacked one. No test had ever passed a symbolic value as an attribute. Every check in the package is now an isinstance and nothing else. An attribute value is a number, a string, an enum member, None, or a tuple of those. All three symbolic types answer False to every one of those and to bool, int, float, str, bytes and tuple besides, with the guard list unmoved -- measured, and now asserted per field against real SymInt, SymBool, SymFloat and unbacked objects under a live shape environment. A rendering is read against one capture's symbol table, so a rendered size now carries its capture. Two unrelated traces both produce `s26` and both produce `u0`; comparing a body recorded in one against a body recorded in another is exactly what finding repetition and validating a grouping do, and on the text alone those comparisons silently agreed. Nothing is rendered on a caller's behalf any more. `as_dim` takes a concrete int or an already-rendered size, because the fallback rendered whatever it was given: a width from another library rendered to its digits and became a symbol that compares unequal to the int it is, turning one shape into two nodes, and an object with no rendering of its own rendered to its address. Both are now refused by the rendering itself, so the explicit call is guarded too. Also: * a mapping is refused as a shape and as a list of shapes. Iterating one yields its keys, so what was read would not be what was written. * the ambient-reading names are matched case-insensitively and regenerated from a census of what ATOM reads off its attention metadata, which the module docstring carries as a command. Forty-five names, up from twenty. The list is not a tripwire after all: six of the readings arrive as plain ints, which no type rule can tell from a width that really is the operator's, so the names carry that half on their own and the module says so. Issue: #54 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 3 pushed as The blocking finding is fixed by deleting the question, not by widening it.
So every check in the package is now an You named this as the third appearance of one trap, and that framing is what The five non-blocking items, all closed:
Interface change, and it breaks what is stacked. Gates. Integration moved twice during this round; measured on node 18 in
4338 − 4178 = +160, exactly the tests in the new file (70 → 106 → 160 across |
| if isinstance(regions, Region): | ||
| raise TypeError(f"{what} holds several regions; pass them in a sequence") | ||
| try: | ||
| items = tuple(regions) |
There was a problem hiding this comment.
The mapping defect is closed in shapes.py and open here — in the two types where order is the meaning. tuple(regions) accepts any iterable, so Seq and Par take a mapping and a set. Measured on node 18, xiaobizh_n18_cpu, torch 2.10.0+rocm7.2.4, against this tree:
Seq({a, b, c}).items -> ('a', 'c', 'b') a set; the order is the hash order
Seq({a: 1, b: 2}).items -> ('a', 'b') a mapping; the values vanish
Par({a, b}, MAX).branches -> ('a', 'b') same
Seq(iter([a, b])).items -> ('a', 'b') a generator, consumed
Seq's own docstring is "Regions one after another, in the order they run". A set has no order, so the first line above is a recorded sequence whose order came from id(). A mapping is round 2's in_shapes={(4, 8): 'x'} finding one field over: the node is built from half the argument, without a word.
The fix is already written, three lines away, in the sibling module: shapes._refuse_unless_ordered refuses a Mapping by name and then anything that is not a Sequence. _as_regions wants the same two checks. That also removes the generator case, which is harmless here only because the tuple is taken immediately.
Not blocking, and I am classifying it exactly as round 2 classified its twin: a caller mistake, not a soundness hole. I am flagging it because it is the same defect, the fix for it exists in this diff, and the reason round 2 raised its version at all was that a finding neither fixed nor answered is indistinguishable from an unread one.
| #: is. Checked with `isinstance` and nothing else -- a live symbolic value | ||
| #: answers False to every one of these and is refused without being asked a | ||
| #: question it would answer by installing a guard. | ||
| ATTR_VALUE_TYPES = (bool, int, float, str, bytes, enum.Enum) |
There was a problem hiding this comment.
The rule is right and it is the strongest form available — and it is not quite the guarantee the docstring states. I went looking for a fourth face of the trap, since the first three were each one field over from the last. I found one, and it is admitted by this line.
Measured on node 18, xiaobizh_n18_cpu, torch 2.10.0+rocm7.2.4, against this tree, with a live ShapeEnv:
Mode = enum.Enum("Mode", {"CAUSAL": <live SymBool s26 > 100>})
enum class creation guards 0 -> 1 (installed by enum's own value map)
Op(attrs=(("mode", Mode.CAUSAL),)) ACCEPTED guards 1 -> 1
stored value type Mode
stored_value.value type SymBool <- a live symbolic object, on the node
hash(op); op == op2 ok guards 1 -> 1
So the module docstring's "a tensor cannot be stored, and neither can a live symbolic value" is false as written: one can, wrapped in an enum member. What is true, and is the part that matters, is that no operation this package performs reaches it — Enum.__hash__ hashes the member's name and Enum.__eq__ is identity, so node hashing and node equality never touch .value, and the guard above was installed by enum at class creation, before Compass saw anything. The package is behaving; the sentence overstates.
I am not asking for a fix, and I would refuse one that added a value-type check on enum members — that would be asking the value a question again. An enum class is built at import time, before any trace exists, so a capture cannot produce this. Two sentences in the docstring, when something else is next edited here: the allowlist closes over what a value is, and an enum member's own value is outside it.
Second, smaller, same line: isinstance admits subclasses, so "checked with isinstance and nothing else" does not by itself mean "never asked a question". An int subclass that raises from __eq__/__hash__ is accepted as an attribute value and as a dimension:
class Guarded(int):
__eq__ = __hash__ = <raises>
Op(attrs=(("g", Guarded(4)),)) ACCEPTED, stored as Guarded
as_shape([Guarded(4)]) ACCEPTED -> (4,) <- and any later hash(op) raises from it
No such type exists in torch today — SymInt is not an int subclass, and np.int64 is not one either and is correctly refused both ways. np.float64 and np.str_ are float/str subclasses and are accepted, benignly. So this is a bound on the claim, not a live path: the allowlist is a guarantee about this torch and the types it names, which is what the measurement in the docstring actually supports.
| metadata: both are `int`. The names are the ones ATOM reads off its attention | ||
| metadata, counted from the tree with | ||
|
|
||
| grep -rhoE '\b(attn_metadata|metadata|md|gdn_metadata|attn_md)\.[a-z_][a-z0-9_]*' \ |
There was a problem hiding this comment.
I re-ran this census, as the round-2 finding asked, and it does not produce this list. Verbatim, against the merged tree:
grep -rhoE '\b(attn_metadata|metadata|md|gdn_metadata|attn_md)\.[a-z_][a-z0-9_]*' \
atom/ --include=*.py | sed 's/.*\.//' | sort -u | wc -l
-> 174 distinct names
174, not 45, and no frequency cut explains the membership: dtype_q (16 hits) is out while batch_ptr (1) is in; cu_seqlen_ks (9) is in and its sibling cu_seqlen_ke (9) is out. Going the other way, three of the 45 do not appear in this command's output at all — block_table_tensor, num_reqs, query_start_loc. Those three are the names round 1 supplied by hand, and the reason the census cannot see them is the receiver alternation:
receivers of .query_start_loc / .num_reqs / .block_table_tensor in atom/
common_attn_metadata 41 input_batch 2 cad 1 ...
\b fails before attn_metadata inside common_attn_metadata, so all 122 uses of that receiver are invisible to the command, as are forward_metadata (79), sparse_meta (44), kda_metadata (39), chunk_meta (26), decode_md (20), plugin_md (18), prefill_md, live_metadata, indexer_meta, main_metadata, swa_metadata and several more.
Widening the receiver to every *metadata / *meta / *md spelling:
grep -rhoE '\b[A-Za-z_][A-Za-z0-9_]*(_metadata|_meta|_md|metadata|attn_md)\.[a-z_][a-z0-9_]*' \
atom/ --include=*.py | sed 's/.*\.//' | sort -u | wc -l
-> 232 distinct names; 39 of the 45 appear in it
Near-misses left off, in the same class round 1 named — a one-character-off spelling of a name that is here:
block_table (block_tables is in) query_start_loc_cpu (query_start_loc is in)
slot_mapping_owned (slot_mapping is in) batch_id_per_token_cpu
state_slot_in (state_slot_out is in) cu_seqlen_ke (cu_seqlen_ks is in)
kv_indptr_swa / kv_indices_swa block_tables_per_token
So: 45 is not complete, and I would not have expected it to be — this docstring says so twice and the round-3 note concedes it. My finding is narrower and it is the one I would fix: the command recorded here is not the command that produced the number. A successor who follows the instruction "regenerate it the same way when the metadata grows" gets 174 names, loses three of the current 45, and has no stated rule for which to keep. That is a claim without its measurement.
Cheapest correction, and not a code change: widen the receiver alternation in the docstring to the pattern above, and say the list is a hand-curated subset of its output rather than its output. Non-blocking — the type rule carries every tensor-valued reading whatever it is called, and this half was never offered as a proof.
| """The repeat indices this key is written in terms of, in order.""" | ||
| return tuple(dict.fromkeys(self._fields())) | ||
|
|
||
| def bind(self, **values: int) -> "ContextRef": |
There was a problem hiding this comment.
bind is annotated **values: int and checks nothing, so a live symbolic value renders straight into a context key. Measured against this tree, same session as the other numbers:
ContextRef('model.layers.{layer}').bind(layer=<live SymInt s26>)
-> ContextRef(key='model.layers.s26') guards unmoved
ContextRef('model.layers.{layer}').bind(layer='oops')
-> ContextRef(key='model.layers.oops')
ContextRef('{layer:>5}').bind(layer=<live SymInt s26>)
-> TypeError: unsupported format string passed to SymInt.__format__
The first line is the one worth a fix: a key that names a layer s26 is a handle to a module that does not exist, produced silently at the moment the price is asked for. It is not a guard leak — str.format on a SymInt with no spec moves the guard list not at all, which I checked — so it is a wrong answer rather than an unsound one.
The third line is this type's own stated contract failing: the class docstring says a bad placeholder is "refused here rather than at the point somebody tries to bind them, and a key that does not parse is refused with a reason rather than raising out of the formatter later". That is exactly what happens — an unnamed TypeError out of the formatter, at bind time.
One isinstance(value, int) per value in bind, with the same shape of message as IndexBinding's, closes both. IndexBinding.value_at already guarantees the values a Repeat supplies are int, so this only bites a caller binding by hand — which is why it is not blocking.
| """ | ||
|
|
||
| text: str | ||
| scope: str |
There was a problem hiding this comment.
The ruling asked for: the scope is load-bearing, not stored. I checked that it actually participates in comparison and hashing rather than sitting in the record, because a field that is carried but not compared would leave the false equal exactly where it was. Measured against this tree:
dataclasses.fields(SymDim) -> ['text', 'scope'] (frozen, so both are in __eq__/__hash__)
SymDim('s26','cap-a') == SymDim('s26','cap-b') -> False
hash(...) equal -> False
len({SymDim('s26','cap-a'), SymDim('s26','cap-b')}) -> 2
Op(in_shapes=((A,8),)) == Op(in_shapes=((B,8),)) -> False
SymDim('s26','cap-a') == SymDim('s26','cap-a') -> True
So the cross-capture false equal is closed at the level the readers use — node equality, node hashing and set membership — and not only at SymDim. Making it mandatory rather than a documented limit is the right call for the reason the round-3 note gives: this error direction produces a match where there is none, and a match is the answer a repeat detector and a grouping validator both act on without looking further.
One property to be aware of rather than change: __str__ returns text alone, so the scope is invisible in a rendering and in any log line built from one. That is correct for a rendering, and repr carries both. Nothing validates that a scope string names a real capture either — it cannot, here, and the capture side owns that convention the same way it owns the rendering.
| unbacked = shape_env.create_unbacked_symint() | ||
| assert [type(v).__name__ for v in (causal, scale)] == ["SymBool", "SymFloat"] | ||
|
|
||
| for label, value in ( |
There was a problem hiding this comment.
This test is the most valuable thing in the diff, and it covers two fields, not "every field". The loop runs the five live values against _op(attrs=...); below it, in_shapes gets the SymInt alone. stream_id, Repeat.count, IndexBinding.start/step, EqualPrice's two durations, ContextRef.key and SymDim.text are not exercised by it.
I re-measured all of them by hand against this tree and every one refuses, on an isinstance, with the guard list unmoved — so the claim is true, it is just not this test that holds it. Given that the trap has now appeared three times, each a field along from the last, the loop body is three lines from also being the thing that would catch the fourth: iterate the constructors the way it already iterates the values.
The PR body's "against every field" should read "against attrs and in_shapes" until it does. Cosmetic; noted because the test's whole value is that it is checkable rather than asserted.
Review record — IR-1, round 3, agent-authoredVerdict: APPROVE. Reviewed at head I spent this round on the two things the brief said to weigh: whether every round-2 item was fixed or answered, and whether there is a fourth face of the trap. Answers below, then the measurements. Every round-2 item: fixed or answeredRound 2 raised one blocking finding, five non-blocking, and two trailing sub-remarks it explicitly said a third round should halt on. Eight of eight are closed. I re-measured each rather than reading the diff.
Nothing was silently dropped. The two trailing items are the ones I checked first, because round 2 named them precisely so a third round could halt on them, and a rejected finding and an unread one look identical from outside. Both were carried out. The blocking fix: verified against the real typesThe fix is the right kind. The three refusals that asked "can this be hashed?" now ask what the value is, and the reason that is a different class of rule — rather than a bigger list — is in the docstring where a future author will hit it. Measured on node 18, The round-2 table inverts cleanly: every cell that read "accepted, stored live, guard installed" now reads refused at zero guard movement. The fourth face of the trap: found, and it is not a live pathThe brief was right that this is where to look, and the honest answer is that the strongest remaining path is admitted by So a live symbolic value does reach a field, wrapped in an enum member, and the docstring's "neither can a live symbolic value" is false as written. What is also true, and is why I am not asking for a fix: The second half of the same inline is a bound rather than a hole: Three more I chased and found clean: a tuple nested two and three deep, The one new finding I would actually fix
This is round 2's The census: I re-ran it, and 45 is not completeInline, in full. Short form: the command in the docstring yields 174 distinct names, not 45, and three of the 45 do not appear in its output at all ( The incompleteness is conceded twice in the docstring and I hold nobody to it. The finding is the other thing: the recorded command is not the command that produced the number, so "regenerate it the same way" hands a successor 174 names and no rule for which to keep. Non-blocking, and the correction is to the docstring, not the code. The breaking change at cycle 3: right call, and the bill is already paidAsked to rule, so: agree with the refusal of
So the breaking change at cycle 3 costs one already-completed restack. Deferring it would have cost a format migration. Gates: I re-derived the control, and it had moved againThe PR's stated control Protocol: three
Effort: the metric reproduces to the line, and one ratio is missingI reverse-engineered the protocol rather than assuming it — strip docstrings,
Both tables land on the line, and One number the body does not give. The estimate is a range, 150–200. 312 is 1.56x against 200 — the body's figure — and 2.08x against 150. Round 2 computed 1.97x against the lower bound and called it "one line short of the 2x halt"; it crossed this round. I am not calling a halt on it: I read "overruns its estimate by more than ~2x" against the upper bound of a range estimate, which puts the threshold at 400. But #55, #56 and #57 all report against this same estimate and at least one of them will read under 2x by one end and over by the other, so the record should say which end the threshold is measured from. That is a question for the owner about the protocol, not a finding against this PR. The loop stopCycle 3 is the last the loop allows, so the call has to be explicit rather than implied. APPROVE converges, and I am not applying Accepted with reservation
What #55, #56 and #57 must change on restack
Not checked
|
…efused merge leaves it untouched (#434) The base-update exception said "first patching the PR's base via REST", which put the patch ahead of the merge. When the merge then refused, the held PR kept a patched base and no merge: on #61 its diff would grow from 3 files / +751 to 7 files / +2208, 1457 lines of it from the landed #60. Tie the patch to the push instead. A refused merge commits nothing and pushes nothing, so it patches nothing. To keep the line count, drop the `--diff-merges=remerge` pointer after "one delta review of the resolutions": the delta-review rule already names that flag. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…an PR (#437) The need human label stopped every commit on a held PR, with one exception: gh stack link. So a held branch could not take the base update the branch-update rule calls for, and it drifted further from the tip with every landing. After #410 and #417, the held clock chain conflicted with the tip in 8 files, and the owner had to authorize each update by hand (2026-09-24). This adds a second exception. An agent may merge as the branch-update rule calls for, and nothing else: - For an unlinked child whose parent landed, the agent patches the PR's base via REST just before the push. A refused merge therefore never leaves a patched base. - The merge keeps every change from both sides. Where it cannot, the agent commits nothing and names the conflict hunk in a PR comment. This also rules out -s ours and -X ours. - A PR comment lists each resolved file. - The label allows one delta review of the resolutions. - Only the owner removes the label. The review measured why the patch timing matters on #61. Merging without patching the base shows 122 files. Patching and then refusing the merge shows 7 files / +2208, of which 1457 lines are the landed #60. Merging and patching keeps #61's own 3 files. No test or script reads the file. Gate (node 18, CPU tier, merged tree on 1dc8939): 5280 passed, 155 skipped, 3 xfailed, GATE_CPU_RC=0. Closes #434 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Base update: the parent #60 landed as 68ef4f3, and this unlinked child still sat on compass/ir-1-data-model. Merges the tip c92e4c1. The branch was built on an older #60 head (eb9c41b), so the four #60 files it carries hit add/add conflicts against the landed copies. Each was resolved as a three-way merge with eb9c41b as the base, which keeps both sides: - atom/compass/ir/__init__.py: the tip's version plus this branch's three applicability lines (import and two __all__ entries). - atom/compass/ir/nodes.py, atom/compass/ir/shapes.py, tests/compass/test_ir_data_model.py: the tip's version; this branch made no change to them past eb9c41b. git diff c92e4c1 <this merge> equals git diff eb9c41b d1e828b (3 files, +751) apart from hunk line numbers. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Base update: this branch conflicts with its base. It carries the #60 commits up to c25edb4, which landed squashed as 68ef4f3, so three of the #60 files hit add/add conflicts against the landed copies. Merges the tip c92e4c1. Each conflict was resolved as a three-way merge with c25edb4 as the base, which keeps both sides: - atom/compass/ir/__init__.py: this branch's version (the repeats import and __all__ entries); the tip made no change to it past c25edb4. - atom/compass/ir/nodes.py, tests/compass/test_ir_data_model.py: the tip's version (#83, #194, #279, #347, #360); this branch made no change to them past c25edb4. git diff c92e4c1 <this merge> equals git diff c25edb4 b18dc20 (3 files) exactly. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 participant
Dev record for #54 — the Cost IR's data model.
atom/compass/ir/holds the four region types, the node, the join policy, andthe graph that pairs a region with a statement of where its record is valid.
Nothing here walks a tree, prices one, builds one from a trace, or decides
whether a repeat was safe to take.
Three rounds.
a2b7e91d4original;eb9c41bc1the canonical symbolicdimension and four refusal holes;
c25edb4afchecks every value by type ratherthan by what it can do, and scopes a rendered size to its capture.
The interface the next three tasks consume
Op(Region)name: str,kind: NodeKind,in_shapes: tuple[Shape, ...],out_shapes: tuple[Shape, ...],attrs: tuple[tuple[str, Any], ...] = (),stream_id: int = 0,context_ref: ContextRef | None = NoneSeq(Region)items: tuple[Region, ...]Repeat(Region)body: Region,count: int,index: IndexBinding,evidence: GroupingEvidencecount >= 2;index_values() -> tuple[int, ...]Par(Region)branches: tuple[Region, ...],join: JoinPolicyGraphapplicability: Applicability,region: RegionRegion; refuses a region with free indicesSymDimtext: str,scope: str;SymDim.of(expr, scope)NodeKindCAPTURED,OPAQUE_LEAF,DECLAREDJoinPolicyMAX,RESOURCE_BOUND,EXCLUSIVEContextRefkey: str;index_names;bind(**values){name}placeholdersIndexBindingname: str,start: int = 0,step: int = 1;value_at(position)start + step * iGroupingEvidencedescribe() -> str;IdenticalStructure(signature),EqualPrice(flat_seconds, grouped_seconds)ATTR_VALUE_TYPES(bool, int, float, str, bytes, enum.Enum)Noneand tuples of thoseEvery region exposes
bound_indicesandfree_indices, each composed fromimmediate children rather than by searching the tree.
Repeatsubtracts its ownname from the free set;
Graphrefuses a region with any left, naming them.Applicabilityis abstract ondescribe()only — how a step is offered and howa refusal names its guard stay #55's to shape.
Values are checked by type, never by what they can do
This is the one rule the package now has, and it took three rounds to find.
Three separate refusals were written as can this value be hashed? — a question
that reads as safe and is not. Measured on node 18, torch
2.10.0+rocm7.2.4:SymIntSymBoolSymFloatSymInthash(v)TypeError: unhashable0TypeErrorisinstance(v, (bool,int,float,str,bytes,tuple))isinstanceSo a hashability gate refuses one of them loudly, accepts two silently and
specialises them, and blows up on the fourth. Asking a value a question is the
leak; asking its type is not. Every check in the package is now an
isinstance, and the round-2 miss — no test ever passed a symbolic value as anattribute, one field over from where round 1's miss was — is covered by
test_a_live_symbolic_value_reaches_no_field_of_a_node, which runs realSymInt,SymBool,SymFloat, an unbacked symbol and one nested in a tupleagainst every field and checks the guard list after each.
An attribute value is now a number, a string, an enum member,
None, or a tupleof those. A tensor and a live symbolic value are both refused by type, without
being asked anything.
A size that is not known yet
hash()on aSymIntraises, backed and unbacked alike, while a node has to behashable and comparable — deciding two blocks are interchangeable is how
repetition gets found, and the generated
__eq__reaches every dimension. Thelive object supports neither, so the canonical form is forced:
SymDimholds the text an expression renders to, and the capture that text isread against. Rendering is the one safe operation —
str()/repr()ofs26,of
2*s26 + 1and ofu0left the guard list at 0. A live symbolic object istherefore never stored, and no comparison can reach one.
Two changes this round, both from the review:
as_dimtakes a concreteintor an already-rendered
SymDim. The old fallback rendered whatever it wasgiven:
np.int64(4096)becameSymDim('4096'), which compares unequal to4096, so one shape became two nodes; an object with no rendering of itsown became its memory address; and a tensor was accepted as a dimension while
being refused as an attribute.
SymDimnow also refuses a rendering that is anumber and one of the form
<… at 0x…>, so the explicit call is guarded too.per capture, so two unrelated traces both produce
s26and both produceu0.Comparing a body recorded in one against a body recorded in another is exactly
what IR-3 — the nested Repeat detector over canonical block signatures #56 and IR-4 — the provably-free grouping rule and its flat-versus-grouped validation #57 do, and on the text alone both comparisons silently agreed.
The scope is mandatory rather than a stated limit, because the wrong answer
here is a false equal, which is the direction that does not announce itself.
How each well-formedness rule is carried by the types
Repeat.bodyis typedRegion, the base ofall four. No leaf-body type, no arity field, no depth counter. Tested to
depth 6 and on the nested non-contiguous form.
index: IndexBindinghas nodefault, so omitting it is a
TypeErrorbefore validation runs.step=0isrefused. A body that already binds the same name is refused via
self.index.name in self.body.bound_indices— tested at depth, through aPar, and with sibling scopes reusing a name.evidence: GroupingEvidenceismandatory and the type is abstract on
describe(), so the bare instance thatused to satisfy the field no longer exists.
IdenticalStructuredemands anon-empty signature;
EqualPricetwo finite, non-negative, equal durations.The signature
IdenticalStructurecarries must be recomputable fromrepeat.body, or the field is ceremony. That is stated in the type's docstring:a constraint on #56 — the signature function must be a pure function of the
body — and a precondition #57 can lean on.
What D20 settled about
attrsandcontext_refCost is not a function of a node's arguments. The tensors that decide an opaque
operator's cost are ambient, and widening the operator's signature to carry them
was tried and reverted.
context_refis a field, not a derivation. Optional, deliberately: atuned GEMM's cost really is a function of its shapes, and requiring a context
key on every opaque leaf would refuse a legitimate node.
attrsrefuses a per-step snapshot two ways, and the two cover differenthalves. The type allowlist is the guarantee.
AMBIENT_READINGSis not atripwire, which is what I called it last round and was wrong about: six of
the readings arrive as plain
int, and no type rule can tell a token countfrom a width that really is the operator's, so the names carry that half on
their own. The list is now 45 names, matched case-insensitively, regenerated
from a census of what ATOM reads off its attention metadata — the command is
in the module docstring so it can be re-run when the metadata grows.
alone is measured unsound: two equally valid allocations of one step moved 64
of 2,439 operator signatures, 32.667 ms against 28.360 ms. Node equality is
structural identity, not a claim that two nodes cost the same.
context_refis what makes rule 2 operational: a body recordsmodel.layers.{layer}.self_attnand instance i bindslayertoindex.value_at(i). The unbound template is also the layer-index-free canonicalform #56 needs.
Decided here, because the design did not say
Regionis the union,Opits only leaf; the grammar'sSeq[Node]uses"node" for both.
Graphis not aRegion— nesting one would put two validity statementson one sequence of work.
in_shapesis one shape per operand; a mapping is refused outright, sinceiterating one yields its keys and what was read would not be what was written.
kindis where the price comes from, not the operator family.count >= 2, non-emptySeq,stream_iddefaults to 0.What surprised me
That the same trap had three faces, one per round, each a field further along:
as_dimraised on aSymInt, and_as_attrssilently specialised aSymBooland a
SymFloat. The defect was never the field — it was that the check askedthe value a question. A rule stated positively over types is the only form that
does not have a next instance.
Second: rule 3 cut against the task that lands next. Evidence demanding two
prices would have forced the repeat detector, which has no prices, to fabricate
a pair — the accident the rule exists to prevent. Hence two evidence subclasses.
Left undone, deliberately
validator (IR-4 — the provably-free grouping rule and its flat-versus-grouped validation #57).
Paris defined and not derived;JoinPolicycarries no arithmetic, sinceresource_boundneeds a device peak that is not this package's to hold.Opis hashable and compares structurally,which is what one would be built from, but defining it is IR-3 — the nested Repeat detector over canonical block signatures #56's.
Gates
Integration moved twice during this task. Measured on node 18, container
xiaobizh_n18_cpu, fromgit archivesnapshots extracted to separate paths andmd5-verified across every hop.
GATE_CPU_RCfe8c8f63ec25edb4af2ef5a153ftests/compass/test_ir_data_model.py(70 → 106 → 160 across the three rounds).f67618eb9(4084) = 4244 − 4084 = +160, the samedelta, so nothing in the merge interacts.
which shows as one fewer passed and one more skipped.
2ef5a153fis a throwaway merge made to measure and not pushed, so nothingstacked on this branch is disturbed.
ruff checkandruff formatclean.Effort, and a caveat on the metric
atom/compass/ir/nodes.pyatom/compass/ir/shapes.pyatom/compass/ir/graph.pyatom/compass/ir/__init__.pytests/compass/test_ir_data_model.py312 against a 150–200 estimate is 1.56× by the project's method, inside the 2×
threshold. By physical lines it is 4.1×, and the reviewer is right that the
gap is now material rather than cosmetic. Three things
ast.unparsecollapses:a multi-name
__all__(53 physical → 4), a 45-name frozenset literal (one line),and every multi-line string. 51
raisestatements account for most of the rest,one unparsed line each against four or five physical. The AST number is the
agreed protocol and I am reporting against it, but it is a measure of statement
count, not of how much there is to read — and #55, #56 and #57 all report against
the same number, so the gap is systematic rather than particular to this task.
Named result
Each of the three rules demonstrated failing on an attempted violation, in
tests/compass/test_ir_data_model.py:Repeat(body=[op]),(op,),"gemm",None,3TypeError: a repeat body is any region -- an operator, a sequence or another repeat -- got listindex="layer"; an inner repeat rebindinglayer, including three levels down through aParTypeErroron the missing argument;TypeError: a repeat carries the index it varies its body over…;ValueError: 'layer' is already bound inside this body. The inner binding would shadow this one…GroupingEvidence();evidence="checked";EqualPrice(32.667e-3, 28.360e-3)TypeErroron the missing argument;TypeError: Can't instantiate abstract class GroupingEvidence…;TypeError: a repeat carries what was compared…;ValueError: the sequence prices at 0.032667 s and the repeat replacing it at 0.02836 s. Grouping has to be free…Source:
atom/compass/design/04_model_capture_and_cost_ir.mdD19, D20, D21.🤖 Generated with Claude Code