Add CPU ClickHouse host scaling benchmarks - #104
Conversation
There was a problem hiding this comment.
Pull request overview
Hardens native offload correctness and expands ClickHouse benchmarking and telemetry.
Changes:
- Adds atomic ring reservation, safer publication, shutdown, and failure propagation.
- Adds ClickHouse worker metrics, readiness, socket timeouts, and scaling sweeps.
- Updates tests and architecture/benchmark documentation.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_engine_runtime_api.py |
Tests flush defaults and cleanup errors. |
tests/test_cpu_native_build.py |
Tests batching and metrics defaults. |
tests/test_clickhouse_host_benchmark.py |
Tests benchmark telemetry and sweeps. |
tests/native/ring/test_ring_engine.cu |
Tests reservation and failure propagation. |
tests/native/ring/test_producer.cu |
Tests descriptor publication. |
tests/native/ring/Makefile |
Links P2P implementation in tests. |
src/dmi/transport/ring.py |
Updates safety-net documentation. |
src/dmi/hooks/point.py |
Uses atomic ring reservations. |
src/dmi/engine.py |
Propagates shutdown failures. |
native/csrc/ring/task_ring.cuh |
Uses system-scope publication fencing. |
native/csrc/ring/task_entry.h |
Updates publication protocol documentation. |
native/csrc/ring/ring_engine.h |
Tracks engine start state. |
native/csrc/ring/ring_engine.cu |
Hardens ring shutdown. |
native/csrc/ring/ring_engine_py.h |
Revises reservation API. |
native/csrc/ring/ring_engine_py.cu |
Adds checked CUDA and atomic capacity handling. |
native/csrc/ring/ring_config.h |
Changes flush defaults. |
native/csrc/ring/p2p_thread.cpp |
Propagates processing failures. |
native/csrc/ring/drain_thread.h |
Adds snapshots and failure state. |
native/csrc/ring/drain_thread.cpp |
Hardens draining and D2H commits. |
native/csrc/dmx_host_engine.h |
Adds readiness and metrics. |
native/csrc/clickhouse_client.h |
Defines timeout and metric APIs. |
native/csrc/clickhouse_client.cpp |
Implements worker telemetry and timeouts. |
native/csrc/bindings.cpp |
Exposes new native APIs. |
docs/integration-api-v1.md |
Documents updated public APIs. |
docs/config.md |
Updates ring configuration guidance. |
docs/clickhouse-offload-pipeline.html |
Adds architecture overview. |
docs/benchmarks.md |
Documents scaling workflow. |
benchmarks/bench_clickhouse_host.py |
Implements telemetry and parallelism sweeps. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
benchmarks/bench_clickhouse_host.py:449
- The process CPU window does not match the throughput window:
cpu_startis captured before the sampler's synchronous initial queries, whiletotal_startis captured afterward; similarly,cpu_endis captured after the final sampler query althoughtotal_endis captured before it. This includes telemetry setup/teardown CPU incpu_secondsbut excludes its elapsed time fromeffective_cpu_cores, inflating that metric, especially for short trials. Capture both CPU endpoints inside the sametotal_start/total_endboundaries.
cpu_start = time.process_time()
if sampler is not None:
sampler.start()
sampler_started = True
total_start = clock()
benchmarks/bench_clickhouse_host.py:410
- A positive timeout below 0.5 ms rounds to
0, which the native API defines as unbounded. That defeats the benchmark's bounded-socket guarantee for an input accepted byBenchmarkConfig; round upward and clamp to at least one millisecond.
timeout_ms = round(config.socket_timeout_seconds * 1000)
|
|
||
|
|
||
| class ServerTelemetrySampler: | ||
| _PROCESS_QUERY = "SELECT count() FROM system.processes WHERE query_kind = 'Insert'" |
There was a problem hiding this comment.
Does this really matter?
There was a problem hiding this comment.
@copilot Fix the code for all comments in this review comment.
When a review comment includes a suggested change, apply the suggestion exactly.
Do not make changes beyond what is described in the linked review comment.
There was a problem hiding this comment.
Yes — on a shared server, unrelated insert workloads inflate peak_active_inserts, making it impossible to tell whether the requested worker concurrency was actually realized.
Fixed in the latest commit: ServerTelemetrySampler.__init__ now accepts database and table parameters and builds the process query with AND has(tables, %(table)s) when they are provided. The sampler construction site passes config.database and config.table.
| bool wait_until_ready(Duration timeout) { | ||
| if (timeout.count() < 0.0) { | ||
| throw std::invalid_argument("timeout must be non-negative"); | ||
| } |
There was a problem hiding this comment.
@copilot Fix the code for all comments in this review comment.
When a review comment includes a suggested change, apply the suggestion exactly.
Do not make changes beyond what is described in the linked review comment.
There was a problem hiding this comment.
Fixed in the latest commit. All three timeout_s lambdas (wait_until_ready, stop, join) now call std::isfinite before constructing the Duration and throw std::invalid_argument("timeout_s must be finite and non-negative") for NaN or infinite inputs.
Co-authored-by: zaoxing <[email protected]>
Co-authored-by: zaoxing <[email protected]>
Summary
Validation
make test-host— 47 passedmake check— 198 CPU tests and package-layout check passedScope
This draft intentionally contains no CUDA, GPU ring, or native ring-test changes.
Notes
Draft pending a live ClickHouse benchmark run; no local server was available during final verification.