Skip to content

CI baseline for the capture path: real sink chain, multipart on MinIO, backend compile, undefined-name lint - #145

Merged
zaoxing merged 13 commits into
mainfrom
ci/capture-chain-baseline
Sep 24, 2026
Merged

zaoxing merged 13 commits into
mainfrom
ci/capture-chain-baseline

Conversation

@zaoxing

@zaoxing zaoxing commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #143. Merge #143 first; GitHub then retargets this PR to main.

Changes

(a) The live job proves the real sink-to-reader chain. It now also builds build/_dmi_native_sink. The new tests/test_native_capture_chain_live.py drives the real NativePackSink (through its attach/submit_envelope entry points, with no CUDA and no conformance driver) → spool → in-process CaptureStorageService → ClickHouse plus object store → NativeCaptureReader, with byte-equal payloads. It has three cases:

  • f16, bf16, f32 and i64 rows in 3 packs, against the in-repo fake S3;
  • a 68 MiB single-pack multipart upload (5 parts) against the fake S3. The test asserts the part sizes the client sent, because the fake enforces no part-size rules;

(b) A new native-backend-compile job, which is the first time CI compiles _native_backend at all. On ubuntu-24.04 it:

  • frees disk;
  • initialises only the clickhouse-cpp submodule;
  • installs a minimal CUDA 13.0 from NVIDIA's apt repository (about 2.7 GB; the package list comes from the build's own dependency files);
  • installs torch 2.14 cu130 and builds clickhouse-cpp as install.md does;
  • runs make -C native all against the libcuda stub;
  • imports the backend;
  • builds the six tests/native/ring binaries and runs the two that need no device.

A step applies #138's C++20 flags by pattern until #138 merges. After that it matches nothing and asserts all three lines read c++20. Delete that step once #138 lands. A container image was rejected: the 3.7 GiB CUDA devel image plus about 4.4 GB of torch wheels doesn't reliably fit a hosted runner.

(c) Undefined-name lint. The cpu job runs ruff check --select F821 src/dmi. The tree had 14 existing violations, handled visibly in their own commit (f48becd), which you can drop if you prefer another fix:

  • src/dmi/config.py (2): MonitoringConfig's string annotations "NativeSinkConfig" and "NativeCaptureStorageConfig" were never imported. They are now imported under TYPE_CHECKING. Latent, not fixed here: typing.get_type_hints(MonitoringConfig) still raises NameError; B2's move of NativeSinkConfig into live code is the natural fix.
  • src/dmi/hooks/specs.py (12, on 10 lines): the HOOK_TYPE_* names are injected by a globals() loop the linter can't follow, so these are false positives. They're suppressed per line, not per file, so the rest of the module stays checked. The alternative, declaring the constants statically, is a maintainer call.

Evidence (local)

  • Chain tests: 2 passed, 0 skipped (after MinIO was removed; see the update below).
  • The tests catch a real break. With the client's multipart chunk forced to 4 MiB: the fake-S3 case fails on part count (18 vs 5).
  • Full live glob, as CI runs it: 211 passed, 1 failed. The failure is a role test that needs a CREATE USER grant this local server lacks. The live skip gate reports 0 skipped.
  • CPU tier: 2213 passed, 1 skipped (no CUDA device); the cpu skip gate is clean.
  • Backend build steps, rehearsed locally (C++20, CUDA 13.0, sm_89, libcuda stub): builds, links without libcuda, and imports with devices hidden. The stock C++17 flags fail with ATen's #error, as expected.
  • Lint: 14 violations at the base; clean at the head; an injected undefined name fails the step with a GitHub annotation.
  • An independent review re-verified all of the above and found no defects.

Not verifiable before CI runs: the compile job on the hosted runner (apt install, disk headroom, gcc 13, runtime under the 60-minute timeout), and pip install ruff there. Making these required checks on main is a branch-protection change I haven't made.

Since the independent review (2026-09-24)

An independent review found one major, three minors and some nits; all are fixed (eb79cfd..e68fea2), and one CI break was fixed after (a2033b3):

  • Major: the chain test now compares every catalog metadata field (all 21) against what was submitted. A mutant sink that stored step_number + 7 used to pass the chain test and all of CI; it now fails it.
  • The multipart part count is deduplicated by part number, so one transient HTTP retry cannot fail it.
  • The compile job now also builds _dmi_native_sink beside the backend and asserts the sink binds the backend's RecordSink (not stand-ins), through dmi's loaders and via a bare import. A pytest check alone stayed green with the binding broken, which is why the assertions are there.
  • The temporary C++20 step emits a ::warning:: once Build the torch-including native targets as C++20 #138 has landed and it has nothing left to change.
  • Comments no longer claim numbers or releases that were not observed.

MinIO removed: MinIO later withdrew its public images, and nothing in DMI runs against MinIO (its object store is Garage), so the MinIO step and test case were dropped (latest commit). The fake-S3 multipart case, which asserts the part sizes the client sends, remains; checks against a real store belong to the Garage suites.

Evidence before MinIO was removed (CI at a2033b3): live suite 219 passed, 0 skipped; native-backend-compile green, sink binding "1 passed" and 21 ring tests passed; CPU job 2234 passed.

Copilot AI lite review requested due to automatic review settings September 23, 2026 20:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@zaoxing
zaoxing requested a review from Samfisheryu September 23, 2026 20:17
@zaoxing
zaoxing force-pushed the ci/capture-chain-baseline branch 2 times, most recently from a3a1280 to e68fea2 Compare September 24, 2026 14:07
@zaoxing
zaoxing force-pushed the feat/native-capture-service branch from d14ca30 to b3dfb0d Compare September 24, 2026 14:46
Every live suite staged its packs through the conformance_sink driver,
which feeds the sink core JSON and base64, so the torch-facing
NativePackSink that the ring hands envelopes to never reached a catalog
in CI. tests/test_native_capture_chain_live.py drives it with torch CPU
tensors (through its attach/submit_envelope test entry points) into a
spool, runs CaptureStorageService in-process against ClickHouse and the
signature-verifying fake S3, and reads the payloads back through
NativeCaptureReader, requiring the bytes to match. No CUDA, no ring, no
conformance driver.

It also sends one 68 MiB pack up as a multipart upload, twice. The fake
S3 accepts parts of any size, takes the part list on trust and never
checks the completion ETags, so against it the test asserts the part
sizes the client sent. Against MinIO, S3's 5 MiB part floor is enforced
by the server; the MinIO case skips only when DMI_MINIO_ENDPOINT is
unset. The clickhouse-live job now builds build/_dmi_native_sink and
starts MinIO (quay.io image pinned by digest, via docker run because a
service container takes no command) and sets that variable, so its
skip gate turns a lost variable into a red job.

Evidence, local ClickHouse 127.0.0.1 and a MinIO binary extracted from
the pinned image on 127.0.0.1:19100: 3 passed; with the endpoint unset,
2 passed, 1 skipped. With the client's multipart chunk shrunk to 4 MiB
(not committed), the fake-S3 case failed on the part count (18 != 5)
and the MinIO case on CompleteMultipartUpload EntityTooSmall.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
Nothing in CI compiled _native_backend: the cpu job builds only the
CPU-only goals and dry-runs `host`, so the .cu sources, bindings.cpp
and the torch/CUDA link could break unnoticed, as the C++20 floor of
torch 2.14 did (#138). The new native-backend-compile job builds
clickhouse-cpp (docs/install.md step 5), runs `make -C native all`,
imports the result, compiles all six tests/native/ring binaries and
runs the two that need no device.

It installs the CUDA 13.0 pieces from NVIDIA's apt repository on the
runner instead of using an nvidia/cuda devel container. A container job
pulls its image before any step can free disk, and that image is 3.68
GiB compressed before the ~4.4 GB that torch 2.14+cu130 takes
installed. The package list (nvcc, cudart-dev, cublas/cusparse/cusolver
dev, about 2.7 GB by Installed-Size) is what a real build reads: the
backend's -MMD files and `nvcc -M` of the ring tests, mapped with
dpkg -S. #138 is not in this base, so a step makes #138's three
-std=c++17 -> c++20 edits with sed; after #138 merges it is a no-op and
should be deleted.

Local evidence: the sed step run against this Makefile changes exactly
lines 125, 130 and 175 (the #138 diff) and against #138's head changes
nothing; with those edits, CUDA_HOME=/usr/local/cuda-13.0, SM_ARCH=sm_89
and the libcuda stub on LIBRARY_PATH, `make all` built and passed
check-link, and the backend imported with no GPU visible. The stock
flags stop at bindings.cpp on "#error C++20 or later compatible compiler
is required". Ring tests: all six built with CXX_STD=c++20, and
test_record_consumer (14 passed) and test_clickhouse_record_sink (21
passed) ran with CUDA_VISIBLE_DEVICES empty. Not verified until CI runs:
the apt install on ubuntu-24.04, disk headroom, gcc 13 (local is 11.4),
and the job as a whole.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
ruff F821 over src/dmi reported 14 names, in two groups, and neither is
a NameError today:

- config.py annotates capture_sink_config and capture_storage_config
  with the strings "NativeSinkConfig" and "NativeCaptureStorageConfig"
  and imports neither. With `from __future__ import annotations` nothing
  evaluates them at runtime, but typing.get_type_hints(MonitoringConfig)
  raises NameError on the first one (reproduced; nothing in src/ calls
  it today). They are now imported under TYPE_CHECKING, so a static
  tool resolves them and the module still loads no storage package at
  runtime (checked: importing dmi.config loads no dmi.storage module).
  get_type_hints still raises, as before; B2's move of NativeSinkConfig
  into live code is where a runtime import can go.
- hooks/specs.py compares against HOOK_TYPE_Q and eleven siblings that
  the module binds through a globals() loop over _HOOK_DEFS, which a
  linter cannot see. Each of those ten lines now carries its own
  `# noqa: F821` with a comment saying why: per line rather than per
  file, so the rest of the module stays checked. A typo inside one of
  those ten lines would now be suppressed too; that is the cost.

No behaviour change. ruff 0.16.5 `check --select F821 src/dmi`: 14
errors before, "All checks passed!" after; tests/test_tp_shapes.py,
test_hook_spec_flags.py and test_config.py: 63 passed.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
An undefined name is a NameError that waits for its line to run, and
the cpu job had nothing that would see one first: `make check` compiles
with `python -m compileall`, which accepts any name. The new step runs
ruff's F821 rule, and only that rule, over src/dmi, pinned to ruff
0.16.5 with GitHub annotations.

The 14 existing hits are handled in the previous commit, in the source,
not excluded here. Local evidence with ruff 0.16.5: the step's command
passes on this tree, and with an undefined name appended to
src/dmi/config.py (not committed) it exits 1 naming the file and line.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
The chain test compared only capture_id, payload bytes, shape and dtype,
so a metadata field the torch sink's parser mangled went unnoticed. A
reviewer changed native/csrc/sink/record_row.cpp to store step_number + 7
and the chain test still passed, as did every CPU suite. This chain is the
only CI test that reaches record_row.cpp, so it has to catch that.

Each capture's read-back descriptor is now compared field by field against
the CaptureMetadata the test submitted for it: all 21 fields, with shape
as a tuple and a NULL adapter_revision as the empty string the native
reader returns for it. The integer fields now also take distinct values
(token_start and batch_position vary apart from step_number), so a parser
that wires one field into another's slot fails too. producer_rank stays
fixed, at 1, because the sink seals a pack when the rank changes.

The fake-S3 multipart check counted every logged PUT carrying a
partNumber, so one transient retry would have failed the part count. It
now keys parts by part number, keeping the last attempt.

Evidence, from a scratch copy of HEAD built with
make build/_dmi_native_sink build/_dmi_native_store:
- step_number + 7 mutant: 2 failed, 1 skipped (MinIO unset); both fail on
  {'step_number': 7} != {'step_number': 0} for chain-0000.
- unmutated rebuild: 2 passed, 1 skipped. Same in this worktree.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
The fake-S3 import sits among the module's top-level imports, before any
statement, so E402 never fires on it and the directive suppresses
nothing. ruff 0.16.5 with --select E402,RUF100 reports it as an unused
noqa (unused: E402); with it removed the same check passes.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
The step exists only until #138 moves the Makefile to -std=c++20, and it
had no way to say when that happened: after the merge the sed matches
nothing, and the closing grep still counts 3, because the three lines
already read c++20. The step would pass silently forever.

An unchanged native/Makefile after the sed is exactly that state (before
#138 the sed always rewrites all three lines), so the step now emits a
::warning:: asking for its own deletion. Rehearsed on a scratch copy of
the Makefile: at this branch's Makefile the diff is 3 lines and no
warning; with those edits committed, as #138 would leave them, the
warning prints and the count is still 3.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
_dmi_native_sink picks its RecordSink base once, when it loads: the
_native_backend registration when that backend is loadable, module-local
stand-ins otherwise. Only the first is attachable, since
create_record_runtime checks isinstance against the backend's class. The
cpu and live jobs build the sink with no backend, and native-backend-compile
built the backend with no sink, so the backend branch of
test_the_sink_derives_from_the_engines_record_sink ran in no job.

native-backend-compile now builds build/_dmi_native_sink after the
backend, with the same torch and PYTHON, and checks the binding in both
import orders: through dmi's loaders (backend first, as the engine does)
and as a bare import (the sink finds the backend itself). Each asserts
RING_TYPES_ARE_STANDINS is false and NativePackSink subclasses the
backend's RecordSink. Then it runs the adapter test and refuses a skip.

The assertions are needed because the test alone is not enough. Rehearsed
on a scratch copy of this branch with a copied torch 2.14+cu130 backend:
with the backend present, both checks print an MRO through
_native_backend.RecordSink and the test passes (1 passed). With the
backend removed, both assertions fail on the stand-ins, and the test
still passes, because its stand-in branch is a pass by design. The rehearsal
also found that the bare import needs torch imported first, since the .so
has no rpath to libtorch_cpu, so the check does that, as the suite does.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
The health loop ran curl -sf with no timeout, so a socket that accepts
and never answers would have held the first probe until the 30-minute
job timeout, and the loop's 30 attempts would never have run. Each probe
is now capped with --max-time 2. Checked locally against a listener that
accepts and sleeps: curl gives up after 2.01 s with exit 28, which the
loop treats as not up yet and retries.

The comment also called the pinned release "the last release published
as an image". Review found later tags on quay.io, for example
RELEASE.2025-09-07T16-13-09Z.hotfix.7aa24e772. (I could not list the
tags from here: quay's API wants a token.) The pin is still right, so
the comment now says only what it does: tag and digest together keep an
upstream retag from changing what the job tests against.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
The job comment justified freeing disk with "the ~14 GB a hosted runner
guarantees", but the runner this job actually got had far more. The
df output from run 35933626430 (job 107425626899) shows 87 GB free
before the step, 103 GB after it, 99 GB after the CUDA apt install and
94 GB after torch. Freeing the Android SDK and .NET took about two
minutes of that run.

The step stays, since the free space belongs to the runner image and can
change, but both comments now call it a precaution and quote only the
figures that log shows.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
Two CI failures on the previous push, neither in the code under test:

- The MinIO step's image could no longer be pulled. The MinIO project has
  withdrawn its public images: quay.io/minio/minio and docker.io/minio/minio
  both now answer anonymous pulls with 401, including `latest`, although
  this job pulled the pinned quay image fine a day earlier. The step now runs
  bitnamilegacy/minio:2025.7.23-debian-12-r5, pinned by digest -- a frozen
  archive, which suits a fixture whose S3 multipart rules do not change. The
  Bitnami image starts its own server from MINIO_ROOT_USER/_PASSWORD, so the
  `server /data` argument is gone, and the health loop allows about two
  minutes for its entrypoint's setup. The live skip gate then failed only
  because the suite never ran.
- The sink-binding check passed ("1 passed, 1 warning in 1.50s") but the
  step greps for '^1 passed in ', and torch's NumPy warning changed the
  summary line. The step now accepts a summary with warnings and still
  fails on any skip.

Validated locally: the workflow parses, and the new pass condition accepts
"1 passed" and "1 passed, 1 warning" and refuses "1 skipped". The image's
digest, amd64 manifest and entrypoint were read from Docker Hub's registry;
running it needs docker, which only CI has.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
MinIO was there for one check: that a multipart pack follows real S3's
rules (every part but the last at least 5 MiB, and a "-N" ETag), which the
in-repo fake S3 does not enforce. Nothing in DMI runs against MinIO -- the
object store it uses is Garage -- and the MinIO project has withdrawn its
public images, so the job had come to depend on a frozen third-party
archive for a store DMI does not use.

The CI step, its DMI_MINIO_* settings and the MinIO test case are removed.
The fake-S3 multipart case stays: it asserts the parts the client sends
(every part but the last is exactly the client's chunk and at least 5 MiB)
and reads the pack back byte-exact. Checks against a real store belong to
the Garage suites.

Chain test against local ClickHouse: 2 passed, 0 skipped; no tables left.
The workflow parses, and the only step removed from any job is the MinIO
one.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
#144 landed

- #138 is on main, so the temporary step that applied its C++20 flags with
  sed changed nothing and would only raise its "delete me" warning. Deleted,
  as its own comment asked.
- #144 puts the capture extensions in `make -C native all`, and the store
  needs libcurl's headers; without them the build now stops before
  anything else, by design. The job installs libcurl4-openssl-dev and
  pkg-config and lets the Makefile find libcurl through pkg-config -- its
  default lookup, with no CURL_* override -- so it builds exactly what
  docs/install.md tells a user to run. It lists the three extensions it
  expects in src/dmi afterwards.

Validated locally: the workflow parses and the job's steps are as listed;
ruff 0.16.5 F821 passes on the merged src/dmi; with #144's Makefile the
CPU targets build and install as links, and the chain test passes (2
passed). The CUDA build itself runs only in CI.

Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
@zaoxing
zaoxing force-pushed the ci/capture-chain-baseline branch from d2761f7 to 6b08e74 Compare September 24, 2026 15:10
@zaoxing
zaoxing changed the base branch from feat/native-capture-service to main September 24, 2026 15:10
@zaoxing
zaoxing merged commit a987dfe into main Sep 24, 2026
6 checks passed
@zaoxing
zaoxing deleted the ci/capture-chain-baseline branch September 24, 2026 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants