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

enable pre-commit #452

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

antoinejeannot
Copy link
Contributor

@antoinejeannot antoinejeannot commented Jan 31, 2025

This PR (re)enables pre-commit!

It has been discussed in a few issues & PRs now (#120, #119, #431), with good reason since it definitely improves the developer's workflow.

I've already ran into a few linting & formating issues myself with just a few PRs 😅

A few points:

  • to share the same behavior locally and in the CI (which leads to pain), ruff was pinned in both the pyproject.yaml and .pre-commit-config.toml, let's bump it from time to time or setup a dependabot
  • to assess quality, only pre-commit is used (more robust thanks to its dependencies isolation), but is iso-functional with a plain ruff xyz since both rely on the pyproject.toml for configuration
  • ruff is still mentioned in pyproject.toml so that the developer and its editor code editor can pick it up from the virtualenv and apply linting / formating
  • make style is redundant with make quality, so I deprecated it with a log for those who use it
  • the quality workflow, thanks to some caching, can save up to 15s per run (when cache hit), this might definitely help the tests workflow as well
  • ruff is now configured with the lower bound of supported versions (3.10, instead of 3.12) as recommended, which probably led to some flaky linting & formating issues
  • the check tests in ci is now a .pre-commit hook

I have another PR which adds a few needed additional rules to the linter but will lead to major merge conflicts across all opened PRs lol I'll open it in a second time.

Thanks for reviewing @aymeric-roucher @albertvillanova

python-version: "3.12"

# Setup venv
- name: Setup venv + uv
Copy link
Contributor Author

@antoinejeannot antoinejeannot Jan 31, 2025

Choose a reason for hiding this comment

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

pre-commit manages its own isolated venv, hence no more env management.
Also no need to install smolagents for lint & formating, only pre-commit and ruff are needed

python-version: "3.10" # lower bound of the supported Python versions
- uses: pre-commit/[email protected]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this action uses some caching under the hood, which speeds up ci actions

# Equivalent of "make quality" but step by step
- run: uv run ruff check examples src tests utils # linter
- run: uv run ruff format --check examples src tests utils # formatter
- run: uv run python utils/check_tests_in_ci.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now a pre-commit hook

Copy link
Member

@albertvillanova albertvillanova left a comment

Choose a reason for hiding this comment

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

I would prefer to stay aligned with all the other huggingface open source libraries.

Additionally, previous attempt to use pre-commit had a compatibility issue with the CI quality job (handled with ruff), if I remember correctly.

@antoinejeannot
Copy link
Contributor Author

antoinejeannot commented Jan 31, 2025

Several other HF repos also seem to leverage pre-commit (datasets, accelerate, peft, tgi etc.).

I feel your concern about compatibility issues, and that's actually what drives this PR:
pre-commit actually runs its hooks inside their own venv, bringing robustness and reproducibility to the table.

There are a few reasons pre-commit might have been flaky so far in smolagents:

  • ruff is not stable and must be pinned, otherwise ruff 0.9.0 will differ from ruff 0.9.1 (patch): this explains why there might be some issues between the CI and local environments right now
  • ruff must be pinned in pre-commit-config.yaml to the same version as in pyproject.toml, otherwise there could be a difference as well
  • ruff's version in .pre-commit-config.yaml is outdated
  • pre-commit base hooks must run before ruff, otherwise they could introduce changes after ruff (depending on the hook) that can be committed

I am confident this will make the CI/CD more reliable, as well as smolagents more maintainable. WDYT ?

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