Updat .pre-commit hook version

[juanitorduz]

Update pre-commit hook versions:

---

Edit: This was the original scope of this PR. After #1795 (comment) we decided to unco these changes as we want to keep pre-commit usage optional :)

In this PR, I want to suggest a way to improve the development experience by adding https://pre-commit.ci/ as a unique source of truth for the project's linter.

Motivation

Currently, the CI (GitHub actions) uses the command make lint, which runs ruff's check. The problem is that it depends on the local version of ruff and is therefore ambiguous. See:

numpyro/Makefile

Lines 3 to 6 in 2b85765

	lint: FORCE
	ruff check .
	ruff format . --check
	python scripts/update_headers.py --check

Suggested Solution

In other projects I have contributed to, we have seen the benefit of having this manager by https://pre-commit.ci/ to:

1. Make sure we have the package's specific versions (revisions). See Add pre-commit-ci py-econometrics/pyfixest#375
2. Have periodic automatic PR updates. See, for example, [pre-commit.ci] pre-commit autoupdate py-econometrics/pyfixest#417

The only thing you (NumPyro devs) need to do is to add the app to the repo (one click as described in https://pre-commit.ci/)

If we merge this one, we could even remove the lint from GitHub actions (see py-econometrics/pyfixest#397), and the linter would be managed independently. That is, remove

numpyro/.github/workflows/ci.yml

Lines 35 to 37 in 2b85765

	- name: Lint with ruff
	run: |
	make lint

[fehiepsi]

Hi @juanitorduz, just want to double check my understanding:

• this suggests that we should add pre-commit to the dev dependencies
• when a user makes a commit, ruff will report and fix the lint issues automatically
• occasionally, pre-commit-ci will make PRs to update the ruff version
• what happens if the local ruff version is mismatched?
• what happens if the local version is up-to-date but the pre-commit is out-dated?
• make lint now does the fix on numpyro and tests folders, so its behavior is changed
• Fix ruff not format the changes #1761 and Fix ruff not fixing linter errors #1764 are incorporated somehow

[juanitorduz]

Hey @fehiepsi, thanks for your feedback. Here are some comments:

• this suggests that we should add pre-commit to the dev dependencies

Yes! I just added it ✅

• when a user makes a commit, ruff will report and fix the lint issues automatically

Yes! The user has to simply do once pre-commit install and then every commit will be checked. One can skip this check by adding the -n option. For example git commit -m"my commit without check' -n

• occasionally, pre-commit-ci will make PRs to update the ruff version

Yes. They look like py-econometrics/pyfixest#417

• what happens if the local ruff version is mismatched?

In principle, once you do pre-commit install there is a separate environment with the required packages and versions (revisions) to run the pre-commit hooks. If you want to have it in your environment as well (say, for the IDE like VSCode), you can pip install ruff with the version from the pre-commit config (this is the unique source of truth ! This is why it is recommended to avoid ambiguities). If two different versions are installed, the one specified in the pre-commit config will be the one to be applied.

• what happens if the local version is up-to-date but the pre-commit is out-dated?

As mentioned above, the pre-commit environment will always be in sync with the version from the config. This is the version in which all the checks (locally and in CI) will be used. If you install it in your environment, one needs to install the version from the .pre-commit config (only source of truth)

• make lint now does the fix on numpyro and tests folders, so its behavior is changed

The checks will be done on whatever is specified in

numpyro/.pre-commit-config.yaml

Line 8 in 1d0cedb

files: "numpyro/.*|tests/.*"

It's up to us to decide which folders to consider. At the moment is doing it in both (numpyro and test) and checks are 🟢 :)

• Fix ruff not format the changes #1761 and Fix ruff not fixing linter errors #1764 are incorporated somehow

The ruff configurations from the pre-commit hooks are consistently specified in https://github.com/pyro-ppl/numpyro/blob/master/pyproject.toml. This is why they are incorporated.

To test this PR (and see if this is something you want to have), I suggest you create a fresh Python environment and make a dummy change without installing ruff and see how everything is taken care of by the pre-commit hook. Of course, I am here to answer all questions (I might not be able to answer edge cases, but we can learn together :) )

[fehiepsi]

Thanks, @juanitorduz! Just a couple of thoughts:

• since the introduction of ruff, we have made some release with failing lints because CI was green. ruff did not format and fix the codes properly because of wrong configurations (see above PRs). I'm not sure if this precommit change will introduce other wrong configurations.
• the need to fix lint before committing is inconvenient in some cases. I would prefer committing temporary code before fixing lint issues (when the changes are large). Could we make this optional?
• I guess we also need to check lint in other folders like examples, notebooks as well
• typically, make lint is used to check for issues. make format will try to fix them, if possible.

[juanitorduz]

Thanks for the clarification. Here are some answers to your valid concerns:

• since the introduction of ruff, we have made some release with failing lints because CI was green. ruff did not format and fix the codes properly because of wrong configurations (see above PRs). I'm not sure if this precommit change will introduce other wrong configurations.

I am sorry to hear! Feel free to open issues if you see this happening again and I will take a look at it. In my experience, having everything synced with pree-commit does help (this is just a personal opinion based on previous experiences)

• the need to fix lint before committing is inconvenient in some cases. I would prefer committing temporary code before fixing lint issues (when the changes are large). Could we make this optional?

This makes sense when developing. As described before you can add the -n and the checks won't be done.

• I guess we also need to check lint in other folders like examples, notebooks as well

Indeeed! Some projects do it and some projects do not. If you want to do it we can have a separate issue / PR to tackle this and atdd the examples folder to the ruff configurations.

• typically, make lint is used to check for issues. make format will try to fix them, if possible.

In view of this observation, I moved the pre-commit to make format in 78b77e9

@fehiepsi, think about the proposed changes, and if you are not convinced, it is totally ok (this was just a suggestion). We can close this one and come back once we are ready :)

[fehiepsi]

Thanks for your clarifications, @juanitorduz! I'm also learning. I think we will want to make this optional for contributors who prefer using precommit, rather than requiring it.

Regarding the configuration, the upstream doc seems to suggest that an entry for ruff format needs to be specified in the yaml file. In addition, currently our make lint and make format cover all folders so precommit is lacking some checks.

How about just enhancing precommit in this PR?

[juanitorduz]

Sure! Let's do it! As we want to keep pre-commit optional, I just left the update in the pr-commit-hooks version. I think is then better to leave the ruff local version and remove the automated PRs as this is not relevant for all users :)

[fehiepsi]

LGTM. It would be nice if you can update the precommit to include the id ruff-format and support for other folders like notebooks and examples. This will make precommit aligns with what make format does. But it is fine if you just want to use it to fix lint (not format) issues on numpyro and tests folders.

[juanitorduz]

LGTM. It would be nice if you can update the precommit to include the id ruff-format and support for other folders like notebooks and examples. This will make precommit aligns with what make format does. But it is fine if you just want to use it to fix lint (not format) issues on numpyro and tests folders.

Added in 0156028 :)

I also ran pre-commit run --all-files and everything is green except for the trailing spaces in 2021016 :)

[fehiepsi]

just curious: would this include subfolders, like notebooks/source or examples/cvae_flax?

[juanitorduz]

Now that you mention this, I'll actually try to remove this so that it can target all relevant files as in the make commands 👌

[juanitorduz]

ok! 😅 After some manual tests and edge cases I think 44595d0 is what we want :D

[fehiepsi]

thanks!

[juanitorduz]

Thank you for your feedback and patience @fehiepsi 🙌