Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI/Build] Limit github CI jobs based on files changed #9928

Merged
merged 9 commits into from
Nov 5, 2024

Conversation

russellb
Copy link
Collaborator

@russellb russellb commented Nov 1, 2024

I noticed that these github CI jobs run against all PRs, including ones where
the job is not relevant. We can save a bit of time and resources by only running
jobs when relevant files have changed.

This PR would reduce the jobs that run by quite a bit on a PR that only changes
docs, for example.

8f2f678 [CI/Build] Conditionally run clang-format
b1daf19 [CI/Build] Conditionally run mypy job
692d7cf [CI/Build] Conditionally run ruff job
bd611ca [CI/Build] Conditionally run yapf job
b837e46 [CI/Build] Add actionlint error matcher to job paths
18fc405 [CI/Build] Move codespell to its own job
8dbb595 [CI/Build] Only run the ruff job with one Python version
3942c38 Run mypy job when pyproject.toml changes
faff4e6 Only run yapf with Python 3.12

commit 8f2f678
Author: Russell Bryant [email protected]
Date: Fri Nov 1 17:18:49 2024 +0000

[CI/Build] Conditionally run clang-format

Update the `clang-format` job to only run when relevant files have
changed: `*.h`, `*.cpp`, `*.cu`, or `*.cuh`. This matches the patterns
in the `find` command in this job. Also run the workflow when the
workflow definition itself changes.

Signed-off-by: Russell Bryant <[email protected]>

commit b1daf19
Author: Russell Bryant [email protected]
Date: Fri Nov 1 17:23:07 2024 +0000

[CI/Build] Conditionally run mypy job

Only run the `mypy` CI job when the following paths change:

- any Python file, `*.py`
- the workflow definition
- `tools/mypy.sh`

Signed-off-by: Russell Bryant <[email protected]>

commit 692d7cf
Author: Russell Bryant [email protected]
Date: Fri Nov 1 17:26:23 2024 +0000

[CI/Build] Conditionally run ruff job

Only run the `ruff` CI job when relevant paths change:

- the workflow file, or the error matching config
- pyproject.toml
- any Python file, `*.py`
- requirements-lint.txt

Signed-off-by: Russell Bryant <[email protected]>

commit bd611ca
Author: Russell Bryant [email protected]
Date: Fri Nov 1 17:28:19 2024 +0000

[CI/Build] Conditionally run yapf job

Only run `yapf` job when the following paths change:

- any python file, `*.py`
- the workflow definition file

Signed-off-by: Russell Bryant <[email protected]>

commit b837e46
Author: Russell Bryant [email protected]
Date: Fri Nov 1 17:29:13 2024 +0000

[CI/Build] Add actionlint error matcher to job paths

Run the actionlint job if the error matching config changes.

Signed-off-by: Russell Bryant <[email protected]>

commit 18fc405
Author: Russell Bryant [email protected]
Date: Fri Nov 1 17:43:15 2024 +0000

[CI/Build] Move codespell to its own job

Move `codespell` out of the `ruff` job because `codespell` checks more
file types than `ruff`. This one needs to run on doc-only changes,
whiel `ruff` does not.

Signed-off-by: Russell Bryant <[email protected]>

commit 8dbb595
Author: Russell Bryant [email protected]
Date: Fri Nov 1 17:44:34 2024 +0000

[CI/Build] Only run the ruff job with one Python version

Ruff itself is not a Python tool, so running it in 5 different Python
environments doesn't provide much value, other than ensuring
`requirements-lint.txt` can be installed in all of these versions.
That doesn't seem like enough value to run 5 copies of the job all the
time.

Coverage using different Python versions makes more sense for jobs
that are actually running the code.

Signed-off-by: Russell Bryant <[email protected]>

commit 3942c38
Author: Russell Bryant [email protected]
Date: Mon Nov 4 20:57:39 2024 +0000

Run mypy job when pyproject.toml changes

Signed-off-by: Russell Bryant <[email protected]>

commit faff4e6
Author: Russell Bryant [email protected]
Date: Mon Nov 4 20:59:52 2024 +0000

Only run yapf with Python 3.12

There does not seem to be much benefit of running several versions of
this.

Signed-off-by: Russell Bryant <[email protected]>

Update the `clang-format` job to only run when relevant files have
changed: `*.h`, `*.cpp`, `*.cu`, or `*.cuh`. This matches the patterns
in the `find` command in this job. Also run the workflow when the
workflow definition itself changes.

Signed-off-by: Russell Bryant <[email protected]>
Only run the `mypy` CI job when the following paths change:

- any Python file, `*.py`
- the workflow definition
- `tools/mypy.sh`

Signed-off-by: Russell Bryant <[email protected]>
Only run the `ruff` CI job when relevant paths change:

- the workflow file, or the error matching config
- pyproject.toml
- any Python file, `*.py`
- requirements-lint.txt

Signed-off-by: Russell Bryant <[email protected]>
Only run `yapf` job when the following paths change:

- any python file, `*.py`
- the workflow definition file

Signed-off-by: Russell Bryant <[email protected]>
Run the actionlint job if the error matching config changes.

Signed-off-by: Russell Bryant <[email protected]>
Copy link

github-actions bot commented Nov 1, 2024

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can do one of these:

  • Add ready label to the PR
  • Enable auto-merge.

🚀

@mergify mergify bot added the ci/build label Nov 1, 2024
Move `codespell` out of the `ruff` job because `codespell` checks more
file types than `ruff`. This one needs to run on doc-only changes,
whiel `ruff` does not.

Signed-off-by: Russell Bryant <[email protected]>
Ruff itself is not a Python tool, so running it in 5 different Python
environments doesn't provide much value, other than ensuring
`requirements-lint.txt` can be installed in all of these versions.
That doesn't seem like enough value to run 5 copies of the job all the
time.

Coverage using different Python versions makes more sense for jobs
that are actually running the code.

Signed-off-by: Russell Bryant <[email protected]>
There does not seem to be much benefit of running several versions of
this.

Signed-off-by: Russell Bryant <[email protected]>
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) November 5, 2024 02:11
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 5, 2024
@russellb
Copy link
Collaborator Author

russellb commented Nov 5, 2024

I mentioned this on Slack, too, but this isn't merging because the repo is still configured to expect ruff (3.8) to pass before it merges. That requirement must be updated to expect ruff (3.12).

@DarkLight1337 DarkLight1337 merged commit 731aec5 into vllm-project:main Nov 5, 2024
44 checks passed
russellb added a commit to russellb/vllm that referenced this pull request Nov 6, 2024
A previous PR, vllm-project#9928, did some refactoring to run certain things only
when relevant files have changed. One change was to move `codespell`
into its own workflow since it applies to both code and docs.
Unfortunately, that commit removed it from the `ruff` workflow, but
then I forgot to `git add` the new workflow file. It's included here.

Signed-off-by: Russell Bryant <[email protected]>
russellb added a commit to russellb/vllm that referenced this pull request Nov 6, 2024
A prior PR, vllm-project#9928, modified this workflow to only run when relevant
files have changed. However, github is configured to expect and
require this workflow to run and pass before a PR can be auto-merged.
GitHub configuration is not quite flexible enough to say "require this
workflow to pass before merging, but only if we plan to run it."

This change is the simplest: just run it always. The change also
leaves a comment in the workflow explaining why it is this way.

An alternative in the future could be to switch to a different
framework for doing auto-merges. Mergify, which is already in use for
some things, has flexible enough policy configuration to capture more
complex merge criteria. Swithing to it just because of this is not
necessary.

Signed-off-by: Russell Bryant <[email protected]>
JC1DA pushed a commit to JC1DA/vllm that referenced this pull request Nov 11, 2024
sumitd2 pushed a commit to sumitd2/vllm that referenced this pull request Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants