Skip to content

[CI] Shard long misc test groups - #52346

Open
khluu wants to merge 8 commits into
vllm-project:mainfrom
khluu:ci-shard-v1-core-kv-metrics
Open

[CI] Shard long misc test groups#52346
khluu wants to merge 8 commits into
vllm-project:mainfrom
khluu:ci-shard-v1-core-kv-metrics

Conversation

@khluu

@khluu khluu commented Aug 14, 2026

Copy link
Copy Markdown
Member

Why

In the 24-hour dashboard window ending 2026-09-01 10:31 UTC:

  • NVIDIA H200 V1 Core + KV + Metrics: 3,975s P90 (37 runs)
  • CPU Async Engine / Inputs / Utils / Worker / Config: 3,330s P90 (35 runs)
  • AMD V1 Sample + Logits: 2,917s P90 (36 runs)

What changed

Split the long misc test groups into separate Buildkite steps by command rather than by pytest sharding, so each job runs a coherent suite:

  • V1 Sample + Logits → V1 Sample, V1 Logits + Oracle
  • V1 Core + KV + Metrics → V1 Core, V1 Executor + Worker, V1 KV Offload, V1 KV Connectors, V1 Metrics + LM Eval
  • CPU Async Engine / Inputs / Utils / Worker / Config → Params, Env, Tokenizers, Parser, Multimodal + Config, Reasoning + Renderers, Tool Parsers

Shared source_file_dependencies lists are factored out with YAML anchors. No parallelism / --num-shards / --shard-id logic remains; the LM Eval streaming-correctness integration test now runs unconditionally in its own step instead of being gated on a shard index. Each NVIDIA step keeps its AMD mirror.

This consolidates and replaces draft #52348. No merged or active non-draft PR covers these jobs.

Validation

  • YAML parse (23 steps, no duplicate keys)
  • current ci-infra Buildkite step pydantic schema
  • all YAML command entries pass bash -n
  • end-to-end pipeline-generator run: 641 Buildkite steps, no duplicate keys, all new steps + AMD mirrors render correctly
  • git diff --check

Targeted CI

TODO: trigger a targeted build on the new steps and record wall times.

This is a draft pending human review. AI assistance was used.

Summary by CodeRabbit

  • Tests
    • Reorganized the miscellaneous CI test pipeline into smaller, more focused jobs.
    • Split V1, CPU, executor, worker, KV, metrics, tokenizer, parser, multimodal, reasoning, renderer, and tool-parser tests into separate jobs.
    • Adjusted time limits and shared test dependency configuration for improved test execution management.

@mergify mergify Bot added the ci/build label Aug 14, 2026
@mergify

mergify Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @khluu.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Aug 19, 2026
Assisted-by: AI
Signed-off-by: Kevin Luu <51931015+khluu@users.noreply.github.com>
Signed-off-by: Kevin Luu <51931015+khluu@users.noreply.github.com>
@khluu khluu changed the title [CI] Shard v1 core, KV, and metrics [CI] Shard long misc test groups Sep 1, 2026
@khluu
khluu force-pushed the ci-shard-v1-core-kv-metrics branch from 335d833 to e466477 Compare September 1, 2026 10:57
Signed-off-by: Kevin Luu <51931015+khluu@users.noreply.github.com>
@khluu

khluu commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Targeted build #86571 exposed a real sharding bug in V1 Sample + Logits: applying pytest sharding to each small file separately left some shard/file pairs with zero nodes, and pytest correctly exited 5. Commit 293dab6 fixes that by collecting the whole group in one pytest invocation before partitioning it. The other changed groups in the build passed: CPU async 16m21s; V1 Core shards 14m34s / 15m46s / 22m18s / 18m55s / 13m39s. A targeted V1 Sample rerun is pending.

Signed-off-by: Kevin Luu <51931015+khluu@users.noreply.github.com>
@khluu

khluu commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Follow-up from targeted build #86585: keep the five pytest paths in separate processes to preserve CUDA/fork isolation. Each command now accepts only pytest exit 5 (an expected empty per-file partition) and still propagates every real test failure. YAML/schema validation, rendered-command bash -n, explicit 0/5-pass and 1/2-fail exit-policy checks, and git diff --check pass at head 0734320.

@mergify mergify Bot removed the needs-rebase label Sep 1, 2026
@khluu
khluu marked this pull request as ready for review September 1, 2026 22:26
@khluu
khluu requested a review from Harry-Chen as a code owner September 1, 2026 22:26

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@njhill njhill left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @khluu!

This looks ok to me as an immediate measure, but I feel like it's a kind of messy/fragile approach and it would be better to split these into multiple groups without all of the env var scaffolding.

@njhill

njhill commented Sep 2, 2026

Copy link
Copy Markdown
Member

/ci run

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #86906 for commit d6f34062bdcc.

Replace pytest --num-shards/--shard-id partitioning with separate
Buildkite steps per command group, so each job runs a coherent suite:

- V1 Sample + Logits -> V1 Sample, V1 Logits + Oracle
- V1 Core + KV + Metrics -> V1 Core, V1 Executor + Worker,
  V1 KV Offload, V1 KV Connectors, V1 Metrics + LM Eval
- CPU Async Engine group -> Params/Env/Tokenizers/Parser,
  Multimodal + Config, Reasoning + Renderers, Tool Parsers

Shared source_file_dependencies lists are factored out with YAML
anchors. No parallelism/shard-id logic remains; the LM Eval
integration test now runs unconditionally in its own step.

Co-authored-by: Kimi Code <noreply@moonshot.cn>
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: aa9906d2-70f9-44d8-9896-0a2f489895ff

📥 Commits

Reviewing files that changed from the base of the PR and between d539de1 and ad746d4.

📒 Files selected for processing (1)
  • .buildkite/test_areas/misc.yaml

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The Buildkite miscellaneous pipeline splits combined V1 and CPU test jobs into smaller jobs. The changes add shared YAML dependency anchors, separate test commands, per-job AMD mirrors, and shorter job timeouts.

Changes

Miscellaneous test pipeline

Layer / File(s) Summary
V1 sample and logits jobs
.buildkite/test_areas/misc.yaml
The V1 Sample job is renamed and shortened. Logits, oracle, request, and outputs tests move to a new V1 Logits + Oracle job.
V1 core test jobs
.buildkite/test_areas/misc.yaml
The combined V1 job is split into V1 Core, V1 Executor + Worker, V1 KV Offload, V1 KV Connectors, and V1 Metrics + LM Eval jobs.
CPU test jobs
.buildkite/test_areas/misc.yaml
The CPU job is renamed and split into Params, Env, Tokenizers, Parser; Multimodal + Config; Reasoning + Renderers; and Tool Parsers jobs.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to ad746

This PR only reorganizes CI test commands into separate Buildkite steps and preserves AMD mirrors; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: andreaskaratzas

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately identifies the main change: splitting long miscellaneous CI test groups into smaller Buildkite steps. It is concise and relevant.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@njhill

njhill commented Sep 3, 2026

Copy link
Copy Markdown
Member

/ci run

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #86987 for commit ad746d42d8f8.

GIT_TERMINAL_PROMPT=0 makes a transient git credential prompt (seen on
agent h200-ci-6-19 in build 86987) error immediately instead of hanging
the V1 Metrics + LM Eval job until the timeout.

Co-authored-by: Kimi Code <noreply@moonshot.cn>
@mergify

mergify Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @khluu.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Sep 3, 2026
Resolve conflict in .buildkite/test_areas/misc.yaml: main added a
Git HTTP/1.1 env workaround for the flaky LM Eval source install on the
monolithic V1 Core + KV + Metrics step. After the command-based split,
that install lives only in the V1 Metrics + LM Eval step, so the env
block moves there.

Co-authored-by: Kimi Code <noreply@moonshot.cn>
@mergify mergify Bot removed the needs-rebase label Sep 3, 2026
@mergify

mergify Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @khluu.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants