build: use ruff to replace yapf, flake8 nbqa and docformatter

[martibosch]

As mentioned in #1078, I think it would be a great idea to use a deterministic formatter such as ruff (which includes almost all the features of yapf, flake8, nbqa and docformatter).

I would further suggest to set up pre-commit.ci so that pre-commit runs within pull requests. This would result in clearer diffs (I also encountered some import order differences in #901). Of course this would require first running pre-commit run --all-files to apply the rules to all files, which will result in a huge commit, but I think it will be benefitial on the long run.

PS: Running it locally, I found several "optional" corrections, e.g. "E712 Avoid equality comparisons to True; use results.match: for truth checks" or "E402 Module level import not at top of cell", but also more important ones such as "F841 Local variable bounds is assigned to but never used" or "F821 Undefined name plot_name`". We could further configure ruff to see which rules we want to apply.

[ethanwhite]

Thanks for this contribution @martibosch! I like the approach, but I'd also like to hear from @henrykironde & @jveitchmichaelis on this one.

The one thing I'm a little more initially hesitant about is relying on pre-commit.ci since it seems really new. I not necessarily opposed to it, but I think we'd at least want to keep our GitHub Actions style checks in place. Given that, can you also modify the style checking in https://github.com/weecology/DeepForest/blob/main/.github/workflows/Conda-app.yml to match your proposed pre-commit changes (this should also get the tests passing).

If we end up merging this then lets coordinate on the timing of the big commit fixing ruff issues throughout the code base just to minimize refactoring time on other big PRs.

[jveitchmichaelis]

We do soft-enforce pre-commit hooks so maybe we could get 90% of the way there by just running pre-commit as an Action step and fail if it returns a bad exit code? Then the contributor is responsible for fixing.

As far as I can see, precommit.ci just automates that fix and I guess modifies the commit, but as a developer I don't think re-add + re-commit takes much time. Is the main benefit that developers don't need to run/set it up locally?

Otherwise I'm all for automating things that we'd spend time worrying about in reviews. As I mentioned, I've defaulted to using isort+black in the past, but ruff looks great (Astral's stuff is usually nice).

[henrykironde]

I will try these options you have mentioned putting in mind the current state of things, I will let you know what my opinion is soon.

[jveitchmichaelis]

You should also remove the yapf step in the current .github/actions workflow (and replace with ruff).

[henrykironde]

It looks like we're heading in this direction based on my initial research. I'm not done yet, currently testing some changes from this setup, but a few of them seem unusual. I’ll take a deeper look and review everything more on my folk. I believe we may end up having more setting to deal with than the general settings.

[martibosch]

Hello @jveitchmichaelis, for some reson I cannot see your requested changes (maybe because there is still one pending review?). In any case, at first I was also using pre-commit in GitHub Actions but then I moved to pre-commit.ci, which in my view has the main advantage of auto-updating the hooks. But the rest is essentially the same except that pre-commit.ci would show as a separate check rather than as a step in the CI workflow.

Eitherway should be fine. If you prefer running pre-commit in GitHub actions, I can change the two yapf checks in Conda-app.yml for pre-commit check. If you prefer pre-commit.ci, I will instead remove the two yapf checks completely.

[ethanwhite]

If you prefer running pre-commit in GitHub actions, I can change the two yapf checks in Conda-app.yml for pre-commit check. If you prefer pre-commit.ci, I will instead remove the two yapf checks completely.

I think we want to change the yapf checks, not remove them. Even if we end up including pre-commit.ci I think we want this in place as a fall back since pre-commit.ci is so new

[jveitchmichaelis]

I couldn't see a way to flag the file for review if it's not touched by the commit, but yes - either route we go, the yapf steps need to be removed from the workflow for this PR as it's no longer installed. I also think having a backup isn't a bad idea. If we can specify the config in both places, the results should be identical. I assume pre-commit would use the definition in the repository, so by definition it ought to be the same.

Anecdotally, I've never found any of the existing tools to be slow for formatting. Type checking/static analysis, yes, because the analyzer has to figure out all the references. However at the moment we don't have consistent type hinting and it might be nice to move towards that.

[martibosch]

I couldn't see a way to flag the file for review if it's not touched by the commit, but yes - either route we go, the yapf steps need to be removed from the workflow for this PR as it's no longer installed.

I replaced the yapf and docformatter step (both superseded by ruff) by pre-commit/action (which in turn runs the ruff linting and formatting hooks).

Anecdotally, I've never found any of the existing tools to be slow for formatting. Type checking/static analysis, yes, because the analyzer has to figure out all the references. However at the moment we don't have consistent type hinting and it might be nice to move towards that.

Actually astral recently released ty, which is still in very early alpha stages but worth keeping track of.

[ethanwhite]

@martibosch - looks like the tests are failing due to an interaction between the precommit action and the fact that we're running all of our tests using uv. If there isn't a simple fix here I'd be OK with just replicating the ruff command in place of the yapf command

[martibosch]

indeed we could run the ruff commands instead of using pre-commit. Alternatively, we could also add a uv step to install pre-commit and another one to run it.

I think the latter my be better because it would enforce the other pre-commit hooks for non Python files (e.g., remove trailing whitespaces and fixing end of files). This would become especially useful if you want to add further hooks (e.g., to format/lint rst, markdown, github actions workflows...). Otherwise we can simply run ruff.

[jveitchmichaelis]

Can we move all the Python-specific checks to ruff and not pre-commit, e.g. trailing whitespace/end of file handling?

    - id: trailing-whitespace
    - id: end-of-file-fixer

EDIT: Actually maybe not, I assume this also applies to markdown as well as other files.

I also agree on the latter point though - being able to squish things like malformed yaml/json and big files would be good to run in the CI as well.

EDIT EDIT:

I tried running the formatter locally, some notes from diffs:

• single -> double quotes (PEP allows either, but yapf uses singles)
• splitting function args to newlines (consistency here is good)
• parenthesis cleanup
• isort is not run by default (requires extend-select = ["I"]), many imports change and we should double check whether we have anything that requires a certain order.

There are some opinionated edits like this where it seems yapf doesn't care:

  (a,
  b,
  c)
  
  ->
  
  (
  a,
  b,
  c
  )

For the larger PRs we can probably run ruff format over the codebase before rebasing.

[ethanwhite]

I don't feel strongly about the specific approach as long as it doesn't slow down the tests significantly. My initial reaction to the last few comments is that if everything could be moved to ruff that would keep things as simple as possible; but if it can't then installing pre-commit with uv sounds good.

[henrykironde]

Just for reference, I am going through the changes here, you can take a look https://github.com/henrykironde/DeepForest/pull/9/files