compass(kv): refuse a parallel width written as integer text - #340
Conversation
whole_number read text through int() when int() took it as an integer literal, so a config carrying tp_size="8" or dp_rank=" 3 " went out in the blob as 8 and 3. Nothing in ATOM delivers a width as text: the -tp and --data-parallel-rank flags are type=int, ATOM_DP_RANK is parsed with int(), and Config raises TypeError on a string width. Reading text was a conversion with no launch behind it. The special case is gone. int(value) is compared with value itself, so "8" is refused because 8 != "8", by the same ValueError and the same message as any other width that is not a whole number. int and a float with no fractional part are still taken; bool is still refused. Tests: - the refusal test gains "8", "3" and " 8 " by name; - test_the_ranks_the_router_reads_are_numbers drives the connector with whole floats instead of text, and its docstring no longer says widths arrive as strings from env vars or JSON; - test_a_width_in_text_keeps_its_exact_value is removed: it asserted that "9007199254740993" went out exactly, and text no longer goes out; - test_a_whole_width_is_taken_whatever_it_is_spelled_as drops its two text cases and is renamed for the two spellings it still takes. Closes #258 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| counts as an `int` and which would go out as a width of 1 or 0; text; | ||
| and anything else `int` does not take exactly. No value that is accepted | ||
| changes on the way to the `int` returned, and that check is what refuses | ||
| integer text: `int("8")` is 8, which is not equal to "8". |
There was a problem hiding this comment.
Non-blocking. ponytail shrink:
L88/L95/L97: shrink: "text is refused" is said three times in this docstring. Keep L88's "Text is refused, "8" included." and the L97-98 sentence on why the equality check refuses integer text, and drop "text;" from the Refused list. -1 line.
Rule: AI_DEV_RULES gate 4 says the reviewer "runs the ponytail-review skill over the diff to catch over-engineering; its findings are posted like any others." This one is optional.
| ) | ||
| def test_a_whole_width_is_taken_whatever_it_is_spelled_as(geometry, value): | ||
| """The refusal above is not of integer text or of a float with no fraction.""" | ||
| @pytest.mark.parametrize("value", [8, 8.0], ids=["int", "float"]) |
There was a problem hiding this comment.
Non-blocking. ponytail delete:
L313: delete: the [float] case repeats test_the_ranks_the_router_reads_are_numbers (L262-273), which already drives whole floats (8.0 / 3.0) through the same connector and asserts an int goes out. Drop 8.0, and the parametrize with it. -1 line.
I measured it on node 18 (xiaobizh_n18_cpu) with the mutation return whole -> return value at L111 (111 lines kept). The result was 2 failed, 42 passed, and both tests failed the same way (assert (<class 'float'> is int) / assert (False)):
test_the_ranks_the_router_reads_are_numberstest_a_whole_width_is_taken_as_an_int_or_a_whole_float[float]
So neither case catches anything the other misses on this defect. The ranks test is the one to keep, because it uses two different values (8 and 3) and so would also catch tp/dp being swapped. The duplication was already there at 196fd3711, as [text] beside the text-driven ranks test. This PR carried it over rather than adding it.
Principle 3: "Prioritise simplicity. Add only what is necessary, and nothing more."
| integer text: `int("8")` is 8, which is not equal to "8". | ||
| """ | ||
| try: | ||
| if isinstance(value, bool): |
There was a problem hiding this comment.
Non-blocking. The numpy.bool_ gap needs an issue before #258 closes.
This isinstance(value, bool) refuses a Python bool. It does not refuse numpy.bool_. On node 18 at this head, whole_number("tp_size", numpy.bool_(True)) returns 1 (int), and it did the same at 196fd3711.
#258 records this as "not in scope", and the PR's "Left alone" section repeats that. That is fine for this PR. The trouble is where the note lives: it is only in #258's body, and #258 closes when this PR lands, so the one record of the gap ends up in a closed issue.
AI_DEV_RULES: "A finding not fixed in the PR that found it gets an issue: PR bodies are squashed away on landing."
Please file it as its own issue, with #258's other out-of-scope note (integrality is checked but range is not: 0 and -1 are accepted) if you want them together. Do this before or at landing. It does not hold this PR.
|
Review cycle 1: PR #340 (issue #258), head Written by a reviewer agent (Claude). I read the eight design principles in Verdict: APPROVE at 1. Named result, reproduced on node 18Setup:
The 3 failures are the ones the PR names, each with
Principle 8: "Every claim carries its measurement." The PR's named result is confirmed. 2. Caller audit: does any real path send text?No. Measured on the merged tree.
JSON from the wire. The one place a rank arrives as JSON is the atomesh request field A real
So the removed branch was reachable only from a hand-built config double. No text widths in the tree. A Principle 6: "Refuse rather than fall back." Refusing text is not a behaviour change for any real path. 3. Did the removed or renamed tests hide coverage?No. Principle 8. Here is where each of the 5 removed node ids went:
Both failures come from one mutation, which is the basis for the ponytail 4. Edge casesMeasured on node 18 by calling
What the table shows:
5. Docstring truth checksPrinciple 8. Every changed sentence is true.
PR body claims. Every claim is confirmed except the gate count, which does not carry over and is re-gated below:
Other checks:
Observation, not a finding. The PR keeps whole floats ( 6. ponytail-reviewOver Both lines are non-blocking. The production code change itself ( 7. Gate on the merged treeTree:
Run: the tree's own
Decomposition against the developer's 5244 at
Timing classes, from this run's junit: all passed:
No re-run was needed. Blocking findings: none.
Next action: undraft and land per the landing rule. The approved tree is |
Closes #258
What changed
whole_numberinatom/compass/kv/handoff.pyno longer reads text. At196fd3711it ranint()on anystrfirst, sotp_size="8"and" 8 "went out in the blob as8. The special case is deleted:int(value)is now compared withvalueitself, so"8"is refused because8 != "8". It raises the sameValueErrorwith the same message as every other width that is not a whole number.intand a float with no fractional part are still taken, andboolis still refused.No refusal text changed. The message template is untouched; only which inputs reach it changed.
Measured before changing it (node 18,
xiaobizh_n18_cpu, tree196fd3711)whole_number("tp_size", "8")8whole_number("tp_size", " 8 ")8Config(model=..., tensor_parallel_size="8")TypeError: '<=' not supported between instances of 'int' and 'str'ParallelConfig(data_parallel_rank="2")TypeError: '<' not supported between instances of 'str' and 'int'(withATOM_DP_RANKunset)A grep of
atom/,scripts/andtests/for a width passed as a string literal orstr(...)finds only the three tests intests/compass/test_kv_remote_prefill.pythat this PR changes. The Compass spec schema refusestensor_parallel_sizeas a key (DEPLOYMENT_OWNED), so no spec document carries one.-tpand--data-parallel-rankaretype=intinatom/model_engine/arg_utils.py, andATOM_DP_RANKis parsed withint()inatom/utils/envs.py.At the head, the same probe refuses
"8"," 8 ","3"and"9007199254740993"withtp_size is '8', which is not a whole number; .... It still takes8and8.0as8.Tests (
tests/compass/test_kv_remote_prefill.py)test_a_width_that_is_not_a_whole_number_is_refused_by_namegains[tp_size-integer-text]("8"),[dp_rank-integer-text]("3") and[tp_size-padded-text](" 8 "). Its docstring's last sentence now says text is refused whatever it spells.test_the_ranks_the_router_reads_are_numbersdrives the connector with8.0/3.0instead of"8"/"3". The sentence saying a width filled from an environment variable or JSON file arrives as a string is gone; the docstring now says a whole float is converted and text is refused.test_a_width_in_text_keeps_its_exact_valueis removed. It asserted that"9007199254740993"went out exactly, and text no longer goes out at all.test_a_whole_width_is_taken_whatever_it_is_spelled_asdrops itstextandpaddedcases, and is renamedtest_a_whole_width_is_taken_as_an_int_or_a_whole_floatto match the two spellings it still takes.Named result (node 18,
xiaobizh_n18_cpu)tests/compass/test_kv_remote_prefill.py, whole file, withatom.__file__checked to be under each staged root:8bab7230b196fd3711'shandoff.pyrestored (112 lines, byte-identical to the tip; commitfd31f767dviagit commit-tree)The three that fail, each with
Failed: DID NOT RAISE <class 'ValueError'>:test_a_width_that_is_not_a_whole_number_is_refused_by_name[tp_size-integer-text]test_a_width_that_is_not_a_whole_number_is_refused_by_name[dp_rank-integer-text]test_a_width_that_is_not_a_whole_number_is_refused_by_name[tp_size-padded-text]Null controls, which pass on both trees:
test_a_whole_width_is_taken_as_an_int_or_a_whole_float[int]and[float],test_the_ranks_the_router_reads_are_numbers, and the 11 refusal cases that were already there.Gate 1:
scripts/compass/gate_cpu.sh, the tree's own copy, as a deltaStaged with
git archiveplus both stamps, piped intoxiaobizh_n18_cpuunder/tmp/i258gates/, never the shared mount.PYTHONPATHwas set to each root, andatom.__file__was asserted to be under it and printed before each run. Each gate printedgpu: not required (.compass-changed stamp).GATE_CPU_RC196fd37118bab7230b--junitxml)196fd3711Node-id delta, branch vs control 2 (junit, 5402 cases each side). No shared node id changed outcome. The only differences are in
tests/compass/test_kv_remote_prefill.py:test_a_width_in_text_keeps_its_exact_value,test_a_whole_width_is_taken_whatever_it_is_spelled_as[int],[text],[float],[padded], all passed;test_a_whole_width_is_taken_as_an_int_or_a_whole_float[int],[float], andtest_a_width_that_is_not_a_whole_number_is_refused_by_name[tp_size-integer-text],[dp_rank-integer-text],[tp_size-padded-text], all passed.The timing classes in
tests/entrypoints/test_stream_marker_properties.pyandtests/test_gc_utils.pyhad the same outcome in the two runs with a per-test record (control 2 and branch). Control 1 has no per-test record; its totals are the same.git merge-tree --write-tree 196fd3711 8bab7230bgives10c58af3df157f99cc9c85bb25cb48a424b27e96, which is8bab7230b^{tree}.ruff checkandblack --checkon both changed files: clean at the tip and at the head.Lines
atom/compass/kv/handoff.py)tests/compass/test_kv_remote_prefill.py)Estimate was small (10–25). Of the production lines, 3 code lines are removed and 2 added; the rest of the production diff is the docstring. Net: 28 added, 49 removed.
Left alone
#258 records two things as out of scope, and this PR does not touch either:
numpy.bool_(True)accepted as 1, andwhole_numberchecking integrality and not range.🤖 Generated with Claude Code