Skip to content

Conversation

tmuttaki
Copy link

@tmuttaki tmuttaki commented Sep 18, 2025

Purpose

Related Issue: #23452

Tests will need to be re-run regardless in this case so it's better to fail and have the PR author fix linting issues than proceed to run the whole job.

It would be good to still run pre-commit in parallel so the total running time isn't impacted, just abort other tests (if possible).

Test Plan

Run pre-commit checks on bad code and see if it aborts the pipeline.

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: Tahmid Muttaki <[email protected]>
@mergify mergify bot added documentation Improvements or additions to documentation ci/build labels Sep 18, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a pre-commit check step into the Buildkite CI pipeline to abort builds early if linting fails. This is a great improvement for faster feedback cycles. The implementation adds a new step that installs and runs pre-commit. I've suggested a small improvement to the commands in the new CI step to use the pinned version of pre-commit from requirements/lint.txt and to remove an unnecessary command. The test files with intentional linting errors are well-suited for verifying this new CI functionality. One question: are the new test files (test_precommit_issues.md and test_precommit_issues.py) intended to be removed before merging, or should they remain in the repository?

Comment on lines 38 to 40
- pip install pre-commit
- pre-commit install
- pre-commit run --all-files --hook-stage manual
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The pre-commit install command is not necessary in a CI environment, as it's used to install git hooks for local development. In CI, you only need to run the checks. Additionally, it's a good practice to install dependencies from requirements/lint.txt to ensure the version of pre-commit is pinned and consistent with what developers would use locally. This avoids potential discrepancies between local and CI environments.

  - pip install -r requirements/lint.txt
  - pre-commit run --all-files --hook-stage manual

Signed-off-by: Tahmid Muttaki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant