You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[https://nvbugs/7003528][fix] Align ADP connector fixtures with executor contracts - #21465
The ADP connector tests construct PyExecutor without its initializer. Their fixtures no longer supplied all state used by cleanup and transfer handling, and their queue-pop mocks still returned a request list instead of the current (requests, placement) pair.
The loop tests also installed an unconstrained disaggregation coordinator mock. Its truthy handle_errors_synced() result falsely selected an early response flush even though these tests configure no KV cache transceiver. Removing that mock lets the executor select its real no-transceiver coordinator, which returns False for this operation.
Initialize the serving response mode and optional executor resources used by the tests.
Return ([], None) from the empty queue-pop mocks.
Exercise the real disabled-transceiver coordinator rather than inventing transfer errors.
Remove the module's waiver for this bug.
All existing assertions, exception checks, parameterizations and timeout values are retained.
Verification
ARM64 CPU verification on base cbef70dbc9879798b4106c694928d59976cf0ac1, with the same native artifacts for both arms:
pytest -m cpu_only -p no:xdist tests/unittest/_torch/test_connector_adp.py: 39 failed / 97 passed before; 136 passed after, with no skipped tests.
pytest -m cpu_only -p no:xdist tests/unittest/_torch/test_connector_shared_pool.py: 7 passed on both arms.
The fixed module also passes 136 tests with the CPU-stage test prefix and the updated waiver file supplied explicitly.
Configured pre-commit checks pass for both changed files.
The fixture now initializes the PyExecutor response-mode and optional-resource fields, and empty waiting-queue mocks return ([], None). The tests use the real no-transceiver coordinator instead of a mock that reports disaggregation errors. These changes align the fixtures with executor contracts. No production behavior changes are reported.
The waiver for test_connector_adp.py was removed. The test remains in the CPU CI list. The author reports that all 136 ADP tests passed on ARM64 CPU after the changes, and that pre-commit checks passed for both changed files.
QA Engineer Review
The PR modifies one test module and one waiver list. It changes fixture setup and queue-pop mocks; it does not add or remove test functions or parameterizations. The module continues to cover ADP connector behavior, failure handling, recovery, cancellation, transfer deadlines, and shutdown paths.
The test is listed in tests/integration/test_lists/test-db/l0_cpu.yml. The supplied evidence does not show a manual-QA list entry. Coverage verdict: sufficient, based on the reported 136 passing tests and the existing CI listing.
Per-File QA Perspective
tests/unittest/_torch/test_connector_adp.py: Covers ADP connector and executor behavior, including recovery and error paths. Listed in the CPU CI test list; the author reports 136 passing tests.
tests/integration/test_lists/waives.txt: Removes the waiver for the ADP test module, allowing it to run without that exclusion. The test remains listed in CPU CI.
…tor contracts
Initialize the optional executor state used by cleanup and transfer paths,
and return the requests/placement pair from the queue-pop mocks. Let the
executor select its real no-transceiver coordinator instead of installing
a truthy mock that incorrectly reports disaggregation errors.
Retain the existing assertions and remove this module's bug waiver.
Validation on ARM64 CPU at base cbef70d:
- test_connector_adp.py: 39 failed / 97 passed before, 136 passed after.
- test_connector_shared_pool.py: 7 passed before and after.
- The fixed module also passes all 136 cases with the waiver file enabled.
Fixes https://nvbugs/7003528
Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
No actionable comments were generated in the recent review.
Walkthrough
The connector tests add executor state defaults, update waiting-queue mock results, and remove two disaggregation coordinator initializations. The integration skip entry for this test file is removed.
Changes
Connector test updates
Layer / File(s)
Summary
Update connector tests and enable integration coverage tests/unittest/_torch/test_connector_adp.py, tests/integration/test_lists/waives.txt
Executor setup initializes additional attributes. Two tests no longer initialize _disagg_coordinator, and waiting-queue mocks return ([], None). The integration skip entry is removed.
No merge-blocking issue is identified in these fixture and waiver changes; merge after normal checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Title check
The title names the NVBugs ID and the [fix] type, and it describes the main change: aligning ADP connector test fixtures with current executor contracts. The title is 82 characters, which is longer th…
Description check
The description covers the required template elements. It gives the problem and solution in short form, lists the verification performed (test counts before and after, the related test module, and pre…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the NVBugs ID and the [fix] type, and it describes the main change: aligning ADP connector test fixtures with current executor contracts. The title is 82 characters, which is longer than ideal, but it is specific and relevant to the changeset.
Full details: Description check
Explanation
The description covers the required template elements. It gives the problem and solution in short form, lists the verification performed (test counts before and after, the related test module, and pre-commit checks), and links the NVBugs ID in the Fixes line. The template's PR checklist is not ticked, and the Test Coverage section is written as verification results rather than a list of safeguarding tests. These gaps are minor, so the check passes.
Full details: Docstring Coverage
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 1 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Reviewing files that changed from the base of the PR and between bc77820 and 5de2aa4.
Files selected for processing: 2
tests/integration/test_lists/waives.txt
tests/unittest/_torch/test_connector_adp.py
Files with no reviewable changes: 2
tests/integration/test_lists/waives.txt
tests/unittest/_torch/test_connector_adp.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Approving — the comments below are optional touch-ups, not blockers.
Removing the _disagg_coordinator mock is the key fix. PyExecutor.disagg only builds the coordinator when __dict__ has none, so with kv_cache_transceiver = None the tests now run the real NoopDisaggCoordinator and no longer depend on a MagicMock's truthy return value. One optional fidelity nit is inline.
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements.
…in the ADP batch-start test
_can_queue reads self.model_engine.cuda_graph_runner (NVIDIA#20078). The test
builds its executors with object.__new__, and its file was waived when
NVIDIA#20078 stubbed the model engine for the other _can_queue tests, so
un-waiving it (NVIDIA#21465) left it without one. Give it the same stub.
Signed-off-by: Kaiyu Xie <26294424+kaiyux@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
brnguyen2
left a comment
Summary
The ADP connector tests construct
PyExecutorwithout its initializer. Their fixtures no longer supplied all state used by cleanup and transfer handling, and their queue-pop mocks still returned a request list instead of the current(requests, placement)pair.The loop tests also installed an unconstrained disaggregation coordinator mock. Its truthy
handle_errors_synced()result falsely selected an early response flush even though these tests configure no KV cache transceiver. Removing that mock lets the executor select its real no-transceiver coordinator, which returnsFalsefor this operation.([], None)from the empty queue-pop mocks.All existing assertions, exception checks, parameterizations and timeout values are retained.
Verification
ARM64 CPU verification on base
cbef70dbc9879798b4106c694928d59976cf0ac1, with the same native artifacts for both arms:pytest -m cpu_only -p no:xdist tests/unittest/_torch/test_connector_adp.py: 39 failed / 97 passed before; 136 passed after, with no skipped tests.pytest -m cpu_only -p no:xdist tests/unittest/_torch/test_connector_shared_pool.py: 7 passed on both arms.Fixes https://nvbugs/7003528
Dev Engineer Review
The fixture now initializes the
PyExecutorresponse-mode and optional-resource fields, and empty waiting-queue mocks return([], None). The tests use the real no-transceiver coordinator instead of a mock that reports disaggregation errors. These changes align the fixtures with executor contracts. No production behavior changes are reported.The waiver for
test_connector_adp.pywas removed. The test remains in the CPU CI list. The author reports that all 136 ADP tests passed on ARM64 CPU after the changes, and that pre-commit checks passed for both changed files.QA Engineer Review
The PR modifies one test module and one waiver list. It changes fixture setup and queue-pop mocks; it does not add or remove test functions or parameterizations. The module continues to cover ADP connector behavior, failure handling, recovery, cancellation, transfer deadlines, and shutdown paths.
The test is listed in
tests/integration/test_lists/test-db/l0_cpu.yml. The supplied evidence does not show a manual-QA list entry. Coverage verdict: sufficient, based on the reported 136 passing tests and the existing CI listing.Per-File QA Perspective
tests/unittest/_torch/test_connector_adp.py: Covers ADP connector and executor behavior, including recovery and error paths. Listed in the CPU CI test list; the author reports 136 passing tests.tests/integration/test_lists/waives.txt: Removes the waiver for the ADP test module, allowing it to run without that exclusion. The test remains listed in CPU CI.