test: tcp test enable #4843
test: tcp test enable #4843
Conversation
WalkthroughThe changes introduce parameterized request plane selection ("nats" and "tcp") across multiple test suites via pytest fixtures and environment variable propagation. New fixtures are added to conftest.py files at different scopes, and test files are updated to run with both request planes by passing the selected plane through environment variables to worker processes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks❌ Failed checks (2 warnings, 1 inconclusive)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/bindings/python/tests/cancellation/test_cancellation.py (1)
168-168: Consider consolidating parametrization to module level.The
request_planeparametrization is repeated on 6 individual tests. Since all tests in this file require the same parametrization, consider moving it topytestmarkfor consistency with other test files in this PR and to reduce duplication.pytestmark = [ pytest.mark.pre_merge, pytest.mark.parametrize("request_plane", ["nats", "tcp"], indirect=True), ]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
lib/bindings/python/tests/cancellation/test_cancellation.py(6 hunks)lib/bindings/python/tests/conftest.py(3 hunks)tests/conftest.py(1 hunks)tests/fault_tolerance/cancellation/test_sglang.py(2 hunks)tests/fault_tolerance/cancellation/test_trtllm.py(2 hunks)tests/fault_tolerance/cancellation/test_vllm.py(2 hunks)tests/fault_tolerance/cancellation/utils.py(1 hunks)tests/fault_tolerance/migration/test_sglang.py(2 hunks)tests/fault_tolerance/migration/test_trtllm.py(2 hunks)tests/fault_tolerance/migration/test_vllm.py(2 hunks)tests/fault_tolerance/migration/utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T17:45:54.735Z
Learnt from: tzulingk
Repo: ai-dynamo/dynamo PR: 3194
File: tests/utils/managed_deployment.py:620-656
Timestamp: 2025-09-24T17:45:54.735Z
Learning: In the fault tolerance test infrastructure for Dynamo, port-forwards created by the `port_forward` method in `ManagedDeployment` are intended to run in the background and should not be stopped immediately after use. They are designed for persistent connections that can be reused across multiple metrics collections during test execution.
Applied to files:
tests/fault_tolerance/migration/utils.py
🧬 Code graph analysis (1)
lib/bindings/python/tests/conftest.py (2)
lib/runtime/src/distributed.rs (2)
request_plane(382-384)runtime(288-290)lib/bindings/python/src/dynamo/_core.pyi (1)
DistributedRuntime(36-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (25)
tests/fault_tolerance/cancellation/utils.py (1)
30-30: LGTM!The request plane fixture value is correctly propagated to the frontend process environment, enabling the test to run with both "nats" and "tcp" configurations.
tests/fault_tolerance/migration/test_sglang.py (2)
31-31: LGTM!The indirect parametrization correctly routes the request plane values through the fixture mechanism, enabling tests to run with both "nats" and "tcp" request planes.
60-63: LGTM!The environment variable setup correctly propagates the request plane fixture value to the worker process, maintaining consistency with other worker process initializations in this PR.
tests/fault_tolerance/cancellation/test_trtllm.py (2)
29-29: LGTM!The indirect parametrization is correctly configured for TRT-LLM cancellation tests to run with both request plane configurations.
89-93: LGTM!The environment variable setup is consistent with other worker process initializations. The updated comment accurately reflects the broader scope of environment configuration.
tests/fault_tolerance/migration/test_trtllm.py (2)
31-31: LGTM!The indirect parametrization is correctly configured for TRT-LLM migration tests.
58-60: LGTM!The environment variable propagation is consistent with the established pattern across the test suite.
lib/bindings/python/tests/cancellation/test_cancellation.py (5)
202-203: LGTM!The parametrization correctly enables dual request plane testing for this test.
235-236: LGTM!
260-261: LGTM!
289-290: LGTM!
312-313: LGTM!tests/fault_tolerance/cancellation/test_vllm.py (2)
28-28: LGTM!The module-level parametrization correctly enables dual request plane testing for all vLLM cancellation tests.
69-73: LGTM!The environment variable setup is consistent with other worker process initializations in the test suite.
tests/fault_tolerance/migration/test_vllm.py (2)
31-31: LGTM!The module-level parametrization correctly enables dual request plane testing for vLLM migration tests.
57-60: LGTM!The environment variable setup is consistent with other worker process initializations.
tests/fault_tolerance/migration/utils.py (1)
27-34: LGTM!The frontend process correctly propagates the request plane fixture value to the environment. The organization of environment variable setup is clean and consistent with the cancellation utils.
tests/fault_tolerance/cancellation/test_sglang.py (2)
28-28: LGTM: Correct parameterization for multi-transport testing.The indirect parameterization will correctly pass "nats" and "tcp" values to the
request_planefixture, enabling these tests to run with both transport planes.
93-95: LGTM: Correct propagation of request plane to worker environment.The environment variable
DYN_REQUEST_PLANEis correctly set from the parameterized fixture, enabling worker processes to use the selected transport plane (nats or tcp).lib/bindings/python/tests/conftest.py (3)
405-415: LGTM: Correct fixture implementation for KV store selection.The
store_kvfixture correctly implements the indirect parametrization pattern with a sensible default of "file" for unit tests.
418-428: LGTM: Correct fixture implementation for request plane selection.The
request_planefixture correctly implements the indirect parametrization pattern with a default of "nats".
432-432: TheDistributedRuntimeconstructor signature is confirmed to acceptstore_kvandrequest_planeparameters. Evidence fromlib/bindings/python/src/dynamo/runtime/__init__.pyshows the same usage pattern:DistributedRuntime(loop, "etcd", request_plane). The fixture dependency injection at line 472 is correct, withstore_kvandrequest_planefixtures providing appropriate string values ("file"/"etcd" and "nats"/"tcp" respectively).tests/conftest.py (3)
415-425: LGTM: Correct fixture implementation for KV store selection.The
store_kvfixture correctly implements the indirect parametrization pattern with a default of "etcd", which is appropriate for integration and end-to-end tests.
428-438: LGTM: Correct fixture implementation for request plane selection.The
request_planefixture correctly implements the indirect parametrization pattern with a default of "nats".
442-460: LGTM: Correct conditional service startup based on test configuration.The
runtime_servicesfixture correctly starts only the required services based onstore_kvandrequest_planeparameters. This optimization allows TCP-based tests to skip NATS startup and file-based KV tests to skip etcd startup, improving test efficiency.The conditional logic properly handles all combinations:
- Both NATS and etcd when both are needed
- Only NATS for nats transport with non-etcd KV
- Only etcd for non-nats transport with etcd KV
- Neither when using TCP transport with file-based KV
kthui
force-pushed
the
jacky-tcp-test-enable
branch
from
90e954c to
52e1ecf
Compare
Overview:
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Tests
✏️ Tip: You can customize this high-level summary in your review settings.