Repository navigation
[OMNIML-6183] [2/2] Run the GPU, example and regression tests in the merge queue only - #2860
shengliangxu wants to merge 13 commits into
Conversation
The merge queue builds a temporary branch of the target branch plus the queued pull requests and waits for the required status checks on it. The unit, code quality and docs workflows only ran on pull_request events, and the GPU, example and regression workflows only on pushes to the copy-pr-bot branches (pull-request/N) and on schedule, so no required check would ever report on a merge group. Add the merge_group trigger (checks_requested) to all six workflows and treat a merge group like a PR run: - The gate of the GPU, example and regression workflows and the unit tests' changed-files check diff the group against the commit it was built on (the pr-merge-base action reads it from the merge_group payload), so the same lanes run as on the PR and the wait for the unit `linux` check stays. - The *-pr-required-check aggregators, the single-GPU runners, the example timeouts, hf_ptq's -n 4 and the left-out `manual` unit tests all key off "a PR run", which now includes a merge group. - TruffleHog scans base..head of the group, like a PR. - The Codecov uploads are skipped: a queue commit never lands on main. Nightly, on-demand and PR runs behave as before. The queue itself is a ruleset setting (see the PR description); CONTRIBUTING.md describes it. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
|
/ok to test 076c797 |
|
/ok to test ff9cba8 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## shengliangx/ci-12-merge-queue #2860 +/- ##
=================================================================
- Coverage 78.49% 76.12% -2.37%
=================================================================
Files 658 658
Lines 74542 74542
=================================================================
- Hits 58511 56748 -1763
- Misses 16031 17794 +1763
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The merge_group triggers had no branch filter, so a merge queue enabled on any branch would start the required checks there. Give the six triggers the branches of their pull_request triggers (main, release/*, feature/*), which are the branches that have the required checks, and say so in CONTRIBUTING.md. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
…e merge queue A pull request ran up to five GPU lanes and thirteen example lanes on every push, although most of that time goes to tests that start several processes, build engines or compile for tens of seconds. Split the GPU tests by what they do: - GPU unit tests run in the pytest process on one GPU and take seconds. They run on every push to a pull request, as before. - GPU system tests carry the new pytest marker `system` and run in the merge queue and on the nightly. That is every test of tests/gpu_megatron, gpu_trtllm, gpu_vllm, regression and examples, and in tests/gpu the tests that request `dist_workers` or `need_N_gpus` (marked by tests/conftest.py) and the modules marked by hand: puzzletron, the unified HF export, the extension JIT builds, the TensorRT engine and trtexec tests, and the tests that spawn processes or run a subprocess. The pr-merge-base action now also reports `run_system`: always true in a merge queue group and on nightly and on-demand runs, and on a pull request only when it carries the `run-system-tests` label. When it is false the GPU workflow runs the `gpu` lane with `-m "not system"` and skips the system lanes (the lane steps move to a reusable _gpu_tests_runner.yml, like the example runner), and the regression and example workflows skip their jobs. The *-pr-required-check jobs keep their names and count skipped lanes as passed, with a notice on the run. A unit-only run does not upload GPU coverage, so carryforward keeps the last full report. CONTRIBUTING.md describes the split. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
The gate of the GPU workflow lists the files whose change runs the lanes; the new _gpu_tests_runner.yml belongs there like _pr_gate.yml does. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
ff9cba8 to
337790e
Compare
|
/ok to test 337790e |
The PR-like predicate made several runner, if and env values up to 193 columns wide. Fold them into `>-` scalars broken after `&&` / `||` (the parsed YAML value is byte-identical) and wrap two shell conditions and two comments, so no line this PR adds is longer than 110 columns. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Takes this branch's lines in the three workflows it restructures (example, GPU and regression tests); the next commit wraps the lines this PR added. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Fold the runner, if, description and notice values this PR added into `>-` scalars (the parsed YAML value is identical) and split the lane condition, the run_system echo and one `if` continuation, so no line either PR of the stack adds is longer than 110 columns. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
|
/ok to test 1f42c84 |
`integration` is the pytest convention next to `unit`, and pyproject.toml already declared a marker of that name that nothing uses, so reuse it instead of adding `system`: the marker, the `run_integration` gate output, the `run-integration-tests` label, the `gpu-integration-tests` job and the docs. Its description now covers what the set holds: other tools or engines, several processes or GPUs, subprocesses and heavy compilation. The same 114 tests of tests/gpu and tests/regression are marked before and after (collected on CPU). Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
… only A plain PR push runs the `gpu` lane in full again, as before the split, instead of its unit-marked subset: +9-20 min per push today, about nothing once the speed-up series lands. Without a marker split inside tests/gpu the pytest hook, the hand marks, the `unit_only` input and the Codecov carve-out are gone. The merge queue only adds the lanes that integrate with other frameworks (Megatron-Core, TensorRT-LLM, vLLM) and the example and regression tests. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
|
/ok to test da2595d |
A full tests/gpu run takes 39-49 min, so a pull request push no longer runs any GPU test: the GPU, example and regression workflows run in the merge queue, on the nightly and on demand, and on a PR that carries the `run-gpu-tests` label. One gate output, `run_gpu`, switches all three, so the lane split, the pytest marker and the reusable runner workflow of the earlier revisions are gone and the GPU job is the matrix job of main again. The gate does not wait for the unit tests when nothing will run, and the shared `gpu` Codecov flag is either complete or absent, which carryforward already handles. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
|
/ok to test e6f9bb1 |
The label now applies to one round: the aggregate job of each of the GPU, example and regression workflows deletes it once that workflow has run its tests, pass or fail, so later pushes go back to leaving the GPU tests to the merge queue. A cancelled run (superseded by a newer push) leaves it, and so does a workflow that skipped its tests, which keeps the label from disappearing before the other workflows' gates have read it. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
|
/ok to test da04173 |
The label makes a pull request push run what the merge queue runs on top of a PR run (the GPU, example and regression tests), not only GPU tests, and the name stays right if other queue-only tests are added later. The gate output follows: run_gpu becomes run_merge_tests. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
|
/ok to test 5f91c3c |
The action and gate descriptions, the comments and the notices still said "GPU tests" for the tests that the label and run_merge_tests switch, which also cover the example and regression tests. Name them merge tests, say which tests that is where the gate or the action describes it, and let each workflow's notice say that its tests are merge tests. Text only: the parsed YAML differs in those six strings and nowhere else. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
|
/ok to test 3a309d3 |
|
/ok to test |
|
/ok to test 3a309d3 |
What does this PR do?
Type of change: CI (test selection)
A pull request push runs five GPU lanes (
gpu39-49 min, Megatron 42-49, TensorRT-LLM, two vLLM versions), a regression lane and thirteen example lanes: about 285 job-minutes on GPU runners (measured below), and a pull request usually takes several pushes. This PR calls the GPU, example and regression tests the merge tests and leaves them to the merge queue:run-merge-testslabel: the merge tests run in full.What runs when.
run-merge-testslabelpr-merge-baseaction reportsrun_merge_tests(a label on the copied PR branch, always in a queue group);_pr_gate.ymland the example gate pass it on, and empty means a nightly or on-demand run, which runs everything.gpu_tests.ymlandregression_tests.ymladdrun_merge_teststo theifof their test job andexample_tests.ymlto its lane selection. The gate does not wait for the unit tests when the tests are skipped.*-pr-required-checkjobs keep their names and count skipped lanes as passed, with a notice on the run saying where the tests run.gpu, which is now either complete or absent on a pull request.carryforwardin.github/codecov.ymlhandles an absent flag; the queue does not upload coverage.*-pr-required-checkjob of each workflow removes it as its last step once that workflow has run its tests, pass or fail, so later pushes leave the GPU tests to the queue again. A run cancelled by a newer push and a workflow that skipped its tests leave it, which keeps it from vanishing before the other workflows' gates have read it. The jobs getpull-requests: writefor this. "Re-run failed jobs" keeps working after the label is gone (the gate's output is kept); "re-run all jobs" needs the label again.Trade-offs: a pull request gets no GPU, example or regression result until it is queued, unless the author adds the label (and re-runs the workflow, which reads the labels when the gate job starts). A GPU failure ejects the PR from the queue and the PRs behind it are rebuilt, so the cost of a failure moves from the author's branch to the queue. The queue's status check timeout has to cover the longest workflow (see #2858). Some lanes have little headroom on a slow host: the 15 min job caps of the vLLM and regression lanes and the 120 s per-test cap of the Megatron lane (see the time section). A flake there now ejects a PR from the queue, so they are worth loosening before the queue is turned on. A cheaper variant, running only the unit-marked part of
tests/gpuon a push (29.6 min), was measured and dropped: it needs a pytest marker and a conftest hook and still ties up a GPU runner on every push.Time impact (measured in CI)
#2858 (the PR below: main plus the merge queue changes) ran every lane on its push. A push of this PR, without the label, runs none of them. Same base, one run each:
gpujobgpu_megatron,gpu_trtllm,gpu_vllm(2) jobsregression-testsjobgpujob, 39-49 min, and Megatron, 42-49 min, run in parallel and set it), 36-40 min with the speed-up series. A pull request that is pushed N times used to cost N full runs.wait-checks, 5-13 min on a hosted runner per workflow; 5 min is its minimum) is skipped when the tests are skipped.gpu(1,833 passed, 72 skipped, the same as [OMNIML-6182] [1/2] Run the required checks in GitHub's merge queue #2858's run),gpu_trtllmand bothgpu_vllmlanes passed, 280 job-minutes for the round (127.8 GPU, 137.8 examples, 14.7 regression; GPU workflow 69.7 min wall on a slow day).test_mamba_search_space, a cold Triton compile that takes 73 s on a normal host and hit its 120 s per-test cap (123 s) on a slower one, where the lane's pytest took 45 min instead of 35. The same test failed the Megatron lane of CI follow-up: cache source-built wheels and extensions, bump CI containers, trim slow GPU tests #2836 and Sync the amax of vLLM's tensor-parallel linears across TP ranks #2842, andtest_fp8_real_quantizefailed the nightly on main, the same way: 4 of the last 33 Megatron lane runs. The vLLM v0.30.0 lane hit its 15 min job cap once with all 196 tests passed, and passed on the re-run. The regression job (15 min cap) took 9.8 min on a fast host, 14.5-14.7 min on two slower ones and hit the cap on a third. These caps are on main already; see the trade-offs for what they mean once the tests only run in the queue.Usage
run-merge-testslabel (a repository label that already exists) and re-run the workflows. CI removes the label when that round of runs finishes; add it again for the next round.workflow_dispatch) still run everything on a normal branch; on apull-request/Nbranch the gate cannot resolve the PR, as on main.Testing
jq) gives true only for a PR with therun-merge-testslabel, also whenlabelsis empty or missing. Thegpu-pr-required-checkcondition was compared with its specification for 144 combinations of the gate and job results.gh(label removed, label absent) and its condition was compared with its specification for 54 combinations. The action'sjqstep was run on sample PR info:run-merge-testsgives true; other labels, an empty list and a null list give false.run-merge-testslabel. The gate read it (wait-checksran), the regression job hit its 15 min cap on a slow host (cancelled, so this is also the failed-round case), and the aggregate job still removed the label (the label event is bygithub-actions[bot]) and printedRemoved the run-merge-tests label after this round; re-add it for the next push.Without the label the step is skipped and the three workflows finish in about half a minute.merge_groupevent (the queue is not on yet, see [OMNIML-6182] [1/2] Run the required checks in GitHub's merge queue #2858).Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: no new dependencyAdditional Information
Slice 2 of 2 of the merge queue work, stacked on #2858, which makes the required checks run on
merge_group; this PR decides which of them run on a pull request and which only in the queue. It is independent of the GPU-CI speed-up series (#2845-#2855, reference draft #2730); together with #2858 it touchesunit_tests.yml,gpu_tests.ymlandregression_tests.ymlnext to lines the series edits (a trial merge against the top of the series has one conflicting hunk), so whichever lands second needs a trivial rebase.