Repository navigation
ci(gqt): build the ordinary job's engine graph once - #896
Conversation
GQT (ordinary) ran three cargo invocations that each selected a different package (omnigraph-gqt, omnigraph-gqt-core, omnigraph-bench). Each selection resolves a different feature graph, so tokio, arrow, datafusion, lance and the engine built three times: about 20 minutes per extra graph even with a warm cache, and past the 60-minute budget cold. Both engine invocations now select the same three packages, so they share one graph; the benchmark units, the fixture parity test among them, run in the units step.
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve, subject to the required CI checks. I found no blocking defect at e844aa150710db746ed44e8efa72ffd249dfbd0d. The inline suggestion is optional.
This PR helps the test job finish before its time limit. Previously, three commands selected different Rust packages. Cargo then rebuilt shared dependencies with different features. Both engine-related commands now select the same three packages. They share the expensive build, while separate targets select the refusal tests and library tests. The practical benefit is more time for tests when the cache is empty.
The contract here is reliable qualification of engine and corpus changes, including changes that touch only .gqt files. Hosted Linux runners must meet that contract with an empty cache. Increasing the timeout would leave the repeated work. This change removes its cause through Cargo's existing feature resolution. See the workflow and Cargo's feature rules.
Coverage remains explicit. The library command includes the benchmark fixture parity test. Both commands retain empty RUSTFLAGS. The DST refusal depends on tokio_unstable, not the engine's dst feature. The separate DST execution, DST lint, seam guard, and required-result aggregation remain in place.
The tradeoffs are acceptable:
- Core and benchmark units now share GQT's
dst,failpoints, andtest-utilengine features. This job no longer exercises their separate default-only engine build. The separate engine lint job still checks the engine withoutdst. Lint does not replace runtime coverage of that configuration. - Every ordinary run now executes all benchmark units, including runs for corpus-only changes. That adds test work while removing repeated dependency builds.
- Separate commands provide separate timing and failure labels. They also require two package lists to stay aligned. The optional comment explains how one command can remove that requirement.
Liability decreases overall. The diff adds four net lines, but removes one invocation and two unnecessary engine build configurations. It adds no API, persisted state, storage rule, or runtime mechanism. Five similar changes should reuse the same package selection, rather than add five independently maintained build configurations. The remaining duplicated selection is the main maintenance cost.
Validation:
- Reviewed the exact head in a clean isolated checkout. Checked the manifests, feature paths, refusal implementation, parity owner, and workflow conditions. The pinned Lance 11.0.0 feature declarations agree with the resolved
awsandazureselection. - Generated Cargo build plans with the pinned toolchain and empty
RUSTFLAGS. The new commands have identical recursive build-unit signatures for the engine, Lance, DataFusion, Arrow-array, and Tokio. The old commands differ. This holds for the local target and an explicit Linux target. These are planning checks, not compiled test results. - Local seam guard: 8 passed. Workflow guards, their applicable self-tests, changed-file spelling, and documentation checks passed. Documentation checking covered 217 Markdown files.
- Exact-head CI run: DST execution and DST lint passed. The ordinary job was still building at review time. I did not repeat the full engine build locally. The PR's cold-build timings remain author evidence, not an independently measured time bound.
|
@azimafroozeh could you review this one? It is a CI-only change to the ordinary job #894 added steps to. Without it, On CI this run restored main's cache (full key match). The shared selection rebuilt 116 crates in 19 minutes, because main's cache holds the three per-crate graphs but not their union. The units step then took 46 seconds and the seam guard compiled one crate. Once main saves a cache with the union graph, a warm run rebuilds only the workspace crates a PR changes; a cold run is a single engine build. |
Problem
GQT (ordinary)has timed out on pull requests that change the engine since #894. Two runs this morning were cancelled at the 60-minute budget: #871 (run 37735019477) andbench/concurrent-merges(run 37733642954).#894 added two cargo invocations to the job, and each selects a different package:
-p omnigraph-gqt-p omnigraph-gqt-core-p omnigraph-benchEach selection resolves its own feature graph, so tokio, arrow, datafusion, lance and the engine are rebuilt in every step. The reason is that
omnigraph-gqtenables the engine'stest-utiland depends onomnigraph-dst, and the other two crates do neither. That is the #755 mechanism. The cache rule indocs/dev/ci.mdforbids it: a secondcargoinvocation in a job must not change the dependency graph, and no job may need its cache to fit its budget.The cold runs were not bad luck that a rerun would avoid. rust-cache hashes every installed toolchain into the key, and that includes the runner image's preinstalled stable. Main's cache was saved from a runner with 1.99.0 (
415b780c), while #871's runner had 1.98.1 (c4cc11d5). Which key a job computes therefore depends on which runner image it lands on.Change
-p omnigraph-gqt -p omnigraph-gqt-core -p omnigraph-bench, so they resolve one feature graph and build it once.--test runner_dispatch. A new units step runs--libfor all three crates.Test Workspace. The fixture parity test is among them, so it still runs on cases-only diffs.docs/dev/ci.mddescribes the shared selection.Evidence
Local run of the job's three steps from an empty target directory (
RUSTFLAGS=""):--test runner_dispatch--libassert_cmd,predicates,toml) plus the three crates' unit-test builds; no tokio, arrow, datafusion, lance or engineWith one graph, the cold cost is a single engine build. On CI that build took 27 minutes in #871's cold run, about half the 60-minute budget, which is what
docs/dev/ci.mdrequires.Checks:
check-ci-cells.py(with--self-test),check-classify-copy.py,check-workflow-action-pins.py,check-merge-group-triggers.py --self-test,check-change-classes.py(with--self-test),check-storage-upgrade-ci.py --self-test,check-docs.pyandtypos.Not in this change
docs/dev/ci.mdsays. Fixing that is separate.Azurite Azure Integrationbreaks the same rule. Itsrun_cellcalls selectomnigraph-storage,omnigraph-azure-admission, the engine withfailpoints,omnigraph-cluster,omnigraph-serverandomnigraph-clione at a time, and each selection resolves its own graph. That step took 89 minutes on main's push run and hit the 90-minute budget there and on two pull requests.Storage Upgrade Compatibilityruns--workspaceand then three-p omnigraph-engine --features failpointsscopes, which build a third graph (20 minutes warm on main). It still fits its 90-minute budget, butdocs/dev/ci.mdcounts only two builds for that job.CI-only, so no changelog fragment.