Repository navigation
[ROCm][CI] Fix NIXL spec-decode acceptance startup and diagnostics - #41313
Conversation
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the NIXL integration tests by introducing a side-channel probe, parameterizing hostnames, and improving server readiness checks to prevent orphaned processes. It also refines diagnostic logging and error reporting within the NIXL worker. Review feedback recommends using more specific exception types for runtime configuration mismatches and ensuring IPv6 compatibility when constructing URLs in shell scripts.
| raise AssertionError( | ||
| "All kv cache tensors must have the same number of " | ||
| f"blocks; layer={layer_name}, " | ||
| f"expected_num_blocks={num_blocks}, " | ||
| f"cache_shape={tuple(cache.shape)}, " | ||
| f"cache_stride={tuple(cache.stride())}, " | ||
| f"layer_spec={type(layer_spec).__name__}, " | ||
| f"backend={self.backend_name}, " | ||
| "all_backends=" | ||
| f"{[backend.get_name() for backend in self.attn_backends]}, " | ||
| f"kv_cache_layout={self.kv_cache_layout}, " | ||
| "blocks_first=" | ||
| f"{self.transfer_topo.is_kv_layout_blocks_first}" | ||
| ) |
There was a problem hiding this comment.
While improving diagnostics is beneficial, raising a generic AssertionError with a manually constructed string is generally discouraged for runtime configuration mismatches that could be triggered by user-provided parameters (like block size or attention backend). Consider using a more specific exception type like RuntimeError or ValueError to distinguish this from internal logic bugs, which is what AssertionError typically represents in Python.
| echo "FAIL: ${server_name} process ${server_pid} exited with status ${status} before port ${port} became ready" | ||
| exit 1 | ||
| fi | ||
| if curl -s "http://${SERVER_HOST}:${port}${endpoint}" > /dev/null 2>&1; then |
There was a problem hiding this comment.
The curl command uses http://${SERVER_HOST}:${port}${endpoint}. If SERVER_HOST is ever overridden with an IPv6 address (e.g., ::1), this URL will be malformed for curl because IPv6 addresses must be enclosed in square brackets. While the current default is 127.0.0.1, it is safer to handle potential IPv6 overrides to ensure robustness in different CI environments.
| if curl -s "http://${SERVER_HOST}:${port}${endpoint}" > /dev/null 2>&1; then | |
| local url="http://${SERVER_HOST}:${port}${endpoint}" | |
| if [[ "$SERVER_HOST" =~ : ]]; then | |
| url="http://[${SERVER_HOST}]:${port}${endpoint}" | |
| fi | |
| if curl -s "$url" > /dev/null 2>&1; then |
AndreasKaratzas
added
the
ready
…llm-project#41313) Signed-off-by: Andreas Karatzas <akaratza@amd.com>
…llm-project#41313) Signed-off-by: Andreas Karatzas <akaratza@amd.com>
ATTENTION_BACKEND=ROCM_ATTNoverride in AMD Buildkite for the NIXL speculative-decoding acceptance test.127.0.0.1and setVLLM_NIXL_SIDE_CHANNEL_HOSTexplicitly so HTTP, proxy traffic, and NIXL side-channel metadata use the same loopback address family.Scope Notes
HTTP readiness is not enough for NIXL. The failing logs showed
/v1/completionsreturning 200, then decode later timing out in_nixl_handshake()while waiting for prefill metadata on the side-channel socket. The new readiness check sends the same NIXLget_meta_msgrequest over ZMQ that decode needs, so the test does not start until the side channel is actually answering.localhostcan resolve through IPv6 while the proxy path is already intentionally bound to127.0.0.1for CI. This test now uses127.0.0.1consistently and passes that host into the Python test for proxy and metrics access.cc @kenroche