Skip to content

Commit 0f52d1e

Browse files
committed
fix(iluvatar): leave out= entry points alone; scope the copy tests
Review follow-ups from flagos-ai#644. `sum_dim_out` (and every other `*_out` binding) is no longer rewired by the alias sweep. An `out=` entry point is not the same function as the one it is named after: flag_gems' `sum_dim_out` passes the caller's `keepdim` through and reads the result dtype off the input, which is how `torch.sum(bool_input, dim=1, out=int64_out)` lands in an int64 buffer. The wrapper forces `keepdim=False` and returns a fresh tensor, so wrapping that binding would leave the caller's buffer untouched and change the dtype it derived. Those calls keep the workaround's documented scope -- unmasked, and therefore still wrong on this backend -- rather than acquiring a second, silent change. Two tests pin it: a build that aliases `sum_dim_out` to `sum_dim` must still leave the `_out` name alone, and the bool-input `out=` dtype rule must survive the patch. The new unit file is now a manifest step of its own, so it runs on every platform with torch. It is not in the platform-agnostic job, which installs no torch by design -- there it would be collected and skipped, which is not evidence. It is not in `unit_tests_per_file` either: widening that group to Iluvatar still surfaces five unrelated failures in the rest of tests/unit (test_profiler_privateuse1 crashes, the ascend/gcu qwenimage rope files fail, and the flagcicd report/coverage files fail), which belongs to the PR that triages them. The file resolves `torch_fl` through the installed wheel first, like tests/integration/ops/conftest.py, so it runs in a wheel-only workspace and in a source checkout alike. The two Iluvatar-lane failures in tests/integration/ops/test_dtype_cast_copy.py were the test's premise, not a regression: * `test_cuda_abi_cast_does_not_route_through_host` reads csrc/aten/copy_ops.cc, which a wheel-only workspace does not have. It now skips there with that reason instead of raising FileNotFoundError. * `test_bf16_fp32_cast_is_transfer_free` asserts a runtime property that only holds where an on-device cast exists. copy_ops.cc's same-device flagos->flagos branch reaches a native cast on the CUDA-ABI platforms and a vendor cast on Ascend/GCU/Kunlunxin/MUSA, but Iluvatar is in the "Fallback: CPU round-trip when no on-device cast is available" set, so the transfer count there is the design. It is skipped for the four builds with no `DtypeCast` (bpu, iluvatar, mps, tsingmicro) and stays a real check everywhere else. Verified on iluvatar-109 (BI-V150, torch_fl 2.10.0+corex4.5.0): tests/unit/test_iluvatar_masked_reductions.py 19 passed, from a source checkout and from a wheel-only directory; test_dtype_cast_copy.py 14 passed / 2 skipped in the checkout and 10 passed / 6 skipped with no csrc present; end-to-end unchanged -- vision tower 3/3 bit-identical, 12 samples 12/12 same side with max|d| = 7.54e-3 and no NaN, bit-identical to the earlier run. Refs flagos-ai#617
1 parent 4410c01 commit 0f52d1e

4 files changed

Lines changed: 168 additions & 12 deletions

File tree

‎.github/integration-manifest.yml‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -681,6 +681,24 @@ steps:
681681
cd "$GITHUB_WORKSPACE" && UNIT_TESTS_ALLOW_TEARDOWN_CRASH=1
682682
python .github/scripts/run_unit_tests.py --timeout 300
683683
684+
# #644: the Iluvatar non-inner reduction workaround wraps flag_gems entry
685+
# points in Python, so its contract -- which calls take the path at all, which
686+
# axis moves, `keepdim` round-trip, and that `*_out` bindings are left alone --
687+
# is checked with torch alone: no device, no FlagGems build, no weights. That
688+
# is why it runs on every platform rather than only where the wrapper is
689+
# installed; a drift in it would be silent everywhere.
690+
#
691+
# Not in .github/workflows/agnostic-checks.yml: that job installs no torch by
692+
# design, so a torch-backed file listed there is collected and skipped, which
693+
# is not evidence. Not in `unit_tests_per_file` either: that group now names
694+
# only dcu, and running the whole of tests/unit on Iluvatar still has
695+
# unrelated failures (test_profiler_privateuse1 crashes, the ascend/gcu
696+
# qwenimage rope files fail), so widening it belongs to the PR that triages
697+
# those, not to this one.
698+
- id: iluvatar_reduction_contract
699+
name: Iluvatar reduction workaround contract
700+
command: python -m pytest tests/unit/test_iluvatar_masked_reductions.py -v --tb=short
701+
684702
- id: runtime_conformance
685703
name: Runtime conformance (hardware-agnostic)
686704
command: python -m pytest tests/integration/runtime -v -s --tb=short

‎tests/integration/ops/test_dtype_cast_copy.py‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,30 @@
5252
CUDA_ABI_PLATFORMS = ("cuda", "dcu", "metax", "ppu")
5353

5454

55+
def _build_platform() -> str:
56+
"""The accelerator this wheel was built for ("" when unrecorded)."""
57+
return torch_fl._build_accelerator()
58+
59+
60+
# Builds with no on-device dtype cast at all: copy_ops.cc puts them in the
61+
# same-device flagos->flagos branch's "Fallback: CPU round-trip when no
62+
# on-device cast is available". The CUDA-ABI platforms reach the native
63+
# TensorIterator cast through the boxed arm, and Ascend / GCU / Kunlunxin /
64+
# MUSA each compile a vendor cast (`aclnnCast`, `topsatenToCopy`, `cast`,
65+
# `MudnnCopy`); these four do not, so a host round trip is the design there and
66+
# the transfer count below cannot be zero.
67+
_NO_ON_DEVICE_CAST = ("bpu", "iluvatar", "mps", "tsingmicro")
68+
69+
70+
def _has_on_device_cast() -> bool:
71+
"""Whether `.to(dtype)` can stay on the device on this build.
72+
73+
See `csrc/aten/copy_ops.cc`, the same-device flagos->flagos dtype branch;
74+
the USE_ILUVATAR group there has no `DtypeCast` implementation.
75+
"""
76+
return _build_platform() not in _NO_ON_DEVICE_CAST
77+
78+
5579
def _flagos_available():
5680
try:
5781
return (
@@ -179,6 +203,14 @@ def test_cuda_abi_cast_does_not_route_through_host(platform):
179203
)
180204

181205

206+
@pytest.mark.skipif(
207+
not _has_on_device_cast(),
208+
reason=(
209+
"this build has no on-device dtype cast: copy_ops.cc takes the documented "
210+
"'Fallback: CPU round-trip when no on-device cast is available' branch, "
211+
"so the transfer count below is the design and not a regression"
212+
),
213+
)
182214
@pytest.mark.parametrize(
183215
"src_dtype,dst_dtype",
184216
[(torch.bfloat16, torch.float32), (torch.float32, torch.bfloat16)],

‎tests/unit/test_iluvatar_masked_reductions.py‎

Lines changed: 105 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,27 +23,48 @@
2323
2424
The module is loaded by path rather than imported: importing ``torch_fl``
2525
executes the package ``__init__`` and needs a built accelerator, and the module
26-
under test itself only needs ``torch``.
26+
under test itself only needs ``torch``. The path resolves through the installed
27+
``torch_fl`` first so the same file runs in a wheel-only workspace and in a
28+
source checkout.
2729
"""
2830

2931
import importlib.util
32+
import sys
3033
from pathlib import Path
3134

3235
import pytest
3336

3437
torch = pytest.importorskip("torch")
3538

36-
_MODULE_PATH = (
37-
Path(__file__).resolve().parents[2]
38-
/ "torch_fl"
39-
/ "compile"
40-
/ "iluvatar_masked_reductions.py"
41-
)
39+
40+
def _module_path() -> Path:
41+
"""Locate torch_fl/compile/iluvatar_masked_reductions.py without importing the package.
42+
43+
From wherever torch_fl resolves: the installed wheel in CI's wheel-only test
44+
workspace (which has no source tree), or the repo copy in a source checkout.
45+
Same arrangement as tests/integration/ops/conftest.py's `_platform_path`,
46+
because this file runs in both.
47+
"""
48+
spec = importlib.util.find_spec("torch_fl")
49+
if spec is not None and spec.origin:
50+
candidate = (
51+
Path(spec.origin).resolve().parent
52+
/ "compile"
53+
/ "iluvatar_masked_reductions.py"
54+
)
55+
if candidate.is_file():
56+
return candidate
57+
return (
58+
Path(__file__).resolve().parents[2]
59+
/ "torch_fl"
60+
/ "compile"
61+
/ "iluvatar_masked_reductions.py"
62+
)
4263

4364

4465
def _load():
4566
spec = importlib.util.spec_from_file_location(
46-
"iluvatar_masked_reductions", _MODULE_PATH
67+
"iluvatar_masked_reductions", _module_path()
4768
)
4869
module = importlib.util.module_from_spec(spec)
4970
spec.loader.exec_module(module)
@@ -162,9 +183,83 @@ def fake_std(x, dim=None, *, correction=None, keepdim=False):
162183
assert shape == (2, 4, 3) and dim == -1 and correction == 1 and keepdim is False
163184

164185

165-
def test_patch_is_idempotent_and_tolerates_missing_flag_gems(monkeypatch):
166-
import sys
186+
def test_out_entry_points_are_never_rewired(monkeypatch):
187+
"""`out=` entry points keep their own argument behaviour.
188+
189+
flag_gems' `sum_dim_out` is not `sum_dim`: with no `dtype` it passes the
190+
caller's `keepdim` through and reads the result dtype off the input, which
191+
is how `torch.sum(bool_input, dim=1, out=int64_out)` lands in an int64
192+
buffer. The wrapper forces `keepdim=False` and returns a fresh tensor, so
193+
wrapping that binding would silently change both. Alias sweeps keyed on
194+
object identity are exactly where that mistake hides: a build that binds
195+
`sum_dim_out` to the same function as `sum_dim` would be rewired with it.
196+
"""
197+
198+
class FakeGems:
199+
pass
167200

201+
def sum_dim(inp, dim=None, keepdim=False, **kw):
202+
return inp
203+
204+
def sum_dim_out(inp, dim=None, keepdim=False, **kw):
205+
return inp
206+
207+
fake = FakeGems()
208+
fake.sum_dim = sum_dim
209+
fake.sum_dim_out = sum_dim_out
210+
monkeypatch.setitem(sys.modules, "flag_gems", fake)
211+
212+
assert mr.patch_iluvatar_masked_reductions() is True
213+
assert fake.sum_dim.__code__.co_name == "patched"
214+
assert fake.sum_dim_out is sum_dim_out
215+
216+
# The identity sweep is what makes this worth pinning: a build that aliases
217+
# the two entry points must still leave the `out=` name alone.
218+
del sys.modules["flag_gems"]
219+
aliased = FakeGems()
220+
aliased.sum_dim = aliased.sum_dim_out = sum_dim
221+
monkeypatch.setitem(sys.modules, "flag_gems", aliased)
222+
assert mr.patch_iluvatar_masked_reductions() is True
223+
assert aliased.sum_dim.__code__.co_name == "patched"
224+
assert aliased.sum_dim_out is sum_dim
225+
226+
227+
def test_bool_sum_out_dtype_survives_the_patch(monkeypatch, tmp_path):
228+
"""`torch.sum(bool, dim=1, out=...)` still writes the int64 the op promises.
229+
230+
Mirrors flag_gems' `sum_dim_comm` dtype rule: an unspecified dtype is read
231+
off the input, and a bool input becomes int64. The patched binding is the
232+
one `sum.IntList_out` resolves, so if the sweep had wrapped it the caller
233+
would get a bool result back and the `out=` buffer would stay untouched.
234+
"""
235+
236+
class FakeGems:
237+
pass
238+
239+
def sum_dim_comm(inp, dim=None, keepdim=False, *, dtype=None, out=None):
240+
if dtype is None:
241+
dtype = torch.int64 if inp.dtype is torch.bool else inp.dtype
242+
result = torch.sum(inp, dim=dim, keepdim=keepdim, dtype=dtype)
243+
if out is not None:
244+
out.copy_(result)
245+
return out
246+
return result
247+
248+
fake = FakeGems()
249+
fake.sum_dim = lambda inp, dim=None, keepdim=False, **kw: inp
250+
fake.sum_dim_out = sum_dim_comm
251+
monkeypatch.setitem(sys.modules, "flag_gems", fake)
252+
mr.patch_iluvatar_masked_reductions()
253+
254+
mask = torch.tensor([[True, False, True], [False, True, True]])
255+
out = torch.empty(2, dtype=torch.int64)
256+
got = fake.sum_dim_out(mask, 1, out=out)
257+
assert got is out
258+
assert out.dtype == torch.int64
259+
assert out.tolist() == [2, 2]
260+
261+
262+
def test_patch_is_idempotent_and_tolerates_missing_flag_gems(monkeypatch):
168263
class FakeGems:
169264
pass
170265

‎torch_fl/compile/iluvatar_masked_reductions.py‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -183,8 +183,19 @@ def patch_iluvatar_masked_reductions() -> bool:
183183
# kernels resolve the package-level name, flag_gems' own ops call their
184184
# siblings through the module, and some modules import a sibling by
185185
# name, so identity is the only reliable way to catch every alias.
186+
#
187+
# ``*_out`` names are left alone. An ``out=`` entry point is not the
188+
# same function: flag_gems' ``sum_dim_out`` keeps the caller's
189+
# ``keepdim`` and casts a bool input to int64 regardless of the dtype
190+
# argument, so writing through it is how ``torch.sum(bool, dim=1,
191+
# out=int64_out)`` gets the result it asked for. The wrapper forces
192+
# ``keepdim=False`` and returns a fresh tensor, which would leave the
193+
# caller's buffer untouched and the dtype it derived from the input.
194+
# Those calls keep the pre-existing behaviour of this workaround --
195+
# unmasked, and therefore wrong on this backend -- which is the
196+
# documented scope rather than a silent semantics change on top.
186197
for name, val in list(vars(flag_gems).items()):
187-
if val is orig:
198+
if val is orig and not name.endswith("_out"):
188199
setattr(flag_gems, name, wrapped)
189200
for mod in list(sys.modules.values()):
190201
if not getattr(mod, "__name__", "").startswith("flag_gems"):
@@ -194,7 +205,7 @@ def patch_iluvatar_masked_reductions() -> bool:
194205
except Exception:
195206
continue
196207
for name, val in list(namespace.items()):
197-
if val is orig:
208+
if val is orig and not name.endswith("_out"):
198209
try:
199210
setattr(mod, name, wrapped)
200211
except Exception:

0 commit comments

Comments
 (0)