Skip to content

compass(kv): refuse a parallel width written as integer text #340

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jgong5 merged 1 commit into from
Sep 23, 2026
Merged

compass(kv): refuse a parallel width written as integer text #340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions atom/compass/kv/handoff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,26 +85,25 @@ def whole_number(field: str, value: Any) -> int:
"""*value* as an `int`, or a `ValueError` naming *field* if it is not one.

An `int` is taken as it is, and so is a float with no fractional part.
Text is taken only if `int` reads it as an integer literal, so "8" and
" 8 " are 8 and "8.0" and "1e1" are refused. ATOM's own launch path does
not deliver a width as text -- its CLI flags and `ATOM_DP_RANK` are parsed
with `int`, and its `Config` raises on a string width -- so text arrives
only from a caller that builds the config by hand.
Text is refused, "8" included. ATOM's own launch path does not deliver a
width as text -- its CLI flags and `ATOM_DP_RANK` are parsed with `int`,
and its `Config` raises on a string width -- so reading text would be a
conversion that no launch needs.

Refused: a fraction, because `int` truncates and 8.5 would go out as 8,
naming a deployment that was never launched; a `bool`, which Python
counts as an `int` and which would go out as a width of 1 or 0; and
anything else `int` does not take exactly. No value that is accepted
changes on the way to the `int` returned.
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".

@jgong5 jgong5 Sep 23, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

"""
try:
if isinstance(value, bool):

@jgong5 jgong5 Sep 23, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

raise TypeError(f"{value!r} is a bool")
number = int(value) if isinstance(value, str) else value
whole = int(number)
whole = int(value)
except (TypeError, ValueError, OverflowError):
whole = None
if whole is None or whole != number:
if whole is None or whole != value:
raise ValueError(
f"{field} is {value!r}, which is not a whole number; a parallel "
"width or rank is a count, so it is refused rather than converted"
Expand Down
56 changes: 18 additions & 38 deletions tests/compass/test_kv_remote_prefill.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,13 +248,13 @@ def test_the_engine_id_is_the_label_the_host_invents_and_nothing_else():
def test_the_ranks_the_router_reads_are_numbers(geometry):
"""The router drops `dp_rank` unless it is a number, and says nothing.

Driven through the connector from a config carrying the widths as text,
because that is the only place they can arrive as anything but an int:
the request does not carry them, and handing `transfer_params` two
literal ints asserts nothing the cast is responsible for. A config field
filled from an environment variable or a JSON file is a string, and the
failure it causes is silent -- the router substitutes its own registry
value for the prefilling worker rather than refusing the blob.
Driven through the connector from a config carrying the widths as floats
with no fractional part, because handing `transfer_params` two literal
ints asserts nothing the conversion is responsible for. A whole float is
converted to an `int` on the way in; text is refused, which the test
below pins. A rank that went out as anything but a number would fail
silently -- the router substitutes its own registry value for the
prefilling worker rather than refusing the blob.
"""
blob = simulated_blob(tp_size=8, dp_rank=3)
assert isinstance(blob["dp_rank"], int) and blob["dp_rank"] == 3
Expand All @@ -264,8 +264,8 @@ def test_the_ranks_the_router_reads_are_numbers(geometry):
model_for(geometry, PEAKS[0]),
lambda: ISSUE_AT,
role="scheduler",
tp_size="8",
dp_rank="3",
tp_size=8.0,
dp_rank=3.0,
)
seq = finished_sequence()
scheduler.request_finished(seq)
Expand All @@ -288,6 +288,9 @@ def test_the_ranks_the_router_reads_are_numbers(geometry):
pytest.param("tp_size", False, id="tp_size-false"),
pytest.param("dp_rank", True, id="dp_rank-true"),
pytest.param("dp_rank", False, id="dp_rank-false"),
pytest.param("tp_size", "8", id="tp_size-integer-text"),
pytest.param("dp_rank", "3", id="dp_rank-integer-text"),
pytest.param("tp_size", " 8 ", id="tp_size-padded-text"),
],
)
def test_a_width_that_is_not_a_whole_number_is_refused_by_name(geometry, field, value):
Expand All @@ -296,9 +299,9 @@ def test_a_width_that_is_not_a_whole_number_is_refused_by_name(geometry, field,
Refused when the connector is built, so a malformed config never serves a
request. Casting 8.5 to 8 would emit a blob for a deployment that was
never launched, and nothing reading it could tell. A `bool` is an `int` to
Python and would go out as a width of 1 or 0. Text that `int` does not
read as an integer literal is refused rather than read as a float, which
would take "1e1" as 10.
Python and would go out as a width of 1 or 0. Text is refused whatever it
spells, "8" included: ATOM's launch path parses its widths with `int`, so
reading text would be a conversion no launch needs.
"""
widths = {"tp_size": 8, "dp_rank": 3, field: value}
with pytest.raises(ValueError, match=f"^{field} is .*not a whole number"):
Expand All @@ -307,32 +310,9 @@ def test_a_width_that_is_not_a_whole_number_is_refused_by_name(geometry, field,
)


def test_a_width_in_text_keeps_its_exact_value(geometry):
"""Text is read as an integer, never through a float that rounds it.

2**53 + 1 is the smallest integer a float cannot hold, so read through a
float it would go out as 2**53: a value changed on the way to an `int`.
"""
exact = 2**53 + 1
scheduler = connector(
model_for(geometry, PEAKS[0]),
lambda: ISSUE_AT,
role="scheduler",
tp_size=str(exact),
dp_rank=str(exact),
)
seq = finished_sequence()
scheduler.request_finished(seq)
relayed = seq.kv_transfer_params_output
assert relayed["tp_size"] == exact, f"tp_size went out as {relayed['tp_size']}"
assert relayed["dp_rank"] == exact, f"dp_rank went out as {relayed['dp_rank']}"


@pytest.mark.parametrize(
"value", [8, "8", 8.0, " 8 "], ids=["int", "text", "float", "padded"]
)
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"])

@jgong5 jgong5 Sep 23, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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_numbers
  • test_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."

def test_a_whole_width_is_taken_as_an_int_or_a_whole_float(geometry, value):
"""The refusal above is not of an `int` or of a float with no fraction."""
scheduler = connector(
model_for(geometry, PEAKS[0]),
lambda: ISSUE_AT,
Expand Down