Added toml-sort tool

[jamesbraza]

Added the toml-sort to pre-commit to keep things clean in pyproject.toml

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c528f78) 82.48% compared to head (6bc6845) 82.49%.

❗ Current head 6bc6845 differs from pull request most recent head 91178f0. Consider uploading reports for the commit 91178f0 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1972      +/-   ##
==========================================
+ Coverage   82.48%   82.49%   +0.01%     
==========================================
  Files          66       66              
  Lines        8165     8147      -18     
==========================================
- Hits         6735     6721      -14     
+ Misses       1430     1426       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Wauplin]

Thanks for the suggestion @jamesbraza :) If we're going for toml-sort, let's fully embrace it. What I would do:

1. add toml-sort to setup.py (along ruff)
2. add toml-sort --in-place pyproject.toml to the make style command (in makefile)
3. add toml-sort --check pyproject.toml to the make quality command (in makefile)
4. add toml-sort --check pyproject.toml to the python-quality.yml github workflow file

And I think that's it. Thanks in advance!

(also since #1971 has been merged you have a small merge conflict to resolve)

[jamesbraza]

Hi @Wauplin, thanks for suggestions and being open! I think running toml-sort each make style/quality doesn't make sense because most of the time, sorting TOML files won't be relevant. Having this tool invoked each time will just slow the process down for local developers.

To share, this is something pre-commit is good at, because it has a files filter. However, pre-commit is oriented around single use, so think either always fixing or always checking. So it's hard to use the same config for one being make quality (read only) and one being make style (can write). It looks like the current pre-commit config here has some autofix rules like trailing-whitespace and runs ruff in check mode. It also seems the current pre-commit config is mostly unused.

I think the best path forward would be making two pre-commit configs:

• One .pre-commit-config-check.yaml emulating make quality that is "read only"
• Default .pre-commit-config.yaml emulating make style that can write

Then you just change make style and make quality to invoke pre-commit:

quality:
	pre-commit run --all-files --config .pre-commit-config-check.yaml

style:
	pre-commit run --all-files

What are your thoughts on this?

[Wauplin]

I think running toml-sort each make style/quality doesn't make sense because most of the time, sorting TOML files won't be relevant. Having this tool invoked each time will just slow the process down for local developers.

While I understand the concern of not slowing down too much the developer experience, I still think it's fine to run it on every make style/make quality. I did some measurements and those 2 commands take on average ~160ms and ~600ms respectively. Adding the toml-sort call would add 50ms in both cases which is still ok in my opinion. What takes the most time in make quality is mypy (by far) so an additional toml sort check is ok. For make stlye, let's add it and if we really see that it's taking too much time, we could have a make style with only ruff by default and a "make style-all" with the additional toml-sort and python scripts calls (for context, we have some under-optimized python scripts running on each make style which is the part that takes most of the 160ms described above). I don't say we shouldn't care about speed here but I think ~200ms is still reasonable. The advantage being that it would integrate with the current developer workflow.

[jamesbraza]

Alright, I rebased the tool atop main.

For the most part, toml-sort is a "one time" tool -- 99.9% of the time it will be a no-op, so it seems wasteful to add it into every invocation of make style/quality/CI, even if it only takes 50-ms.

I guess I think adding three toml-sort invocations is worse than adding one into pre-commit, and having pre-commit skip over it because of the files filter.

I will leave this PR in your hands. I think it's fine to merge as-is, and just know the sorting will eventually go stale unless integrated in CI.

[Wauplin]

Thanks @jamesbraza and understand you concern about running toml-sort for nothing. Let's merge this and if we really want to run toml-sort all the time in the future, we'll do.
Thanks again for the suggested updates in the tooling! It's easy not to think about the tooling when maintaining a project on the long run but it's still important! 😄