Skip to content

chore(ci): drop compile gates already covered by clippy - #1048

Open
luicarus wants to merge 3 commits into
pegainfer-project:mainfrom
luicarus:chore/ci-fuse-duplicate-gates
Open

luicarus wants to merge 3 commits into
pegainfer-project:mainfrom
luicarus:chore/ci-fuse-duplicate-gates

Conversation

@luicarus

Copy link
Copy Markdown

Description

Part of #975.

On current master, a ready-for-review PR expands to 16 job executions. Three of those jobs cover compile surfaces that are already exercised by retained Clippy jobs. This PR removes those redundant executions without reducing package, feature, or target coverage.

1. feature-model-cuda: remove the compile leg (2 jobs)

The compile and clippy legs use the same package, feature, release profile, and --all-targets surface:

compile: cargo check  -p <package> --features <feature> --all-targets
clippy:  cargo clippy -p <package> --features <feature> --all-targets -- -D warnings

For the same package / feature / target surface, the retained Clippy invocation exercises the same compilation surface while also running the lints, so the separate compile leg adds no additional coverage.

The gate: compile value and its corresponding include entry are removed together. Removing only the dimension value would leave the include entry unmatched, causing GitHub Actions to append it as a new matrix combination without the expected model, package, and feature bindings.

2. qwen3-cuda-compile: remove the job (1 job)

The two Qwen3 jobs currently cover:

qwen3-cuda-compile:
  cargo check ... -p pegainfer-server --all-targets

qwen3-cuda-clippy:
  cargo clippy ...
    -p pegainfer-server
    -p pegainfer-qwen3
    -p pegainfer-core
    -p pegainfer-kernels
    -p pegainfer-kv-cache
    -p pegainfer-kv-offload
    -p pegainfer-sample
    -p pegainfer-bench
    --all-targets -- -D warnings

The compile job's package set (pegainfer-server) is a subset of the Clippy job's package set, with the same --all-targets surface.

qwen3-cuda-clippy already carries CUDA_PATH, PEGAINFER_CUDA_SM, and PEGAINFER_NVCC_JOBS on its Cargo step, so the CUDA-specific build environment required by the retained command is preserved.

Coverage map

Every surface covered by a removed job remains covered by a retained command:

Removed job Package / feature / targets Retained job Coverage
Qwen3 CUDA compile pegainfer-server, default features, --all-targets Qwen3 CUDA Clippy Same package included in its 8-package set, same --all-targets surface
Gemma 4 CUDA Compile pegainfer-gemma4, gemma4, --all-targets Gemma 4 CUDA Clippy Identical package / feature / target surface
Qwen3.5 CUDA Compile pegainfer-qwen35, qwen35, --all-targets Qwen3.5 CUDA Clippy Identical package / feature / target surface

All other gates are unchanged: CPU Clippy, CPU unit tests, simulated frontend E2E, DCO, attribution, formatting, and locked Cargo metadata.

Expected impact

Ready-for-review job executions:

16 → 13

Repeated setup executions:

Setup step Before After
actions/checkout 15 12
dtolnay/rust-toolchain 14 11
mozilla-actions/sccache-action 12 9
apt-get install protobuf-compiler libibverbs-dev 12 9
Jimver/cuda-toolkit 6 3

The direct expected benefit is lower summed runner time and less repeated setup work. The wall-time effect depends on which jobs land on the critical path, so I won't claim an improvement there until it is measured.

Before numbers

These numbers come from the Actions jobs API for the two runs linked in #975.

Here, wall time means the active-job span:

min(job.started_at)max(job.completed_at)

and summed job runtime is:

Σ(job.completed_at - job.started_at)

over all executed jobs in the run.

Run Executed jobs Wall time Summed job runtime
32950033957 16 6m19s 44m26s
32869418134 16 10m56s 70m15s

The three jobs removed by this PR account for:

Removed job 32950033957 32869418134
Qwen3 CUDA compile 320s 356s
Gemma 4 CUDA Compile 295s 288s
Qwen3.5 CUDA Compile 284s 656s
Total 899s 1300s

Simply subtracting those jobs from the baseline gives projected summed runtimes of approximately:

  • 29m27s for run 32950033957 — 33.7% lower;
  • 48m35s for run 32869418134 — 30.8% lower.

Those are arithmetic projections, not measured after-results. They assume the retained jobs behave exactly as they did in the baseline runs.

The wall-time effect is less predictable:

  • In 32950033957, the longest retained job was Gemma 4 CUDA Clippy at about 6m17s, so removing the compile jobs alone would not necessarily reduce the critical path.
  • In 32869418134, the 10m56s critical-path job was Qwen3.5 CUDA Compile, which this PR removes. That run therefore could improve substantially if the retained Qwen3.5 CUDA Clippy job does not exhibit the same slowdown.

Because the second run is an outlier, I don't want to infer a wall-time improvement from it without after-data.

Measurement convention

The issue reports 6m39s and 11m00s for the same two baseline runs. Reading them through the Actions API produces a different span depending on the timestamps used:

Run Active-job span run_started_atupdated_at Difference
32950033957 6m19s 7m39s 1m20s
32869418134 10m56s 11m00s 4s

The whole-run span includes runner allocation before the first job starts and workflow finalization after the last one completes, so it is always the larger of the two — by 1m20s in one run and 4s in the other. That gap is scheduling overhead, not compute, and it varies enough between runs that mixing the two definitions would make a before/after comparison unreliable.

The comparison above therefore uses the active-job span, min(job.started_at)max(job.completed_at). I'm stating the convention up front so the after-runs are read the same way.

After numbers

Once this PR has completed runs of its own, I'll report the same measurements across multiple ordinary PR runs, with links to the raw Actions results:

  • wall time, using both timestamp conventions above;
  • summed job runtime;
  • per-job setup and Cargo-step duration;
  • sccache hit/miss/write-error statistics.

These measurements need to come from completed GitHub Actions runs: runner scheduling and cache behavior are properties of the CI environment and cannot be reproduced meaningfully by a local build.

Follow-up under #975

The fused CPU gate is intentionally not included in this PR. CPU fusion can reduce summed runner time while also changing the workflow critical path, so I'd prefer to evaluate and measure that tradeoff separately rather than mix it with the straightforward CUDA deduplication here.

Out of scope

Left untouched as specified in #975:

  • routing direct nvcc compilation through a cache launcher;
  • investigating the GitHub Actions cache write errors;
  • release CI.

release.yml is unchanged.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

CI maintenance / performance optimization.

Checklist

  • My code follows the style guidelines of this project (see docs/conventions/coding-style.md).
  • I have performed a self-review of my own code.
  • I have formatted my commits according to Commitizen conventions.
  • I have run the local test suite and all tests pass (see CLAUDE.md).

Local verification

This PR only changes .github/workflows/ci.yml; there are no Rust source, manifest, lockfile, or build.rs changes.

Ran locally:

  • cargo fmt --all --check — passes (rustc 1.99.0-nightly, matching rust-toolchain.toml).
  • cargo metadata --locked --no-deps --format-version 1 — passes.
  • Workflow YAML structure checked: 9 job definitions; feature-model-cuda expands to 2 executions; no unmatched matrix include entry remains.

The full ## Tests suite from CLAUDE.md was not run locally because the relevant commands require CUDA/GPU/model-weight dependencies unavailable in this environment. The checklist item is therefore left unchecked; the retained CI jobs will exercise those paths on this PR.

`qwen3-cuda-compile` checked one package (`pegainfer-server`) while
`qwen3-cuda-clippy` lints eight that include it, both with `--all-targets`,
so the compile job's surface is strictly contained in the clippy job's.

The `feature-model-cuda` matrix ran a `compile` and a `clippy` leg over the
same package, feature and `--all-targets` surface; clippy subsumes check, so
the `compile` leg is redundant. The corresponding `include` entry is removed
with the dimension value, since a leftover `gate: compile` entry would be
appended as a new combination with no model fields bound.

Ready-for-review PRs go from 16 to 13 executed jobs. No coverage is dropped:
every gate the removed jobs covered is still compiled, linted or run by a
retained job.

Reported in pegainfer-project#975.

Signed-off-by: luxing <luicarus@users.noreply.github.com>
@FeathBow

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: fedcdccf46

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@FeathBow FeathBow left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! Before we go further on job counting, I want to flag where the time actually goes. The wall clock on a ready PR is one CUDA lane, about six minutes, and removing three jobs left it there. Job dedup buys summed runner minutes.

The repeated work is nvcc. build.rs spawns it directly, so RUSTC_WRAPPER=sccache never sees it, and every CUDA lane compiles the same seventeen shared translation units under pegainfer-kernels/csrc from cold. That is why a lane can report 92% sccache hits and still spend three and a half minutes in cargo. We paid for that six times per PR before #1048 and pay three times now. The seam is narrow. pegainfer_build::CudaToolkit hands out nvcc as a PathBuf and build.rs spawns it in four or five places, so turning that into a command that can carry a launcher puts sccache in front of nvcc in CI and on cold dev machines too. pegainfer-build already runs in the CPU unit test job and already has a fake toolkit tree helper, so that logic is testable without a GPU. I recommend we spike it next.

Two smaller things. The toolchain pin is declared in rust-toolchain.toml and again in every dtolnay/rust-toolchain step, which is what produced the rustfmt component conflict we fixed by syncing nine copies. The action only sets rustup default, and the toolchain file overrides that as soon as cargo runs in the repo, so I recommend dropping the input and letting the file drive the install. Separately, the CUDA lanes key on model x gate, and with the gate axis gone Qwen3 still sits outside the matrix while the other two lines sit inside it. One row per model line, with the default features row carrying the shared crates, would make the next line a table row.

@luicarus

Copy link
Copy Markdown
Author

Taking this. Before writing code, I checked what sccache actually caches for nvcc in v0.16.0 (src/compiler/nvcc.rs, the version pinned by CI). The top-level nvcc invocation itself is not cached, but sccache decomposes it and caches the underlying cicc, ptxas, cudafe++, and host-compiler invocations, so the device-side compilation path is covered. I'll implement the launcher seam next and validate the CUDA cache behavior in CI.

I'll do the launcher work as a separate PR since it touches pegainfer-build and pegainfer-kernels/build.rs rather than just the workflow, which keeps #1048 focused and reviewable. I'll keep the two workflow cleanups you mentioned on #1048.

Every dtolnay/rust-toolchain step repeated the channel and components that

ust-toolchain.toml already declares. When the two disagreed, rustup reported
a component conflict; commit ba794bd patched that by hand-syncing the
components list across all nine steps, which is the part that does not scale.

rustup reads the toolchain file as soon as cargo runs in the repository, and
installs the components it lists there, so the duplicated input is redundant.
Verified locally: a stable toolchain installed with --profile minimal (no
rustfmt, no clippy) picks both up on the first cargo fmt inside a project
whose toolchain file requests them.

The 	oolchain: input stays because the action requires it.

Reported in pegainfer-project#975.

Signed-off-by: luxing <luicarus@users.noreply.github.com>
The CUDA lanes were grouped by model x gate, which left Qwen3 outside the
matrix after the compile leg went away while gemma4 and qwen35 stayed
inside it. Three model lines, two ways of declaring them.

This makes the matrix a single include list with one row per model line, so
adding a model line is one more row. The Qwen3 row is the default-feature line,
which is the only one that takes no --features, so it is also the row that
carries the shared crates (pegainfer-core, -kernels, -kv-cache,
-kv-offload, -sample, -bench) alongside pegainfer-server.

Coverage is unchanged, verified by expanding the matrix and comparing the
resulting command lines: the Qwen3 row still lints the same eight packages with
no --features, and the Gemma 4 / Qwen3.5 rows still lint their own package
with their own feature. cargo_command and 	railing_args were only ever
clippy and -- -D warnings through the gate dimension, so they are now
written directly.

Job executions are unchanged at 13; this is a structure change, not a
scheduling one.

Reported in pegainfer-project#975.

Signed-off-by: luxing <luicarus@users.noreply.github.com>
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