Pre-commit setup for the repo

[maichmueller]

This PR addresses what has been mentioned in #1070.

What this PR includes:

• pre-commit configuration for formatters:
  • clang-format:
    c++ project files formatting
  • pyink:
    google's python formatter that is close to black, but allows 2-indent formatting (oh the humanity...)
  • cmake-format:
    formatting style for cmake files
  • isort:
    sort python imports alphabetically (can use profile=black, so I hope this profile works with pyink too)
  • black:
    the python formatter (commented out but added for when google policy on 2-indent changes)
• pylint:
  using google's official configuration. Since this config enforces 2-indent, pyink is needed.
• common pre-commit checks:
  • id: check-added-large-files
  • id: check-case-conflict
  • id: check-docstring-first (currently uncommented as some files would need manual changing)
  • id: check-merge-conflict
  • id: check-symlinks
  • id: check-toml
  • id: check-yaml
  • id: debug-statements
  • id: end-of-file-fixer
  • id: mixed-line-ending
  • id: requirements-txt-fixer
  • id: trailing-whitespace
• common python checks:
  • python-check-blanket-noqa
  • id: python-check-blanket-type-ignore
  • id: python-no-log-warn (uncommented because clashes in multiple places where logging.warn has been used)
  • id: python-no-eval
  • id: python-use-type-annotations

As a side effect of the pre-commit checks I have moved the setup definition into a pyproject.toml, since it allows to also write the configurations for some of these tools. In consequence:

• pyproject.toml now holds the metadata of the project (version, authors, license, etc.)
• setup.py is only the extension builder

The dev-tools needed to run these pre-commits have also been added to the the pyproject.toml as optional dependencies of the dev group and could then be installed via:

pip install 'open_spiel[dev]'

or locally

pip install '.[dev]'

If this PR is accepted a remark in the docs should be made for developers who wish to contribute the upstream to run pre-commit install on their repo in order to activate the hooks.

[lanctot]

Thanks @maichmueller !

Just a heads up: it will take a bit of time before we get to this as it involves learning a few things and discussion with the team.

Also we have a release coming up that I would like to get out first (hopefully next week).

[lanctot]

We might import this (I think either way we will have to move to using pyproject.toml eventually) so I will look further into it eventually.

Could you pull from master and push the merge commit to resolve the branch conflicts?

[alexunderch]

@lanctot as it might be related to #1363, what's status of this. Continue this work or start all over?

[lanctot]

It's been a while. Happy to reconsider this if it adds minimal extra maintenance overhead and I understand how it works (don't under-estimate how hard this is, I have very little time to spend on this)

I think we should start small-- we don't want to make it too hard to contribute

• move to pyproject.toml
• add pre-commit hooks that run the Google style check (using clang-format) for C++ code and pylint with Google style file for python code

We can start there. I don't remember how much more beyond that was done in this PR. @maichmueller can you sync to master, or resubmit?

[alexunderch]

I want to start with a simple thing if @maichmueller agrees. Add pre-commit hooks and linter for python and c++.

Because I am not still sure about dependencies, it may require some further discussion, I think.

P.s. sorry github mobile sometimes acts wild.

[alexunderch]

@maichmueller also, note to update the pyproject.toml with updated versions, open_spiel currently supports python>=3.11, some pre-commit packages seem old

[maichmueller]

Hi,

@alexunderch I have rebased the PR onto master again, so the key details should be up to date (hook configuration, openspiel version, and python requirements).

@lanctot: this PR moves the packaging metadata to pyproject.toml, while keeping setup.py only for the build. I do not anticipate any negative impact for future contributors from this change.

Regarding the hooks: the primary motivation for adding pre-commit is that (at least a few years ago) many PRs required a manual follow-up to ask contributors to run formatting and linting and address failures. A pre-commit setup helps avoid that recurring step. In my experience, auto-fixing formatters and basic linters are near pain-free (a commit fails, pre-commit applies fixes, you re-stage, and commit again). Hooks that enforce very specific stylistic preferences or forbid certain language features can be more disruptive.

If certain hooks are not desired, I can remove or adjust them easily. In my view, a reasonable next step (if hooks are to be added) to ensure the hooks remain effective would be:
1. run pre-commit run -a on the full repository to align the existing code (ideally only auto-formatting triggered).
2. add a CI check to ensure future changes stay compliant.

[alexunderch]

@maichmueller I think adding a formatting/linting action from ruff, for example, might have eased the pain of linting a lot: https://github.com/astral-sh/ruff-action, but it needs to be compatible with google standards. If you know how to do it, welcome. Or can assign it to me.

Also, what do you think about replacing your clang-format with something more general and google complacent likes of https://github.com/kehanXue/google-style-clang-format/blob/master/.clang-format

we need @lanctot 's opinion to be the driver, I guess

[maichmueller]

@alexunderch AFAIK there is no official google ruff definition. We can replace pyink, isort, pygrep-hooks with ruff combined with a line-length of 80 to best approximate pyink, but there would be some deltas left I think. From what I understand, deepmind has an internal repo of open_spiel. If that uses pyink, then I imagine this to be a deal-breaker, their diffs would contain a fair bit of noise. We could also keep pyink and only use ruff for linting. I think those grep-hooks are almost all contained in ruff.

Sure, if that .clang-format file is prefered I will add it in. Waiting for @lanctot on this.

[lanctot]

Hi guys,

Thanks for the work here!

Regarding the hooks: the primary motivation for adding pre-commit is that (at least a few years ago) many PRs required a manual follow-up to ask contributors to run formatting and linting and address failures.

This is 100% still true, and it's now gotten to the point that it's too hard to continue without it, so we definitely have to import something that helps make the contibutions less heavy on our side.

This solution looks great to me. @alexunderch I'm quite reluctant to add any more dependencies because every one that gets introduced adds a maintenance burden down the road. If we can achieve what we need to without adding more dependencies, let's do that.

[alexunderch]

nah, if I don't like introducing external deps either, all good
google flax uses ruff's pre-commit hook: https://github.com/google/flax/blob/2ed95f31084f7da61d095a757887761445f037e8/.pre-commit-config.yaml#L38

but maybe it's for the external repo, so let's stay with pyink and its friends <3
thank you @maichmueller

[lanctot]

If certain hooks are not desired, I can remove or adjust them easily. In my view, a reasonable next step (if hooks are to be added) to ensure the hooks remain effective would be: 1. run pre-commit run -a on the full repository to align the existing code (ideally only auto-formatting triggered). 2. add a CI check to ensure future changes stay compliant.

This will require some time on my part because we need to fiddle with the hooks until the result of (1) is either (i) no changes, or (ii) the changes don't trigger more internal lint errors internally. So please bear with me, there will be some delays. We can't proceed until one of those is true, which will likely be (ii), and needs to be done by trial and error on my end.

Edit: there is probably someone else in a similatr situation at Google, I will ask them. Maybe this won't be as difficult as implied above.

[alexunderch]

@lanctot we can trigger it with a default build action, just with one line or so https://github.com/google/flax/blob/2ed95f31084f7da61d095a757887761445f037e8/.github/workflows/flax_test.yml#L87

or with a pre-commit action #1071 (comment)

[maichmueller]

@lanctot, some of the checks you remarked on in the comment of PR #1074 are clang-tidy checks AFAIK, i.e. the current checks cannot cover them. Adding clang-tidy to pre-commit hooks is a lot more friction, since users need to have a compile_commands.json available somewhere (easy with CLion, somewhat more work with VSCode, others dont know) that the executable can use to evaluate the code. I would expect that knowledge of how to set this up isnt quite commonly available.
To avoid this, clang-tidy can be set up in CI to highlight code issues of this kind making the fixing more of a manual user-request again. Otherwise, you can accept auto-commits from a CI bot that applies clang-tidy auto-fixes whenever possible; false-positives are annoying here too though.

[lanctot]

Ok, thanks. I will ask around internally at Google. I strongly suspect we already have this solved in a way that makes it easy for us to maintain and users to contribute. If I'm wrong we can hopefully get the closest thing.