ci: Move formatting and linting checks out of tests.yml

[silvanocerza]

Proposed Changes:

Move mypy and pylint out of tests.yml into a separate linting.yml workflow.

This also remove the dependency in unit-tests, unit-tests-linux, unit-tests-windows and rest_api jobs, so they're going to run only if black passes.

tests.yml has not been modified further.

A linting-skipped.yml has been added too that is run when the jobs in linting.yml are skipped, this is done to handle the required mypy and pylint checks as specified by the official Github documentation.

How did you test it?

I statically checked the workflow using actionlint.

Notes for the reviewer

I chose to run mypy and pylint only on PRs and not on main since it's a protected branch.
It should never happen that unformatted code is pushed directly to main so it shouldn't be necessary to run checks on it too.

Checklist

• I have read the contributors guidelines and the code of conduct
• I have updated the related issue with new insights and changes
• I added tests that demonstrate the correct behavior of the change
• I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
• I documented my code
• I ran pre-commit hooks and fixed any issue

[silvanocerza]

Stopped checks as am waiting for #4043 to fix pylint failures.
Will run them again after it's merged in main.

[masci]

Tests were depending on linting by design, to avoid wasting CI time in case the code isn't even well formatted. What's the strategy here?

[silvanocerza]

Tests were depending on linting by design, to avoid wasting CI time in case the code isn't even well formatted. What's the strategy here?

I was completely disregarding the runtime to be fair. I don't even have access to our consumptions as of now.

I would trust most people to run those tools locally with hooks or some other ways.
If we remove the dependency we cut ~7 minutes from total tests runtime. It's a pretty nice gain for a workflow that takes almost one hour.

Am I too hopeful? What do you think if any failure in this workflow stops the tests one?

[masci]

I was completely disregarding the runtime to be fair. I don't even have access to our consumptions as of now.

Being an OSS project we have plenty of CI time for free but it's not about the money, it's about good engineering practice and environmental awareness

I would trust most people to run those tools locally with hooks or some other ways.

Maybe I'm old but I would not trust people :)

If we remove the dependency we cut ~7 minutes from total tests runtime. It's a pretty nice gain for a workflow that takes almost one hour.

Am I too hopeful? What do you think if any failure in this workflow stops the tests one?

7 minutes for linting are too much, I agree. What if we remove mypy and find a way to make pylint faster?

[silvanocerza]

7 minutes for linting are too much, I agree. What if we remove mypy and find a way to make pylint faster?

Nevermind I was wrong, it takes ~7 minutes if the cache is invalid and it must reinstall dependencies. It should be no more than 2 minutes if cache is valid.

My idea was splitting tests workflows depending on when they should run. Formatting and linting checks don't make much sense to me when run in main.

If the code is merged I assume it's formatted and checked correctly already, I don't see the value of checking in main too. They may certainly fail in main since we don't pin those tools versions and updates might break them, but these kind of failures I think generate mostly noise and annoyances when in main.

We could skip them in main only and still keep the dependency in tests.yml, but that might be confusing maybe?

Also one of my concerns is the tests.yml size, having all in a single file makes it harder to read and edit, making changes difficult and error prone. Moving things to separate files would help quite a bit I think.

If you feel that the dependency is a necessity I can ditch this solution with no issues, I'll find a different way.

[julian-risch]

I strongly prefer to keep the dependency. I am sure not every community member has the precommit checks installed. And if CI runs all the tests again and again while someone tries to fix mypy issues it's a waste of compute resources. To give another example when it's useful: For very small changes, one might edit files in the browser and there are no precommit checks in this case.

[julian-risch]

Regarding having the checks on main or not, I find your arguments convincing. 🙂 Every change we commit to main is checked already on the feature branch before merging into main.
However, currently we don't always merge main into our features branches and run the tests again there before merging the feature branch into main. So the merge of main and feature branch might get never tested in that case, correct? We could run into problems in some rare cases because of that.
Otherwise, I'd be okay with skipping the checks in main only and still keeping the dependency in tests.yml. It's only a small speedup though so better keep the checks also on main?

[silvanocerza]

@julian-risch I had a discussion with @masci and @ZanSara on this and we decided to keep the black check has dependency for the tests. It's fast and it doesn't require to install all the dependencies so it's a good enough safe guard.

Instead we're still gonna move mypy and pylint into a separate workflow that runs only in PRs.

However, currently we don't always merge main into our features branches and run the tests again there before merging the feature branch into main. So the merge of main and feature branch might get never tested in that case, correct? We could run into problems in some rare cases because of that.

That wouldn't be an issue I think, if merging causes linting or formatting issues probably there are also conflicts so tests won't run preventing the merge. Also it would be a pretty rare occurence.

In the end the important thing is that unit and integration tests are green.

[julian-risch]

Instead we're still gonna move mypy and pylint into a separate workflow that runs only in PRs.

Alright then, let me know, when you need another review.

[julian-risch]

by the way, feel free to re-assign the reviewer(s) if the others have more context. 👍

[ZanSara]

Looks good! Black should be a sufficient gatekeeping mechanism at preventing the CI to start on PRs with syntax errors and other "obvious" mistakes. Let's try this strategy out!

[ZanSara]

Just wondering here: how could we keep in sync this version, the dev version, and the pre-commit hooks version? I imagine black to be fairly stable, so not a big issue right now. It's just a "theoretical" problem that maybe we'll need to address in the future

[silvanocerza]

Probably we could have a [linting] or [formatting] section in pyproject.toml that is installed with [dev] too and install only that.

But as you said it's pretty stable, I wouldn't be much concerned by this.

[ZanSara]

Really cool to see how you handle skipping required workflows! We should apply this technique to many more cases 👀

[silvanocerza]

We will. 👀