feat(asap-tools): separate clickhouse ingest and query cpu/memory monitoring in `experiment run clickhouse` by akanksha-akkihal · Pull Request #522 · ProjectASAP/ASAPQuery

akanksha-akkihal · 2026-07-12T01:52:09Z

Summary

Separate ClickHouse bulk-ingest CPU/memory monitoring from the query-phase monitor in experiment_run_clickhouse.
Baseline ingest writes baseline/remote_monitor_output/monitor_output_ingest.json and stops via a stop file (with kill fallback); query phase still writes monitor_output.json.
SketchDB keeps a single continuous monitor that waits flow.steady_state_wait before the query client so precompute + query land in one time series (with bare-metal thread attribution).
Harden stop/matching: poll during process walks, escalate stuck samplers with kill(), match remote_monitor.py by --execution_mode + experiment dir, and avoid writing null monitor JSON on timeout.

…oring Baseline bulk load now runs under a dedicated ingest monitor that writes monitor_output_ingest.json and stops via a stop file, so ingest CPU/memory is not mixed with the query-phase monitor_output.json. Co-authored-by: Cursor <cursoragent@cursor.com>

Match remote_monitor by execution mode and experiment dir, avoid writing null monitor JSON, escalate stuck sampler with kill(), and clarify ingest stop-file semantics. Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

milindsrivastava1997

Does experiment_run_e2e.py need any changes? Has that script been run once with the new remote monitor to verify?

milindsrivastava1997 · 2026-07-14T00:19:24Z

        backend_tool: Optional[str] = None,
        timed_duration: Optional[int] = None,
+        pre_query_wait_seconds: int = 0,
+        monitor_interval_seconds: float = 1.0,


If this is defined in config.yaml, can we remove the default argument here

milindsrivastava1997 · 2026-07-14T00:20:02Z

        self,
        minimum_experiment_running_time: int,
        polling_interval: int = 10,
+        timeout: int = 600,


do we need this default arg?

milindsrivastava1997 · 2026-07-14T00:20:13Z



-def stop_monitor(monitor, control_pipe, monitor_pipe):
+def stop_monitor(monitor, control_pipe, monitor_pipe, timeout=30):


do we need this default arg

milindsrivastava1997 · 2026-07-14T00:21:38Z

        backend_protocol: str,
        backend_tool: Optional[str] = None,
        timed_duration: Optional[int] = None,
+        pre_query_wait_seconds: int = 0,


do we need this default arg if the cmd line args already have a default value

milindsrivastava1997 · 2026-07-14T00:21:59Z

+    parser.add_argument(
+        "--monitor_interval_seconds",
+        type=float,
+        default=1.0,


if this is defined in config.yaml, do we need this default arg?

milindsrivastava1997 · 2026-07-14T00:50:17Z

Comments from CR with Claude:

Hard violations

remote_monitor.py — the new ingest execution-mode installs global SIGTERM/SIGINT handlers that just set a threading.Event() and never exit/restore. This means the SIGTERM the PR's own new kill-escalation path sends to stuck samplers gets swallowed, so the "force
kill" safety net it adds can itself hang.
remote_monitor_service.py::wait_for_remote_monitor_to_finish — gained an unconditional timeout=600 + force-kill escalation with no execution_mode/dir filter by default. This method is also called from the untouched experiment_run_e2e.py, so its "wait forever"
semantics silently become "pkill any process matching remote_monitor.py after 10 min" for callers outside this PR's scope.

Magic numbers

constants.py was touched (added INGEST_MONITOR_STOP_FILE) but the new timeout/poll literals (30, 60, 300, 10, 5, 2, 0.5s) scattered across the three other files stay inline, breaking the file's existing pattern of named interval constants.
process_monitor.stop_monitor(..., timeout=30) only threads timeout into the first poll; the two .join() calls hardcode 10 and the kill escalation hardcodes 5 — inconsistent within one new function.

Implemented but wrong

Manual mode is broken for the ingest monitor. _run_clickhouse_ingest_with_monitor unconditionally calls wait_for_remote_monitor_start(...) right after start_clickhouse_ingest_monitor(..., manual_mode=...), but unlike existing manual-mode call sites in this file
(which guard with if not manual_remote_monitor), the ingest path never pauses for the user — so cfg.manual.remote_monitor=true will always raise RuntimeError after 30s, before the user can react. This directly contradicts the "harden" intent of the PR by breaking an
existing workflow.
Kill-fallback can cause total data loss. Because the monitor is a forked multiprocessing.Process, pkill -f in kill_remote_monitor() can SIGTERM the sampler child directly before the control_pipe stop handshake completes, leaving monitor_info=None. The null-JSON
guard then deletes any partial monitor_output_ingest.json — so "escalate stuck samplers with kill()" can produce zero data instead of partial data

milindsrivastava1997 · 2026-08-17T22:14:29Z

Hard violation #2 (unconditional timeout=600 + force-kill escalation hitting experiment_run_e2e.py): rather than threading execution_mode/experiment_output_dir scoping into the e2e.py call (which still leaves a "kill after N minutes" semantic that didn't exist before this PR), we removed the force-kill escalation from wait_for_remote_monitor_to_finish() entirely. It's back to its original pre-PR behavior — poll forever, no timeout, no kill — just with the polling now going through the new is_remote_monitor_running() helper. This resolves the issue at the source instead of papering over it with scoping, and doesn't introduce a new "wait forever" → "kill after 10 min" behavior change for callers outside this PR's scope.

Hard violation #1 (SIGTERM/SIGINT handler swallowing): Fix: restore SIG_DFL for SIGTERM/SIGINT right after the ingest loop exits, so a later signal actually terminates the process again.

akanksha-akkihal and others added 2 commits July 11, 2026 21:39

fix(asap-tools): harden ingest-monitor stop and process matching

686981e

Match remote_monitor by execution mode and experiment dir, avoid writing null monitor JSON, escalate stuck sampler with kill(), and clarify ingest stop-file semantics. Co-authored-by: Cursor <cursoragent@cursor.com>

akanksha-akkihal linked an issue Jul 12, 2026 that may be closed by this pull request

feat(asap-tools): Separate ClickHouse ingest and query CPU/memory monitoring in experiment_run_clickhouse #521

Open

style(asap-tools): satisfy black formatting in experiment_run_clickhouse

f5ee516

Co-authored-by: Cursor <cursoragent@cursor.com>

akanksha-akkihal requested a review from milindsrivastava1997 July 12, 2026 02:27

milindsrivastava1997 requested changes Jul 14, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat(asap-tools): separate clickhouse ingest and query cpu/memory monitoring in `experiment run clickhouse` - #522

feat(asap-tools): separate clickhouse ingest and query cpu/memory monitoring in `experiment run clickhouse`#522
akanksha-akkihal wants to merge 3 commits into
mainfrom
521-featasap-tools-separate-clickhouse-ingest-and-query-cpumemory-monitoring-in-experiment_run_clickhouse

akanksha-akkihal commented Jul 12, 2026

Uh oh!

milindsrivastava1997 left a comment

Uh oh!

milindsrivastava1997 Jul 14, 2026

Uh oh!

milindsrivastava1997 Jul 14, 2026

Uh oh!

milindsrivastava1997 Jul 14, 2026

Uh oh!

milindsrivastava1997 Jul 14, 2026

Uh oh!

milindsrivastava1997 Jul 14, 2026

Uh oh!

milindsrivastava1997 commented Jul 14, 2026

Uh oh!

milindsrivastava1997 commented Aug 17, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants



		def stop_monitor(monitor, control_pipe, monitor_pipe):
		def stop_monitor(monitor, control_pipe, monitor_pipe, timeout=30):

Uh oh!

Conversation

akanksha-akkihal commented Jul 12, 2026

Summary

Uh oh!

milindsrivastava1997 left a comment

Choose a reason for hiding this comment

Uh oh!

milindsrivastava1997 Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

milindsrivastava1997 Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

milindsrivastava1997 Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

milindsrivastava1997 Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

milindsrivastava1997 Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

milindsrivastava1997 commented Jul 14, 2026

Hard violations

Magic numbers

Implemented but wrong

Uh oh!

milindsrivastava1997 commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

milindsrivastava1997 commented Aug 17, 2026 •

edited

Loading