Repository navigation
fix: preserve MTML handles across nested lifecycle calls - #15
Conversation
Signed-off-by: CYJiang <googs1025@gmail.com>
|
cc @yeahdongcn /PTAL |
Signed-off-by: CYJiang <googs1025@gmail.com>
yeahdongcn
left a comment
There was a problem hiding this comment.
Merge verdict: no blocking findings — ready to merge.
Head 9aeb2f6 verified against the native contract in mtml_2.2.0.h: one handle per init, one shutdown per handle, released in any order.
Findings: 0 blocking · 2 nit
| nit | location | problem | fix |
|---|---|---|---|
| nit | test_library_lifecycle.py:174 |
barrier workers wait forever when one assertion fails, so a regression hangs make test instead of failing it |
give every Barrier.wait() a timeout= |
| nit | .github/workflows/main.yml:13 |
CI runs three scripts rather than pytest, so this new test file never runs there | add a python -m pytest test_*.py -q step |
What changed · what I could not verify
The contract this now follows. mtml_2.2.0.h:865-866 — several mtmlLibraryInit() pointers keep the library alive "until the corresponding number of mtmlLibraryShutDown() calls have been made with those pointers", and the release order "does not necessarily have to match" the init order. mtml_2.2.0.h:872 — "Providing this function with a lib that has already been initialized causes memory leak". The old code passed the same libHandle into every init, which is exactly that warning; allocating one handle per init and pinning libHandle to the first live one fixes the leak and keeps the implicit query APIs (mtmlLibraryCountDevice and friends) on a handle that cannot have been released, because the stack only ever pops its last entry.
Verified.
python -m pytest test_library_lifecycle.py -vat this head:7 passed in 0.03s. The fixture replaces_LoadMtmlLibraryand_mtmlGetFunctionPointer, so the tests are hardware-independent as claimed, andmonkeypatchrestores the module globals afterwards.- No existing caller regresses.
test_pymtml.py:376-383runs three balanced init/shutdown cycles and both teardowns swallow exceptions (:405-412);example.py:29/57pairs;test_pynvml.py:338shuts down before re-initializing, whichmain()'s leadingnvmlInit()keeps balanced, and that worker catchesException. An unbalanced shutdown used to reach the native call with a NULL handle —MTML_ERROR_INVALID_ARGUMENTper the header — so it already raised;MTML_ERROR_UNINITIALIZEDis a more accurate code, not a new failure mode. - Locking: the native init/shutdown call itself is now serialized by
libLifecycleLock(pymtml.py:995-999), while_LoadMtmlLibraryand_mtmlGetFunctionPointerstay outside it, so thelibLifecycleLock→libLoadLockorder has no inverse path.
Not verified. No MUSA hardware run, no driver smoke; the change is exercised only against the fake library. Native behaviour after a failed shutdown and after a release that follows a failed init is assumed, not observed. gh pr checks 15 --repo MooreThreads/mthreads-ml-py reports no checks reported on the 'fix/mtml-lifecycle' branch: the two workflow runs for this branch (2026-10-13T04:41Z, 10:24Z) sit at action_required on the self-hosted runner, and that workflow would not run pytest even when approved — see the second nit.
auto-review round 1/3 · verdict: clean
|
@googs1025 Please add more tests (MUSA SDK 4.3.x/5.2.x/5.3.x) to PR description. Thanks! |
Signed-off-by: CYJiang <googs1025@gmail.com>
|
Updated the PR description with additional S5000 validation across MUSA 4.3.x, 5.1.x, 5.2.x, and 5.3.x. The validation includes direct native C++ lifecycle tests against |
yeahdongcn
left a comment
There was a problem hiding this comment.
Merge verdict: no blocking findings — ready to merge.
Head d5242fe fixes round 1's barrier hang; pymtml.py is unchanged from the approved commit.
Findings: 0 blocking · 1 nit
Earlier: round 1's hanging barrier test is fixed @d5242fec; the CI nit below still stands
| nit | location | problem | fix |
|---|---|---|---|
| nit | .github/workflows/main.yml:13 |
CI runs three scripts rather than pytest, so this test file never runs there | add a python -m pytest test_*.py -q step |
What changed · what I could not verify
What changed since the approved commit. One commit, d5242fec ("test: bound lifecycle barrier waits"), and it is exactly the first nit of round 1: all three Barrier.wait() calls in test_concurrent_clients_pair_all_native_handles now pass timeout=10 (test_library_lifecycle.py:175,177,179). A threading.Barrier timeout breaks the barrier, so every waiting worker raises BrokenBarrierError instead of blocking the executor — a regression now fails the test rather than stalling make test. The timeout is far above the 0.04 s this suite needs. pymtml.py is byte-identical to 9aeb2f608578, so round 1's contract reading still holds: one handle per mtmlLibraryInit() (mtml_2.2.0.h:865-866), released once each in any order, and libHandle pinned to a live entry so the implicit query APIs never address a released object.
Verified. python -m pytest test_library_lifecycle.py -v at this head: 7 passed in 0.04s. The changed lines are the only diff between the two heads; no callers of mtmlLibraryShutDown were touched. gh pr checks 15 --repo MooreThreads/mthreads-ml-py reports no checks reported on the 'fix/mtml-lifecycle' branch.
Not verified. No MUSA hardware run, no driver smoke; native behaviour after a failed shutdown remains assumed, not observed.
auto-review round 1/3 · verdict: clean
Signed-off-by: CYJiang <googs1025@gmail.com>
|
Addressed the remaining CI nit in I used an explicit unit-test list instead of |
Summary
Fix nested and concurrent MTML lifecycle handling in the Python binding.
This change:
mtmlLibraryInit()for every successful Python init;mtmlLibraryShutDown(handle)once for each Python shutdown;Problem
MTML and NVML expose different native lifecycle models.
NVML exposes process-global initialization APIs:
MTML returns an opaque library handle for each initialization:
The previous Python binding called native
mtmlLibraryInit()repeatedly butstored every result in one global
libHandle:At that point a Python lifecycle reference still existed, but the binding had
lost
handle1and no longer had a valid active handle.MTML native behavior
The MTML implementation already supports multiple handles:
mtmlLibraryInit()returns a newMtmlLibraryhandle;The issue is therefore in Python handle management, not in the MTML C API.
Alignment with pynvml
The public lifecycle control flow follows pynvml:
MTML additionally returns one opaque handle per init. The binding must retain
those handles while providing one stable
libHandlefor existing query APIsthat do not accept an explicit library handle.
Implementation
The binding now maintains all successfully initialized native handles:
On init:
On shutdown, when additional handles exist:
When only the active handle remains:
Lifecycle state transitions are protected by
libLifecycleLock. A failednative init or shutdown leaves
_mtmlLib_handles,libHandle, and_mtmlLib_refcountunchanged.Keeping the first handle stable also prevents a concurrent implicit query from
using an additional handle while another client releases that handle.
Tests
Hardware-independent
Coverage includes:
MTML_ERROR_UNINITIALIZED;Concurrent lifecycle stress:
Additional checks:
Additional MUSA compatibility validation
Lifecycle implementation tested at:
9aeb2f608578e29d09411166fef9514b4e8d7552The current head only adds timeouts to the test barriers; it does not change
the lifecycle implementation.
Hardware: MTT S5000
Compatibility scripts:
python3 test_pymtml.pypython3 test_pynvml.pypython3 test_sglang_compat.pyThe native C++ tests called
libmtmldirectly without going throughpymtml. The Python tests verified that every successful initializationreturns and retains a distinct native handle, every handle is paired with one
native shutdown, and the stable implicit-query handle remains valid until the
final shutdown.
Full pytest on the S5000 environment:
The remaining errors are pre-existing pytest collection issues: script-style
helper functions in
test_pymtml.py,test_pynvml.py, andtest_sglang_compat.pyare collected as fixture-based tests. There were nofunctional test failures.
No new GPU fault, XID, timeout, kernel Oops, or call trace was observed during
the validation window.
Scope
This PR changes:
pymtml.pytest_library_lifecycle.py.github/workflows/main.yml, to run the hardware-independent tests in CIIt does not modify the MTML C API or the existing hardware compatibility
scripts. The CI step uses an explicit unit-test list because the script-style
hardware helpers are not pytest fixture tests and cannot safely be collected by
the broad
test_*.pypattern.Related work