Pre-Commit

[Nils-ChristianIseke]

#4769
Suggestion for a pre-commit configuration.
I've taken the one from ros2 control as a basis and added a few things I am using myself. I still need to put some work into it, but I wanted to provide this in advance as a basis for discussion.

For example, do we want to introduce ruff and maybe mypy?
What about other hooks?
How should we integrate that into the CI?

[SteveMacenski]

I went through about a quarter of the changes, however I think the vast majority of the python3 changes need to be reverted. It creates hundreds of linting errors for flake8 / pep57.

For example, do we want to introduce ruff and maybe mypy?

What are the advantages of these tools? Keep in mind we have some python3 linters already with pep57 / flake8 that come from ament that we use (and have CI jobs that run)

How should we integrate that into the CI?

We have the linters already in our CI, but it might be value to add in codespell or some others that not already running as github actions. I know we added this in Nav2 Docs recently with precommits in CI so you can use it as a template: https://github.com/ros-navigation/docs.nav2.org/blob/master/.github/workflows/precommit.yaml

What about other hooks?

Any suggestions @tonynajjar ?

[Nils-ChristianIseke]

Thank you Steve. Sorry for not pointing out, that the changes are just a preview of what would change with the current preliminary config. Did not meant to create review noise for you.

We have the linters already in our CI, but it might be value to add in codespell or some others that not already running as github actions.

I would vote for using pre-commit as drop in replacement for the lint workflow. Running the linters as pre-commit hooks.

What are the advantages of these tools? Keep in mind we have some python3 linters already with pep57 / flake8 that come from ament that we use (and have CI jobs that run)

I like ruff mainly because of it's speed and support for multiple linter and formatters
Regarding linting this would need configuration of ruff to match ament_flake8 and ament_pep257, however maybe that is an overkill and we should wait for an ament_ruff package. Mypy introduces static type checking to python code, and would be a must if we wanted to enforce python type hints via ruff.

There also exiast a topic on ros discourse.

[SteveMacenski]

I would vote for using pre-commit as drop in replacement for the lint workflow. Running the linters as pre-commit hooks.

One nice thing about that though is that they each show up in the GitHub UX as a separate job, which makes it easier for a developer to see where the error is and break down a laundry list of tasks into more bite-sized results (and the jobs are named after the task for immediate review knowledge about the nature of the failure). Is there a way to do something like that with precommit?

An option we could have is to have the precommit mirror what we check with CI jobs and developers can enable precommit locally to run the workflow, but the workflows in CI run with inidividual jobs that match the set of precommit jobs (so we'd need to add a codespell, etc in our linting workflow).

Perhaps not ruff since we have pep57 and flake8. I try to keep this project fully aligned with the ROS 2 best practices as a reference for other applications.

Mypy introduces static type checking to python code, and would be a must if we wanted to enforce python type hints via ruff.

That sounds nice! Is there much we'd have to do to implement that? We honestly don't have much python anyway, so adding type hints on the returns of each method isn't actually a big deal (I think we can/should ignore /tools and launch files).

[Nils-ChristianIseke]

One nice thing about that though is that they each show up in the GitHub UX as a separate job, which makes it easier for a developer to see where the error is and break down a laundry list of tasks into more bite-sized results (and the jobs are named after the task for immediate review knowledge about the nature of the failure). Is there a way to do something like that with precommit?

The output of pre-commit is visible in the summary of a job. I normally setup git hooks in my dev env, so that i am not allowed to commit if pre-commit fails (output of pre-commit looks the same). Not sure if there is a pretty way to run each pre-commit hook as seperate job.

An option we could have is to have the precommit mirror what we check with CI jobs and developers can enable precommit locally to run the workflow, but the workflows in CI run with inidividual jobs that match the set of precommit jobs (so we'd need to add a codespell, etc in our linting workflow).

Wouldn't be maintaining parity between pre-commit and CI jobs an overhead?

That sounds nice! Is there much we'd have to do to implement that? We honestly don't have much python anyway, so adding type hints on the returns of each method isn't actually a big deal (I think we can/should ignore /tools and launch files).

I will open a separate issue regarding this.

[SteveMacenski]

The output of pre-commit is visible in the summary of a job. I normally setup git hooks in my dev env, so that i am not allowed to commit if pre-commit fails (output of pre-commit looks the same). Not sure if there is a pretty way to run each pre-commit hook as seperate job.

That's not automatic, they manually call it from a github action workflow https://github.com/ros-controls/ros2_control/tree/5d4933d2945b72bf2625a2a28c98107433c31d9d/.github/workflows. I agree turning it on in the dev environment makes sense for those that want to use it! I think that's the intent behind the ticket.

Wouldn't be maintaining parity between pre-commit and CI jobs an overhead?

Sure, but if in exchange our CI has more clear failures to help developers fix those issues, I think that's worthwhile than piling everything into a single job that more junior engineers need to parse to find their issue(s). Keep in mind there's a wide array of developers that contribute to ROS 2 from the most junior writing the first bits of code in their career to Senior Staff Engineers. There are quite a few things in our process that are catered to the junior developers and hobbyists to lower the technical and process barriers of entry.

Plus, its not like we modify linting often. We have never actually made an update to our lint.yml job since we created it (beyond changing the image OS for the ros release). I don't think that's a major overhead :_)

[Nils-ChristianIseke]

So I tested a few things, but apparently the logs from ament_flake8 and ament_pep257 are too noisy to really work well with pre-commit. Maybe that's one of the reasons why the ros2_control people don't use ament for these.
An alternative would be to use flake8 and pep257 in pre-commit, though I'm not sure how much maintenance this would require to match the ament counterparts in the lint workflow.

We could also decide to only run tools via pre-commit (e.g. codespell) that are not present in the lint workflow.

Apart from that, I have had very good experiences with pre-commit and junior developers as they get feedback earlier and there is less “push and pray”.

[SteveMacenski]

An alternative would be to use flake8 and pep257 in pre-commit, though I'm not sure how much maintenance this would require to match the ament counterparts in the lint workflow.

That is actually quite a bit of work, those profiles change not super infrequently - once every distro or so.

We could also decide to only run tools via pre-commit (e.g. codespell) that are not present in the lint workflow.

Sure, but at that point, why not just have a CI job do the additional items like codespell?

Though, Nav2 doesn't have alot of python in general, so I think it would be OK to exclude the python linters from precommit if its just those and not the C++ ones. Don't let the enemy of 'good' be 'great' considering we have vastly more C++.

Apart from that, I have had very good experiences with pre-commit and junior developers as they get feedback earlier and there is less “push and pray”.

Personally, I actually run the tests before committing for the most part or manually execute ament_XYZ on the CLI to check. They really shouldn't need to push in order to get feedback. colcon test or ament_cpplint will tell them what they need to know. I know this is automatic so it is nice, but I'm more saying that there are pretty straight forward ways of doing this without needing to use CI as ground-truth that you cannot reproduce locally. Not running the tests when changing alot of code is treating CI as a proxy for good process - if they're not doing that to see the linter failures (let alone functional breakages), that is a a problem. I'm not saying I'm perfect about always doing it, but I do it more than not when making non-trivial updates.

[tonynajjar]

The motivation behind my ticket was simply the frustration of finding out about linting issues "a little bit too late". It's still not terrible since the linters run first, but still the sooner the better. Putting the discussion of which additional linters to have aside for a bit, I think just having the lint.yaml CI job run locally is already a win. Since I don't think running a CI job locally is common/feasible/popular, the next best thing is to use pre-commit to run the same linters. And that's exactly what this project does: https://github.com/botsandus/ament_lint_pre_commit/blob/main/.pre-commit-hooks.yaml. We could just use that.

[Nils-ChristianIseke]

Thank you @SteveMacenski for sharing your knowledge/ thoughts, i really appreciate this :). I have posted a minimal config file.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

see 3 files with indirect coverage changes

[SteveMacenski]

I do like the idea of adding static type checking for python3 & code spell in our CI and/or precommit. That's a good quality move! Can you add those into this PR? I think it would be good both in a CI job as well as the precommit

I think the lint.yml can have the codespell and mypy run, no? Then they also run as fast, parallel jobs here as well

[mergify]

@Nils-ChristianIseke, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[SteveMacenski]

LGTM in general -- anything else you'd want to add here?

@tonynajjar please review the precommit config file, I can do the rest

[SteveMacenski]

There are a few PGM, SVG files that this modifies. We should make a rule to skip svg/pgm files since we'll have maps and svg design docs that shouldn't be modified

[SteveMacenski]

The linters aren't running for some reason

[Nils-ChristianIseke]

Hi Steve I will ping you as soon as it's ready to review :)

[mergify]

This pull request is in conflict. Could you fix it @Nils-ChristianIseke?

[Nils-ChristianIseke]

@SteveMacenski You can have a look now. There are some open questions from my side:

1. Using https://pre-commit.ci/ (autoupdates pre-commit hooks, autofixes prs), however that would need further modification to get ament stuff running in ci?
2. Are you ok with adding mypy support later, as it is quite an effort to add the missing type hints?
3. Should we enforce en-GB_to_en-US for codespell?
4. I'm still having a bit of trouble with the setup (ament in ci worflows and in pre-commit). I'm wondering if we should keep the ament stuff in pre-commit and only do additional stuff in pre-commit.

[SteveMacenski]

Can we skip all json and css files? These are autogenerated

[SteveMacenski]

his needs to be updated since you changed the value in palnner_plugins. This is a test, so easy enough. Just change here to canceled and remove the codespell ignore

[SteveMacenski]

css / json

[SteveMacenski]

@tonynajjar can you review just this file? I did the rest but I could use your eye on this to make sure you're also happy with it

[SteveMacenski]

Using https://pre-commit.ci/ (autoupdates pre-commit hooks, autofixes prs), however that would need further modification to get ament stuff running in ci?

I'm still having a bit of trouble with the setup (ament in ci worflows and in pre-commit). I'm wondering if we should keep the ament stuff in pre-commit and only do additional stuff in pre-commit.

I think we decided that precommit would be for local users only, not for CI, no? Or maybe I'm missing what you're saying

Are you ok with adding mypy support later, as it is quite an effort to add the missing type hints?

Yes! I thought I was a PR that did some of this already, but I'm having a hard time finding it. Maybe it was a comment in the ticket

Should we enforce en-GB_to_en-US for codespell?

Up to you, I don't feel strongly about it

---

Overall, the last few review comments are tiny, I'm largely happy with this!

[Nils-ChristianIseke]

I think we decided that precommit would be for local users only, not for CI, no? Or maybe I'm missing what you're saying

Sorry, I had a typo: We could only execute things via pre-commit that are not available in the current ament workflows: Codespell and pre-commit-hooks.
I would vote not to run the ament tools in pre-commit as they are currently quite noisy and prevent me from using pre-commit (Independent of running pre-commit in the CI or not).

If we do so not much additional setup would be needed for CI. This would also reopen the CI discussion, as we have mainly been discussing whether pre-commit should replace the current CI workflow. And with this setup pre-commit would be neither a replacement nor a duplication but would run as a supplement in the CI.

But if your earlier concern regarding pre-commit in CI was in general, then my bad for bringing it up again! Just wanted to make sure we’re on the same page.

[SteveMacenski]

I'd be interested in Tony's thoughts, but personally I just run tests locally in my workflow so this feature isn't for me (but love adding codespell into our CI workflow!) :-)

Is there a way to silence precommit steps that don't have error output? Somes docs review makes me think that verbose: false or running it work pre-commit run -q will silence stages that have no errors. Would that meet your issue need?