[NIXL] ASIO support - #996
Conversation
📝 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 Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| assert cmd_args.total_buffer_size == expected_total_buffer_size | ||
|
|
||
|
|
||
| def test_gen_etcd_srun_command(nixl_bench_tr: TestRun, slurm_system: SlurmSystem): |
There was a problem hiding this comment.
a few tests have been removed as they duplicate the same code tested by the central e2e test
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) |
Summary
etcd_endpointsis set to an empty string)Test Plan
Additional Notes