Skip to content

Commit 36062eb

Browse files
committed
[LINT] Fix pre-commit lint failures on pyobject branch
Resolve the lint job failures on PR apache#593: - ruff-check: add one-line docstrings to test methods (D102), hoist threading/weakref imports to top-level (PLC0415). - ty: annotate the rvalue-move callback params as `Any` instead of `object` so `x._move()` resolves (matches test_function.py). - ruff-format / clang-format / cmake-format: apply formatter reflow. - taplo-format: expand the cibuildwheel `build` array to multi-line in pyproject.toml (applied from CI log; taplo has no wheel for this platform). Reproduced locally via pre-commit (taplo-format skipped).
1 parent 1a0e7a7 commit 36062eb

4 files changed

Lines changed: 58 additions & 17 deletions

File tree

CMakeLists.txt

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -273,9 +273,9 @@ if (TVM_FFI_BUILD_PYTHON_MODULE)
273273
VERBATIM
274274
)
275275

276-
# The PyObject-tying impl in tvm_ffi_python_helpers.h uses full Python
277-
# C API (Py_IncRef, PyObject_GC_Del, atomic header reads), so we build
278-
# against the per-version ABI rather than the limited (abi3) ABI.
276+
# The PyObject-tying impl in tvm_ffi_python_helpers.h uses full Python C API (Py_IncRef,
277+
# PyObject_GC_Del, atomic header reads), so we build against the per-version ABI rather than the
278+
# limited (abi3) ABI.
279279
python_add_library(tvm_ffi_cython MODULE "${_core_cpp}" WITH_SOABI)
280280
set_target_properties(tvm_ffi_cython PROPERTIES OUTPUT_NAME "core")
281281
target_include_directories(

pyproject.toml

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,15 @@ docstring-code-line-length = 80
233233
build-verbosity = 1
234234

235235
# Per-Python-version wheels (no abi3 / limited API).
236-
build = ["cp38-*", "cp39-*", "cp310-*", "cp311-*", "cp312-*", "cp313-*", "cp314t-*"]
236+
build = [
237+
"cp38-*",
238+
"cp39-*",
239+
"cp310-*",
240+
"cp311-*",
241+
"cp312-*",
242+
"cp313-*",
243+
"cp314t-*",
244+
]
237245
skip = ["*musllinux*"]
238246
# we only need to test on cp312
239247
test-skip = ["cp38-*", "cp39-*", "cp310-*", "cp311-*"]

src/ffi/custom_allocator.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,12 @@ void* BuiltinDefaultAllocate(size_t size, size_t alignment, int32_t /*type_index
4141
// Total bytes between malloc start and T must be a multiple of `alignment`
4242
// (so T is aligned). The header is sizeof(TVMFFIObjectAllocHeader) bytes;
4343
// if that's already enough, no leading pad. Otherwise round up.
44-
const size_t total_offset =
45-
(sizeof(TVMFFIObjectAllocHeader) + alignment - 1) & ~(alignment - 1);
44+
const size_t total_offset = (sizeof(TVMFFIObjectAllocHeader) + alignment - 1) & ~(alignment - 1);
4645
const size_t total_size = total_offset + size;
4746
void* base_alloc = details::AlignedAllocRuntime(total_size, alignment);
4847
void* ptr = static_cast<char*>(base_alloc) + total_offset;
49-
details::ObjectUnsafe::GetObjectAllocHeaderFromPtr(ptr)->delete_space = &BuiltinDefaultDeleteSpace;
48+
details::ObjectUnsafe::GetObjectAllocHeaderFromPtr(ptr)->delete_space =
49+
&BuiltinDefaultDeleteSpace;
5050
return ptr;
5151
}
5252

tests/python/test_pyobject_tying.py

Lines changed: 43 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,15 @@
3636
"StateC" (Inactive) labels from the resurrection-era design; the behavior they
3737
assert is unchanged under cache & revive.
3838
"""
39+
3940
from __future__ import annotations
4041

4142
import gc
4243
import itertools
4344
import pickle
45+
import threading
46+
import weakref
47+
from typing import Any
4448

4549
import pytest
4650
import tvm_ffi
@@ -83,15 +87,18 @@ class TestStateBIdStable:
8387
"""``a.x is a.x`` and ``id(a.x)`` stable while ``a`` lives."""
8488

8589
def test_attr_access_is_same_object(self) -> None:
90+
"""Repeated attribute access returns the same wrapper object."""
8691
a = Outer(Inner(42))
8792
assert a.x is a.x
8893

8994
def test_attr_access_id_stable(self) -> None:
95+
"""id() of an attribute is stable across repeated access."""
9096
a = Outer(Inner(7))
9197
ids = {id(a.x) for _ in range(100)}
9298
assert len(ids) == 1
9399

94100
def test_chained_attr_access(self) -> None:
101+
"""Chained attribute access reuses the intermediate wrapper."""
95102
# Two-level access: outer.x.val triggers two FieldGetters; the
96103
# intermediate Inner wrapper must be reused so chained access
97104
# doesn't create transient garbage at different addresses.
@@ -101,6 +108,7 @@ def test_chained_attr_access(self) -> None:
101108
assert a.x.val == 11
102109

103110
def test_two_outers_distinct_inners(self) -> None:
111+
"""Distinct C++ objects produce distinct Python wrappers."""
104112
# Different Outer instances wrapping different Inner C++ objects
105113
# MUST produce distinct Python wrappers.
106114
a = Outer(Inner(1))
@@ -125,6 +133,7 @@ class TestStateBFunctionReturns:
125133
"""
126134

127135
def test_function_return_aliases_arg(self) -> None:
136+
"""An FFI return aliases the canonical wrapper of its argument."""
128137
identity = tvm_ffi.convert(lambda x: x)
129138
x = tvm_ffi.convert([1, 2])
130139
assert identity(x) is x
@@ -139,18 +148,20 @@ class TestStateCRevive:
139148
"""
140149

141150
def test_id_preserved_across_finalize(self) -> None:
151+
"""id() and chandle survive a wrapper finalize-and-revive cycle."""
142152
outer = Outer(Inner(1))
143-
first = outer.x # Active: canonical wrapper installed
153+
first = outer.x # Active: canonical wrapper installed
144154
first_id = id(first)
145155
first_chandle = first.__chandle__()
146156
del first
147-
gc.collect() # transitions to Inactive
157+
gc.collect() # transitions to Inactive
148158

149-
revived = outer.x # revive: same address
159+
revived = outer.x # revive: same address
150160
assert id(revived) == first_id
151161
assert revived.__chandle__() == first_chandle
152162

153163
def test_id_preserved_across_many_drops(self) -> None:
164+
"""Each drop+refetch cycle reuses the same wrapper address."""
154165
outer = Outer(Inner(1))
155166
first_id = id(outer.x)
156167
# Deterministic per cycle: each drop+refetch must reuse the same
@@ -180,19 +191,22 @@ def test_no_leak_churn_2k_cycles(self) -> None:
180191
)
181192

182193
def test_chandle_unchanged_across_revive(self) -> None:
194+
"""The chandle is unchanged across a revive cycle."""
183195
outer = Outer(Inner(99))
184196
chandle1 = outer.x.__chandle__()
185197
gc.collect()
186198
chandle2 = outer.x.__chandle__()
187199
assert chandle1 == chandle2
188200

189201
def test_field_value_correct_after_revive(self) -> None:
202+
"""Field value is correct after the wrapper is revived."""
190203
outer = Outer(Inner(123))
191204
_ = outer.x # ensure wrapper exists then dies
192205
gc.collect()
193206
assert outer.x.val == 123
194207

195208
def test_chandle_dies_while_in_state_c(self) -> None:
209+
"""A chandle dying while Inactive reclaims the cached allocation safely."""
196210
# When the chandle finally dies while its wrapper is Inactive,
197211
# ``TVMFFIPyDeleteSpace`` must reclaim the cached (untracked)
198212
# allocation under the GIL via ``PyObject_GC_Del`` and then free the
@@ -240,6 +254,8 @@ class TestNoUnboundedLeak:
240254
"""
241255

242256
def test_distinct_chandles_no_wrapper_leak(self) -> None:
257+
"""Many distinct chandles do not pin one live wrapper each."""
258+
243259
# Locally-registered types: the gc.get_objects() scan below counts
244260
# only instances of *this* InnerLeak, so leftover instances from other
245261
# tests (which use the module-level Inner) cannot pollute the count.
@@ -288,6 +304,7 @@ class TestFinalizerNotInactivated:
288304
"""
289305

290306
def test_del_fires_every_generation(self) -> None:
307+
"""__del__ fires on every drop of a finalizer type, not once-ever."""
291308
calls: list[int] = []
292309

293310
@dc.py_class(_unique_key("InnerDel"))
@@ -314,6 +331,8 @@ class OuterDel(dc.Object):
314331
)
315332

316333
def test_finalizer_type_value_correct_across_refetch(self) -> None:
334+
"""A finalizer type's value and live identity hold across refetch."""
335+
317336
# Even without stable id(), the value and live identity must hold.
318337
@dc.py_class(_unique_key("InnerDel"))
319338
class InnerDel2(dc.Object):
@@ -350,6 +369,7 @@ def test_drop_last_ref_no_crash(self) -> None:
350369
gc.collect()
351370

352371
def test_drop_then_realloc_value_correct(self) -> None:
372+
"""Dropping the last ref frees the memory; a fresh alloc is correct."""
353373
# Inner has no other holder -> __dealloc__ with strong_count == 1 ->
354374
# genuine free (no inactivation), memory reclaimed. Next Inner is fresh.
355375
a = Inner(1)
@@ -370,6 +390,7 @@ class TestRValueRef:
370390
"""
371391

372392
def test_move_into_callback_no_crash(self) -> None:
393+
"""Moving an arg into a callback that re-moves it does not crash."""
373394
# Universal cache-on: callback arg aliases caller's ``x`` (one
374395
# wrapper, one chandle ref). The original blocker was a UAF in
375396
# the deleter path triggered by the eager-detach + chandle-null
@@ -378,7 +399,7 @@ def test_move_into_callback_no_crash(self) -> None:
378399
# and the post-call chandle is null.
379400
use_count = tvm_ffi.get_global_func("testing.object_use_count")
380401

381-
def callback(x: object, expected: int) -> object:
402+
def callback(x: Any, expected: int) -> Any:
382403
gc.collect()
383404
assert expected == use_count(x)
384405
return x._move()
@@ -391,6 +412,7 @@ def callback(x: object, expected: int) -> object:
391412
assert x.__ctypes_handle__().value is None
392413

393414
def test_ffi_move_then_re_fetch_works(self) -> None:
415+
"""Re-fetching a field after an FFI move yields a valid wrapper."""
394416
# The rvalue setter only runs when an FFI call consumes the
395417
# ObjectRValueRef. ``discard`` provides that consumption — the
396418
# setter detaches the canonical-wrapper binding (clearing
@@ -403,6 +425,7 @@ def test_ffi_move_then_re_fetch_works(self) -> None:
403425
assert outer.x.val == 5
404426

405427
def test_state_b_works_after_ffi_move(self) -> None:
428+
"""Active caching resumes on a fresh wrapper after an FFI move."""
406429
# Eager-detach during the move clears the header binding, so
407430
# the chandle returns to Detached. The next access installs a fresh
408431
# canonical wrapper, which then participates in Active caching
@@ -424,12 +447,14 @@ class TestPickle:
424447
"""
425448

426449
def test_pickle_roundtrip_basic(self) -> None:
450+
"""A basic pickle round-trip preserves the value."""
427451
a = tvm_ffi.convert([1, 2, 3])
428452
s = pickle.dumps(a)
429453
b = pickle.loads(s)
430454
assert list(b) == [1, 2, 3]
431455

432456
def test_pickle_roundtrip_preserves_attr_identity(self) -> None:
457+
"""A restored wrapper has stable attribute identity."""
433458
outer = Outer(Inner(77))
434459
s = pickle.dumps(outer)
435460
outer2 = pickle.loads(s)
@@ -449,16 +474,19 @@ class TestPyNativeExempt:
449474
"""
450475

451476
def test_string_construction_and_compare(self) -> None:
477+
"""A converted String behaves as a native str."""
452478
s = tvm_ffi.convert("hello")
453479
assert isinstance(s, str)
454480
assert s == "hello"
455481

456482
def test_bytes_construction_and_compare(self) -> None:
483+
"""A converted Bytes behaves as native bytes."""
457484
b = tvm_ffi.convert(b"world")
458485
assert isinstance(b, bytes)
459486
assert b == b"world"
460487

461488
def test_string_repeated_construction(self) -> None:
489+
"""Repeated String construction does not corrupt adjacent headers."""
462490
# Repeatedly constructing strings shouldn't break the header on
463491
# adjacent objects.
464492
for _ in range(100):
@@ -476,6 +504,7 @@ class TestTypeMismatchOnRevive:
476504
"""
477505

478506
def test_field_access_works_after_many_unrelated_registrations(self) -> None:
507+
"""Unrelated type registrations don't corrupt an existing binding."""
479508
outer = Outer(Inner(1))
480509
first_id = id(outer.x)
481510
# Register a bunch of unrelated classes (no shared chandle).
@@ -503,6 +532,7 @@ class TestFieldSetterAfterRevive:
503532
"""
504533

505534
def test_assign_then_access(self) -> None:
535+
"""Replacing a field after a revive cycle binds the new value cleanly."""
506536
outer = MutableOuter(Inner(1))
507537
first_inner = outer.x
508538
first_chandle = first_inner.__chandle__()
@@ -533,13 +563,15 @@ class TestPickleStress:
533563
"""
534564

535565
def test_many_roundtrips(self) -> None:
566+
"""Repeated pickle round-trips keep the binding state valid."""
536567
for _ in range(200):
537568
outer = Outer(Inner(7))
538569
restored = pickle.loads(pickle.dumps(outer))
539570
assert restored.x.val == 7
540571
assert restored.x is restored.x # Active on restored binding
541572

542573
def test_roundtrip_then_state_c_revive(self) -> None:
574+
"""A pickle-installed binding survives a finalize-and-revive cycle."""
543575
# Round-trip, then exercise Inactive -> Active revive on the restored
544576
# wrapper to confirm the binding installed by pickle survives
545577
# finalize+rewrap.
@@ -562,8 +594,7 @@ class TestThreadingStress:
562594
"""
563595

564596
def test_concurrent_attr_access(self) -> None:
565-
import threading
566-
597+
"""Concurrent attribute access on a shared object does not crash."""
567598
outer = Outer(Inner(123))
568599
errors: list[BaseException] = []
569600

@@ -582,12 +613,11 @@ def worker() -> None:
582613
assert not errors, f"worker(s) raised: {errors!r}"
583614

584615
def test_concurrent_independent_outers(self) -> None:
616+
"""Concurrent wrapper churn on per-thread objects does not crash."""
585617
# Each thread owns its own Outer; threads don't share chandles
586618
# so the test mostly exercises wrapper churn (Active -> Inactive -> Active) in
587619
# isolation. ``gc.collect()`` is called periodically (not every
588620
# iteration) to keep the test under a few seconds.
589-
import threading
590-
591621
errors: list[BaseException] = []
592622

593623
def worker(seed: int) -> None:
@@ -621,6 +651,7 @@ class TestStateCWithCyclicGC:
621651
"""
622652

623653
def test_gc_collect_inside_revive_loop(self) -> None:
654+
"""gc.collect() between drop and refetch does not crash."""
624655
outer = Outer(Inner(5))
625656
# A bad traversal of the untracked cached allocation crashes deterministically on
626657
# the first collect, so a modest count suffices; the cost here is the
@@ -632,6 +663,7 @@ def test_gc_collect_inside_revive_loop(self) -> None:
632663
assert outer.x.val == 5
633664

634665
def test_gc_collect_with_holder_keeping_chandle(self) -> None:
666+
"""gc.collect() around inactive memory keeps id() stable."""
635667
# The Inner chandle is held by ``outer``'s field for the lifetime
636668
# of the test, so wrapper drops always inactivate the allocation. gc.collect
637669
# between drops exercises GC stability around the inactive memory.
@@ -655,6 +687,7 @@ class TestMultipleChandlesIsolation:
655687
"""
656688

657689
def test_distinct_state_c_revive_independently(self) -> None:
690+
"""Each chandle revives to its own address and value independently."""
658691
outers = [Outer(Inner(i * 10)) for i in range(50)]
659692
# Capture Active addresses, then drop to Inactive.
660693
b_ids = [id(o.x) for o in outers]
@@ -667,6 +700,7 @@ def test_distinct_state_c_revive_independently(self) -> None:
667700
assert revived.val == i * 10
668701

669702
def test_interleaved_revives_do_not_cross_contaminate(self) -> None:
703+
"""Reviving in a different order keeps ids matched per-chandle."""
670704
# Build N outers, capture ids, drop all, then revive in a
671705
# different order. Ids must still match per-chandle.
672706
outers = [Outer(Inner(i)) for i in range(30)]
@@ -690,8 +724,7 @@ class TestWeakrefNotSupported:
690724
"""
691725

692726
def test_weakref_raises_typeerror(self) -> None:
693-
import weakref
694-
727+
"""Taking a weakref to a wrapper raises TypeError (documented limit)."""
695728
outer = Outer(Inner(1))
696729
with pytest.raises(TypeError):
697730
weakref.ref(outer.x)

0 commit comments

Comments
 (0)