Skip to content

[PD] Read nixl TransferInfo.is_dummy as a field in unit tests - #39100

Merged
ShangmingCai merged 2 commits into
mainfrom
fix/nixl-is-dummy-field
Sep 11, 2026
Merged

ShangmingCai merged 2 commits into
mainfrom
fix/nixl-is-dummy-field

Conversation

@ShangmingCai

@ShangmingCai ShangmingCai commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

Important

Merge this only after #36612. TransferInfo.is_dummy is still a method on main until that PR lands, so this PR is red on its own and turns green the moment #36612 is in. Merging it first would break main in the opposite direction.

#36612 turns TransferInfo.is_dummy from a method into a bool dataclass field computed once in from_zmq(). It converts the two call sites that existed when it was branched (2026-08-27):

test/registered/unit/disaggregation/test_nixl_backend_basic.py:168,185

test_nixl_backend_basic.py has since grown five more, added by #34977 (2018c64e22, 2026-09-11) — after #36612's last push on 2026-09-09:

test/registered/unit/disaggregation/test_nixl_backend_basic.py:205,226,246,264,284

These are not textual conflicts, so git merge succeeds silently and base-a-test-cpu then goes red with:

TypeError: 'bool' object is not callable

Verified with a local trial merge of #36612 into current main: Automatic merge went well, then five is_dummy() call sites remain.

Modifications

Convert all seven info.is_dummy() assertions in test_nixl_backend_basic.py to field reads. No other change — the production is_dummy() call sites in nixl/conn.py are #36612's to convert.

All seven (not just the five #36612 misses) so the file is self-consistent whichever order the two land in; the two that overlap change to identical content, so the merge stays clean either way.

Scope check

I audited the rest of the disaggregation unit tests against #36612's other behavioral changes (the rewritten CommonKVManager.update_status policy, the deleted NIXL update_status override, and the new settle-then-raise barrier in transfer_worker). Nothing else breaks:

  • TestNixlUpdateStatus — both cases (Failed is terminal; a missing room is not resurrected by Failed) are exactly what the new shared policy implements.
  • TestNixlTransferWorker — builds TransferInfo by keyword without is_dummy, which picks up the new = False default and matches the old computed value; its check_xfer_state stub returns DONE, so _await_handles settles on the first pass and the worker never enters the new failure path.
  • TestNixlNodeFailure, TestNixlNotifications, TestNixlReceiverPoll — unaffected.
  • test_nixl_deferred_kv_release.py, test_receiver_connection_pool.py, test_decode_queue_cleanup.py, test_kv_transfer_replica_metric.py — all mock or stub update_status / failure_exception.
  • test_nixl_sender_failure_cleanup.py — covers NixlKVSender.failure_exception; [PD] Share the prefill->decode failure notification across backends #36612 only changes the receiver's.
  • test_disaggregation_wire.py — its is_dummy assertions are mooncake's, already a field.

Checklist

  • Format check passes (whitespace-neutral: only () removed; every line stays well under the line limit)
  • No production code touched

🤖 Generated with Claude Code


CI States

Latest PR Test (Base): ❌ Run #34615153783
Latest PR Test (Extra): ❌ Run #34615153400
Latest PR Test (AMD ROCm 10): ❌ Run #34615153703

ShangmingCai and others added 2 commits September 11, 2026 23:05
#36612 turns `TransferInfo.is_dummy` from a method into a bool dataclass
field computed in `from_zmq()`. It updates the two call sites that existed
when it was branched; `test_nixl_backend_basic.py` has since grown five
more (added by #34977), and they are not textual conflicts, so the merge
succeeds silently and every one of them then raises
`TypeError: 'bool' object is not callable`.

Convert all seven assertions in the file to field reads. The two that
#36612 also touches change to identical content, so whichever lands first
the merge stays clean.

MUST MERGE AFTER #36612 -- `is_dummy` is still a method until it lands.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ShangmingCai

Copy link
Copy Markdown
Collaborator Author

/rerun-test test/registered/unit/disaggregation/test_nixl_backend_basic.py

@github-actions

github-actions Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Results for /rerun-test test/registered/unit/disaggregation/test_nixl_backend_basic.py:

🚀 ubuntu-latest (1 test): ✅ View workflow run

cd test/ && python3 registered/unit/disaggregation/test_nixl_backend_basic.py

@ShangmingCai
ShangmingCai merged commit 2c10f87 into main Sep 11, 2026
112 of 124 checks passed
@ShangmingCai
ShangmingCai deleted the fix/nixl-is-dummy-field branch September 11, 2026 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant