Adding pre-commit tool config plus cmake-format #3606

KaiSzuttor · 2020-03-30T11:59:27Z

Fixes #3591

Depends on espressomd/docker#158

Description of changes:

pulled out tool version checking to separate files
added configuration file for pre-commit
added cmake-format configuration and style fix

maintainer/format/autopep8.sh

requirements.txt

maintainer/format/autopep8.sh

codecov · 2020-03-30T13:50:00Z

Codecov Report

Merging #3606 into python will decrease coverage by <1%.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           python   #3606   +/-   ##
======================================
- Coverage      87%     87%   -1%     
======================================
  Files         525     525           
  Lines       23398   23371   -27     
======================================
- Hits        20583   20556   -27     
  Misses       2815    2815

Impacted Files	Coverage Δ
src/core/thermostat.hpp	`90% <0%> (-2%)`	⬇️
src/core/integrators/brownian_inline.hpp	`90% <0%> (-1%)`	⬇️
src/core/particle_data.cpp	`96% <0%> (-1%)`	⬇️
src/core/Particle.hpp	`96% <0%> (-1%)`	⬇️
src/core/constraints/ShapeBasedConstraint.cpp	`87% <0%> (-1%)`	⬇️
src/core/forces_inline.hpp	`85% <0%> (-1%)`	⬇️
src/core/pressure.cpp	`90% <0%> (-1%)`	⬇️
src/core/unit_tests/thermostats_test.cpp	`100% <0%> (ø)`	⬆️
src/core/constraints/HomogeneousMagneticField.cpp	`100% <0%> (ø)`	⬆️
src/core/integrators/langevin_inline.hpp	`100% <0%> (ø)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da6259b...14ac4ae. Read the comment docs.

doc/tutorials/01-lennard_jones/CMakeLists.txt

src/core/unit_tests/CMakeLists.txt

src/shapes/unit_tests/CMakeLists.txt

src/utils/tests/CMakeLists.txt

testsuite/python/CMakeLists.txt

doc/tutorials/04-lattice_boltzmann/CMakeLists.txt

doc/tutorials/05-raspberry_electrophoresis/CMakeLists.txt

fweik · 2020-03-31T08:40:07Z

Just my two cents: I'm not sure that adding pre-commit is worth it over just calling the shell scripts. It's just one more thing to know. @jngrad if you decide to keep it, make sure that @KaiSzuttor properly documents it in the CI documentation.

jngrad · 2020-03-31T08:54:17Z

@fweik once this is merged, we won't be able to call the shell scripts anymore, because the find commands that detect the file extensions were replaced by pre-commit. Everyone will have to install pre-commit to run the style scripts. I don't really mind, because I don't use fix_style.sh anyway; I wrote my own implementation of pre-commit almost two years ago (git-style) and will keep using it because it has more features.

fweik · 2020-03-31T10:22:13Z

@jngrad yes, the alternative is to keep the find expressions.

KaiSzuttor · 2020-03-31T10:28:46Z

Btw. you already have to pip install the autopep8 package, the pycodestyle package and the cmake-format package due to the pinned versions. You will most probably do that via pip install -r requirements.txt. If so, you will automagically not notice the change at all.

jngrad

ok from my side

KaiSzuttor · 2020-03-31T16:28:50Z

interesting

Traceback (most recent call last):
24   File "/home/espresso/.local/bin/pre-commit", line 7, in <module>
25     from pre_commit.main import main
26   File "/home/espresso/.local/lib/python3.5/site-packages/pre_commit/main.py", line 189
27     version=f'%(prog)s {C.VERSION}',
28                                   ^
29 SyntaxError: invalid syntax
30 Passed style check

jngrad · 2020-03-31T17:25:25Z

@KaiSzuttor We shouldn't delay #3421 any longer and start moving towards Python 3.6 every time our workflow breaks. espressomd/docker#160 moves the style check packages to the Ubuntu 20.04 image, it'll only be a matter of updating the yml file here to use Ubuntu 20.04 image instead of Ubuntu 16.04.

KaiSzuttor · 2020-03-31T20:27:53Z

but, do you understand where the error comes from?

jngrad · 2020-03-31T20:41:56Z

maintainer/CI/fix_style.sh

-find . \( -name '*.hpp' -o -name '*.cpp' -o -name '*.cu' -o -name '*.cuh' \) -not -path './libs/*' | xargs -r -n 5 -P 8 "${CLANGFORMAT}" -i -style=file || exit 3
-find . \( -name '*.py' -o -name '*.pyx' -o -name '*.pxd' \) -not -path './libs/*' | xargs -r -n 5 -P 8 "${AUTOPEP8}" --ignore=E266,W291,W293 --in-place --aggressive || exit 3
-find . -type f -executable ! -name '*.sh' ! -name '*.py' ! -name '*.sh.in' ! -name pypresso.cmakein -not -path './.git/*' | xargs -r -n 5 -P 8 chmod -x || exit 3
+pre-commit run --all-files


I think dash and bash return the error code of the last command, so if this line fails due to a Python3.6-specific f-string (Ubuntu 16.04 uses Python3.5), the shell script will keep going to the next instructions and return 0, unless we add the following:

Suggested change

pre-commit run --all-files

pre-commit run --all-files || exit 1

Actually no, don't do that! pre-commit returns 1 when it touched a file.

Is there an older version of pre-commit compatible with Python 3.5 that we can use?

KaiSzuttor · 2020-04-01T07:30:31Z

I just realised that the issue is also that the packaged pip silently installs packages that may not be compatible with the python version. If you upgrade pip and try to install pre-commit in the container, you actually get:

ERROR: Package 'pre-commit' requires a different Python: 3.5.2 not in '>=3.6.1'

Installing an older version would not help because a dependency of pre-commit needs 3.6.

mkuron · 2020-04-01T08:39:34Z

Is there an older version of pre-commit compatible with Python 3.5 that we can use?

Installing an older version would not help because a dependency of pre-commit needs 3.6.

You could explicitly install specific (python 3.5-compatible) versions of the dependencies to circumvent that problem. Of course, that means adding all dependencies to the Dockerfile. pip is a terrible package manager.

RudolfWeeber · 2020-04-01T09:31:17Z

I wonder whether this PR takes too much work from too many people. Personally, I just run JN’s script before committing and that’s that. Do we really want to follow through with this? Or is my impression wrong, that this hasn’t converged to the point of just needing a review?

KaiSzuttor · 2020-04-01T09:34:09Z

Or is my impression wrong, that this hasn’t converged to the point of just needing a review?

my impression is that sometimes we comment on PRs without having looked at the changes

jngrad · 2020-04-01T10:33:25Z

I wonder whether this PR takes too much work from too many people.

There wouldn't be so much friction if ICP machines had Clang-9 installed.

Do we really want to follow through with this?

The pre-commit tool removes some complexity from the shell scripts by taking care of the path detection. It's unclear to me how many people will use the commit hook feature. I most likely won't, because I rely a lot on rebase operations and don't want them to be interrupted, nor do I want to work on two clones, one with the hook and one without.

It's also unclear to me why pre-commit works so fast. On my previous ICP machine that had twice as less RAM the fix_style.sh script would use several GB of RAM and freeze the OS for several minutes (Rudolf and Christoph also experience performance issues). I'd prefer to have pre-commit in ESPResSo rather than my tool in ESPResSo, because my tool is untested and quite complex.

KaiSzuttor · 2020-04-02T10:28:42Z

the issues came solely from the default pip version, I fixed it in espressomd/docker#162

KaiSzuttor · 2020-04-02T13:21:38Z

closed for #3622

Contains #3619 #3606 Description of changes: - pulled out tool version checking to separate files - added configuration file for `pre-commit` - added `cmake-format` configuration and style fix - use new docker images created using GitHub Actions

maintainer: added new style checking via pre-commit tool.

a945f8b

KaiSzuttor force-pushed the pre_commit branch 3 times, most recently from b9e7fd8 to efd0c94 Compare March 30, 2020 13:01

jngrad reviewed Mar 30, 2020

View reviewed changes

maintainer/format/autopep8.sh Show resolved Hide resolved

requirements.txt Show resolved Hide resolved

KaiSzuttor force-pushed the pre_commit branch 2 times, most recently from 9d1793a to e209397 Compare March 30, 2020 13:23

jngrad reviewed Mar 30, 2020

View reviewed changes

maintainer/format/autopep8.sh Outdated Show resolved Hide resolved

jngrad requested changes Mar 30, 2020

View reviewed changes

KaiSzuttor force-pushed the pre_commit branch 2 times, most recently from 840a561 to 99da7c9 Compare March 30, 2020 18:35

jngrad requested changes Mar 30, 2020

View reviewed changes

doc/tutorials/04-lattice_boltzmann/CMakeLists.txt Show resolved Hide resolved

doc/tutorials/05-raspberry_electrophoresis/CMakeLists.txt Show resolved Hide resolved

KaiSzuttor added 2 commits March 31, 2020 09:25

maintainer: added cmake-format check.

52113c0

cmake-format.

14ac4ae

KaiSzuttor force-pushed the pre_commit branch from 99da7c9 to 14ac4ae Compare March 31, 2020 07:26

KaiSzuttor marked this pull request as ready for review March 31, 2020 07:31

fweik assigned jngrad Mar 31, 2020

jngrad approved these changes Mar 31, 2020

View reviewed changes

KaiSzuttor requested a review from RudolfWeeber March 31, 2020 20:25

jngrad reviewed Mar 31, 2020

View reviewed changes

KaiSzuttor mentioned this pull request Apr 2, 2020

Pre commit plus new images #3622

Merged

KaiSzuttor closed this Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding pre-commit tool config plus cmake-format #3606

Adding pre-commit tool config plus cmake-format #3606

KaiSzuttor commented Mar 30, 2020

codecov bot commented Mar 30, 2020 •

edited

Loading

fweik commented Mar 31, 2020

jngrad commented Mar 31, 2020

fweik commented Mar 31, 2020

KaiSzuttor commented Mar 31, 2020

jngrad left a comment

KaiSzuttor commented Mar 31, 2020

jngrad commented Mar 31, 2020

KaiSzuttor commented Mar 31, 2020

jngrad Mar 31, 2020

jngrad Mar 31, 2020

jngrad Mar 31, 2020

KaiSzuttor commented Apr 1, 2020

mkuron commented Apr 1, 2020 •

edited

Loading

RudolfWeeber commented Apr 1, 2020 via email

KaiSzuttor commented Apr 1, 2020

jngrad commented Apr 1, 2020

KaiSzuttor commented Apr 2, 2020

KaiSzuttor commented Apr 2, 2020

	pre-commit run --all-files
	pre-commit run --all-files \|\| exit 1

Adding pre-commit tool config plus cmake-format #3606

Adding pre-commit tool config plus cmake-format #3606

Conversation

KaiSzuttor commented Mar 30, 2020

codecov bot commented Mar 30, 2020 • edited Loading

Codecov Report

fweik commented Mar 31, 2020

jngrad commented Mar 31, 2020

fweik commented Mar 31, 2020

KaiSzuttor commented Mar 31, 2020

jngrad left a comment

Choose a reason for hiding this comment

KaiSzuttor commented Mar 31, 2020

jngrad commented Mar 31, 2020

KaiSzuttor commented Mar 31, 2020

jngrad Mar 31, 2020

Choose a reason for hiding this comment

jngrad Mar 31, 2020

Choose a reason for hiding this comment

jngrad Mar 31, 2020

Choose a reason for hiding this comment

KaiSzuttor commented Apr 1, 2020

mkuron commented Apr 1, 2020 • edited Loading

RudolfWeeber commented Apr 1, 2020 via email

KaiSzuttor commented Apr 1, 2020

jngrad commented Apr 1, 2020

KaiSzuttor commented Apr 2, 2020

KaiSzuttor commented Apr 2, 2020

codecov bot commented Mar 30, 2020 •

edited

Loading

mkuron commented Apr 1, 2020 •

edited

Loading