Simplify pre-commit setup

[leifdenby]

This PR simplifies the pre-commit setup by:

• removing code checks for imports against external packages, these checks should be done when running ci/cd integration tests (which will be added in a separate PR) where external dependencies are installed. After this PR is merged, pre-commit will therefore only check if code is clean and self-consistent, without the need to install external dependencies (this is the same approach taken in for example https://github.com/pydata/xarray/blob/main/.pre-commit-config.yaml). Testing imports is also important, but splitting the code-cleaning out allows for much faster iteration (i.e. these linting tests will fail, avoiding the need to install all dependencies)

• pinning versions of used linting tools. The current setup is errorprone because as linting tools evolve the linting rules will update. Pinning versions ensures that we all use the same versions when running pre-commit

• moves linting tool versions to pre-commit config rather than requirements.txt. This allows pre-commit handle the linting in its own virtual env, without polluting the dev environment

• using github action to install and run pre-commit rather than our own run instructions. This ensure that pre-commit is run identically both during local development and during ci/cd in github actions.

I also think we should consider using ruff instead of flake8/pylint, here's an article on why: https://pythonspeed.com/articles/pylint-flake8-ruff/. xarray use it so this must think this is ready for primetime.

[sadamov]

I agree with all your points, thanks a lot for your work. The pre-commit looks a lot cleaner now and using local instead of bespoke hooks makes everything much more reproducible and easier to upgrade.
Before merge please have a look at:

• Why the current action appears frozen Cleaning up orphan processes
• Some smaller comments

I am a big fan uf Ruff and use it everyday when working with this repo. +++ should replace flake8, pylint and isort in a future PR.

[leifdenby]

I am a big fan uf Ruff and use it everyday when working with this repo. +++ should replace flake8, pylint and isort in a future PR.

Ok great, yes let's do that in a later PR.

[leifdenby]

@sadamov as I thought somehow linting with same versions of tools but different version of python leads to different results 😆

Why the current action appears frozen Cleaning up orphan processes

I think this must be something with the pre-commit action. I don't think it is something I can fix unfortunately. Unless you or @khintz can see an issue with my config?

[leifdenby]

Why the current action appears frozen Cleaning up orphan processes

I think this must be something with the pre-commit action. I don't think it is something I can fix unfortunately. Unless you or @khintz can see an issue with my config?

I just re-ran this test and it didn't hang. I think the github action runner must have borked somehow. The log is the same though, so maybe it was just the log sending that borked.

So, @sadamov and @khintz we just have to decide about the different linting rules that seem to apply when you change python version (even though the flake8 version is the same) 🤣

[khintz]

A very quick test could be to try a newer flake8 version?

Eg. using

- repo: https://github.com/PyCQA/flake8
   rev: 7.0.0

Instead of rev: 6.1.0

[leifdenby]

A very quick test could be to try a newer flake8 version?

Eg. using

- repo: https://github.com/PyCQA/flake8
   rev: 7.0.0

Instead of rev: 6.1.0

Yes, of course! Good idea. I'll try that now

[leifdenby]

hmm, even with the most recent version of flake8 the linting when using python3.12 is different 😞

[sadamov]

Oh that is quite annoying. I am also fine with pinning a python version for static analysis, as you describe above (e.g. 3.11). Or if we think Ruff wouldn't suffer from python-version specific issues, we could also go there directly. I don't have a strong opinion, please go ahead with your preference 👍

[leifdenby]

Oh that is quite annoying. I am also fine with pinning a python version for static analysis, as you describe above (e.g. 3.11). Or if we think Ruff wouldn't suffer from python-version specific issues, we could also go there directly. I don't have a strong opinion, please go ahead with your preference 👍

Ok, I'll give ruff a try tomorrow and see if that works the same way across different python versions (which it should since it is actually written in rust). Otherwise I say we stick with python < 3.12 for now for linting. At least we have this issue in case someone comes across issues when doing local development with python 3.12

[leifdenby]

On second thoughts @sadamov I think we should stick with flake8, pylint etc for now and replace with ruff. Otherwise this PR will make too many changes. Are you ok with me simply adding a note to the README section on "developing" to mention that there might be an issue when running linting with python3.12 and link to this PR? That way we can keep a reference this for future work.

[khintz]

Are we sure that the problem is using flake with python3.12 and not a problem of flake not detecting operations inside f-strings for python<3.12 ?

The small changes below works for python3.12 and python3.9

neural_lam/models/ar_model.py:403:53: E226 missing whitespace around arithmetic operator
Changing from

f"t={t_i} ({self.step_length*t_i} h)",

to

f"t={t_i} ({self.step_length * t_i} h)",

neural_lam/models/ar_model.py:545:66: E226 missing whitespace around arithmetic operator
Changing from

title=f"Test loss, t={t_i} ({self.step_length*t_i} h)",

to

title=f"Test loss, t={t_i} ({self.step_length * t_i} h)",

neural_lam/models/base_hi_graph_model.py:39:55: E226 missing whitespace around arithmetic operator
Changing from

title=f"Test loss, t={t_i} ({self.step_length*t_i} h)",

to

title=f"Test loss, t={t_i} ({self.step_length * t_i} h)",

[sadamov]

Good catch @khintz, there is one more operator here print(f" {level_index}<->{level_index + 1}") (in base_hi_graph_model.py)
While PEP8 gives some freedom to the user when it comes to whitespaces around operators. https://peps.python.org/pep-0008/#other-recommendations I agree that in the four cases listed above we should simply add the whitespaces in this PR.

[leifdenby]

Are we sure that the problem is using flake with python3.12 and not a problem of flake not detecting operations inside f-strings for python<3.12 ?

That would be a bug in flake with python<3.12 no? :)

Ok, so I think what we're suggesting here is: 1) include python3.12 in cicd linting setup and 2) fix this linting issue as you mention above @khintz (which is ignored by flake for python<3.12). And then in future if linting in python3.12 with flake fails in cicd with expressions in fstrings then we know why :)

Sound ok @khintz and @sadamov?

[khintz]

That would be a bug in flake with python<3.12 no? :)

That would be my assumption until I am proved wrong 😄

Your suggestion sounds good to me.

[leifdenby]

thanks for your help @sadamov and @khintz, could you give this a last glance-over please and see if you're happy to accept the changes?

[leifdenby]

thank you both!

[joeloskarsson]

Pylint is indeed very slow. If ruff is working well and is fast I think it sounds great to switch to that.

Just for my understanding: @leifdenby do I understand correctly that this removes the use of pylint? Does that relate to not checking imports to external packages?