[NIXL] ASIO support - #996
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 10 remain after this review. 📝 WalkthroughWalkthroughAdds ETCD, ASIO, and null-runtime support to NIXLBench. Updates environment and Slurm command generation, adds runtime validation and lifecycle tests, and introduces POSIX, UCX, network, and storage smoke-test configurations with documentation. ChangesNIXLBench runtime support
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: ⚪ Minimal · up to The PR adds ASIO support and adjusts ETCD startup behavior; no actionable merge-blocking risk remains based on the supplied evidence, so it is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cloudai/workloads/nixl_bench/nixl_bench.py (1)
37-79: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject empty ETCD endpoints for non-storage backends.
When
runtime_typeisETCDandetcd_endpointsis empty,uses_etcdreturnsFalsewithout checkingbackend. A UCX benchmark can therefore launch two processes without ETCD or an ETCD endpoint. Add model-level validation and a regression test that rejects UCX with emptyetcd_endpoints.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cloudai/workloads/nixl_bench/nixl_bench.py` around lines 37 - 79, Update NIXLBenchCmdArgs model validation to reject configurations with backend UCX, runtime_type ETCD, and empty etcd_endpoints, while preserving valid managed-ETCD and ASIO configurations. Add a regression test covering this invalid combination and asserting model validation fails; use the existing NIXLBenchCmdArgs and NIXLBenchTestDefinition symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/cloudai/workloads/nixl_bench/nixl_bench.py`:
- Around line 37-79: Update NIXLBenchCmdArgs model validation to reject
configurations with backend UCX, runtime_type ETCD, and empty etcd_endpoints,
while preserving valid managed-ETCD and ASIO configurations. Add a regression
test covering this invalid combination and asserting model validation fails; use
the existing NIXLBenchCmdArgs and NIXLBenchTestDefinition symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d255cc06-fd0f-42b9-a3d4-f9055087603c
📒 Files selected for processing (9)
conf/experimental/test/nixl-bench-posix.tomlconf/experimental/test/nixl-bench-ucx.tomlconf/experimental/test_scenario/nixl-bench-network.tomlconf/experimental/test_scenario/nixl-bench-storage.tomldoc/workloads/nixl_bench.rstsrc/cloudai/workloads/common/nixl.pysrc/cloudai/workloads/nixl_bench/nixl_bench.pysrc/cloudai/workloads/nixl_bench/slurm_command_gen_strategy.pytests/workloads/nixl_bench/test_command_gen_strategy_slurm.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cloudai/workloads/nixl_bench/nixl_bench.py (2)
39-42: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not serialize ASIO-only fields for the null runtime.
When
runtime_typeis unset, the branch at Line 78 removes onlyetcd_endpoints. It leavesasio_addressandasio_portincmd_args. The Slurm generator serializes every entry fromcmd_args_dict, so a null-runtime command can receive ASIO arguments without--runtime_type=ASIO. This can break storage-only runs or apply ASIO settings to the wrong runtime.Remove the ASIO fields unless
self.uses_asiois true. Add a regression test for the unset-runtime path.Proposed fix
else: # ASIO performs direct peer-to-peer coordination and does not use ETCD endpoints. - cmd_args.pop("etcd_endpoints") + cmd_args.pop("etcd_endpoints", None) + if not self.uses_asio: + cmd_args.pop("asio_address", None) + cmd_args.pop("asio_port", None)Also applies to: 78-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cloudai/workloads/nixl_bench/nixl_bench.py` around lines 39 - 42, Update the command-argument construction around the runtime handling branch to remove both asio_address and asio_port whenever self.uses_asio is false, while preserving them for ASIO runtimes. Add a regression test covering an unset/null runtime and verify the generated cmd_args_dict excludes all ASIO-only fields.
90-93: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFallback when
nixlbench.csvis empty.
metric_observationsreads every existing CSV. A zero-byte CSV can raise during parsing, and a header-only CSV returns no observations even whenstdout.txtcontains valid rows. Use the CSV only when it contains data; otherwise fall back toextract_nixlbench_data(stdout.txt).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cloudai/workloads/nixl_bench/nixl_bench.py` around lines 90 - 93, Update metric_observations to use nixlbench.csv only when the file contains usable data; handle zero-byte or header-only CSVs without raising and fall back to extract_nixlbench_data(stdout.txt) when no observations are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/cloudai/workloads/nixl_bench/nixl_bench.py`:
- Around line 39-42: Update the command-argument construction around the runtime
handling branch to remove both asio_address and asio_port whenever
self.uses_asio is false, while preserving them for ASIO runtimes. Add a
regression test covering an unset/null runtime and verify the generated
cmd_args_dict excludes all ASIO-only fields.
- Around line 90-93: Update metric_observations to use nixlbench.csv only when
the file contains usable data; handle zero-byte or header-only CSVs without
raising and fall back to extract_nixlbench_data(stdout.txt) when no observations
are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: be3ceee8-9fb3-44f8-a126-e44116b1b176
📒 Files selected for processing (1)
src/cloudai/workloads/nixl_bench/nixl_bench.py
|
@rutayan-nv could you please take a look? (it's not urgent) |
rutayan-nv
left a comment
rutayan-nv
left a comment
There was a problem hiding this comment.
LGTM.
Two non-blocking notes:
-
ASIO process startup relies on a bare
sleep 4, while the ETCD path gets a real readiness loop (until curl ... /health). If the first process isn't listening within 4s, the second fails. Was 4 measured, or is a readiness check worth adding here too? -
The null-runtime mode (
etcd_endpoints = "") is documented as storage-only, but nothing enforces it. Combined with UCX it yields two processes with no--etcd-endpointsand no managed ETCD, so there's nothing to coordinate them. A validator would make that a config-time error.
|
3 participants
Summary
etcd_endpointsis set to an empty string)Test Plan
Additional Notes