Add action to format code with black #2272

priyanshuone6 · 2022-08-29T16:48:24Z

Description

Add pre commit hook and ci to format code

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

New feature (non-breaking change which adds functionality)
Optimization (back-end change that speeds up the code)
Bug fix (non-breaking change which fixes an issue)

Key checklist:

No style issues: $ flake8
All tests pass: $ python run-tests.py --unit
The documentation builds: $ cd docs and then $ make clean; make html

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

Saransh-cpp · 2022-09-09T06:47:43Z

I think it might be better to use pre-commit instead of manually writing down the workflow. The pre-commit bot will then automatically commit on the branch/PR making the required changes.

A .pre-commit-config.yaml file like this should work -

ci:
  autoupdate_commit_msg: "chore: update pre-commit hooks"
  autofix_commit_msg: "style: pre-commit fixes"

repos:
  - repo: https://github.com/asottile/pyupgrade
    rev: v2.37.3
    hooks:
      - id: pyupgrade
        args: [--py37-plus]

  - repo: https://github.com/psf/black
    rev: 22.8.0
    hooks:
      - id: black-jupyter

  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.3.0
    hooks:
      - id: check-added-large-files
      - id: check-case-conflict
      - id: check-merge-conflict
      - id: check-symlinks
      - id: check-yaml
      - id: debug-statements
      - id: end-of-file-fixer
        exclude: ^docs
      - id: mixed-line-ending
      - id: requirements-txt-fixer
      - id: trailing-whitespace

  - repo: https://github.com/PyCQA/isort
    rev: 5.10.1
    hooks:
      - id: isort

  - repo: https://github.com/PyCQA/flake8
    rev: 5.0.4
    hooks:
      - id: flake8
        additional_dependencies:
          - flake8-bugbear
          - flake8-docstrings
          - flake8-print

  - repo: https://github.com/codespell-project/codespell
    rev: v2.2.1
    hooks:
      - id: codespell

  - repo: https://github.com/pre-commit/mirrors-prettier
    rev: "v3.0.0-alpha.0"
    hooks:
      - id: prettier
        types_or: [yaml, markdown, html, css, scss, javascript, json]
        exclude: assets/js/webapp\.js

  - repo: https://github.com/asottile/blacken-docs
    rev: v1.12.1
    hooks:
      - id: blacken-docs
        args: ["-E"]
        additional_dependencies: [black==22.8.0]

  - repo: https://github.com/pre-commit/pygrep-hooks
    rev: v1.9.0
    hooks:
      - id: python-check-blanket-noqa
      - id: python-check-blanket-type-ignore
        exclude: ^src/vector/backends/_numba_object.py$
      - id: python-no-log-warn
      - id: python-no-eval
      - id: python-use-type-annotations
      - id: rst-backticks
      - id: rst-directive-colons
      - id: rst-inline-touching-normal

Also, pre-commit ci integration is faster than other ci services -

Saransh-cpp · 2022-09-09T06:48:37Z

We can further filter out the pre-commit hooks as per our needs!

priyanshuone6 · 2022-09-09T07:30:15Z

Thanks, I was having trouble pushing black changes to the pull request or fork. Pre commit should make it so much more convenient to carry out these.

priyanshuone6 · 2022-09-09T11:00:54Z

I've put up a request to install the pre-commit ci bot on pybamm-team/PyBaMM to automatically commit on the PR, making the required changes.

Saransh-cpp

Thanks, @priyanshuone6!

Some comments -

We now won't be needing -

PyBaMM/.github/workflows/test_on_push.yml

Lines 27 to 41 in 8c4b3d6

    
             style: 
        
               needs: pre_job 
        
               if: ${{ needs.pre_job.outputs.should_skip != 'true' }} 
        
               runs-on: ubuntu-latest 
        
               steps: 
        
               - uses: actions/checkout@v3 
        
               - name: Setup python 
        
                 uses: actions/setup-python@v4 
        
                 with: 
        
                   python-version: 3.7 
        
               - name: Check style 
        
                 run: | 
        
                   python -m pip install tox 
        
                   tox -e flake8

I think there should be a note in CONTRIBUTING.md directing users how to use pre-commit. The Pre commit checks section can also be modified to use pre-commit instead of flake8 directly! An extra section can be also be added, saying something like this -

Installing and using pre-commit

PyBaMM uses a set of pre-commit hooks and the pre-commit bot to format and prettify the codebase. The hooks can be installed locally using -

pip install pre-commit
pre-commit install

This would run the checks every time a commit is created locally. The checks will only run on the files modified by that commit, but the checks can be triggered for all the files using -

pre-commit run --all-files

If you would like to skip the failing checks and push the code for further discussion, use the --no-verify option with git commit.

Some more useful pre-commits that can be added -

  - repo: https://github.com/asottile/pyupgrade
    rev: v2.37.3
    hooks:
      - id: pyupgrade
        args: [--py37-plus]

  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.3.0
    hooks:
      - id: check-added-large-files
      - id: check-case-conflict
      - id: check-merge-conflict
      - id: check-symlinks
      - id: check-yaml
      - id: debug-statements
      - id: end-of-file-fixer
        exclude: ^docs
      - id: mixed-line-ending
      - id: requirements-txt-fixer
      - id: trailing-whitespace

  - repo: https://github.com/PyCQA/isort
    rev: 5.10.1
    hooks:
      - id: isort

  - repo: https://github.com/pre-commit/mirrors-prettier
    rev: "v3.0.0-alpha.0"
    hooks:
      - id: prettier
        types_or: [yaml, markdown, html, css, scss, javascript, json]
        exclude: assets/js/webapp\.js

Lastly, I think pre-commit run --all-files should be run once on this PR. This would bring the codebase in sync with the pre-commit hooks!

codecov · 2022-09-09T11:26:25Z

Codecov Report

Base: 99.63% // Head: 99.63% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (200021e) compared to base (f5d16bc).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2272      +/-   ##
===========================================
- Coverage    99.63%   99.63%   -0.01%     
===========================================
  Files          362      361       -1     
  Lines        20035    19929     -106     
===========================================
- Hits         19962    19856     -106     
  Misses          73       73

Impacted Files	Coverage Δ
...bamm/expression_tree/operations/evaluate_python.py	`99.30% <ø> (ø)`
...Enertech_Ai2020/electrolyte_conductivity_Ai2020.py	`100.00% <ø> (ø)`
...adass2004/electrolyte_conductivity_Ramadass2004.py	`100.00% <ø> (ø)`
..._Valoen2005/electrolyte_conductivity_Valoen2005.py	`100.00% <ø> (ø)`
...i2020/graphite_entropy_Enertech_Ai2020_function.py	`100.00% <ø> (ø)`
...te_Ai2020/graphite_ocp_Enertech_Ai2020_function.py	`100.00% <ø> (ø)`
...s/graphite_Ai2020/graphite_volume_change_Ai2020.py	`100.00% <ø> (ø)`
...0_electrolyte_exchange_current_density_Chen2020.py	`100.00% <ø> (ø)`
...raphite_OKane2022/graphite_volume_change_Ai2020.py	`100.00% <ø> (ø)`
...graphite_Ramadass2004/graphite_ocp_Ramadass2004.py	`100.00% <ø> (ø)`
... and 55 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

valentinsulzer

Can you explain the order in which the workflows are run (and depend on each other)?
What happens if someone doesn't have pre-commit installed locally?
What happens to errors triggered by flake8 that can't be fixed automatically by black (e.g. undefined variable error)?

.github/workflows/test_on_push.yml

priyanshuone6 · 2022-09-13T08:13:34Z

Both pre-commit ci and test_on_push will run individually on each PR. The pre-commit CI will take care of formatting the code and will push the formatted code directly to the PR regardless of whether pre-commit is installed locally (it will skip pushing if the code is already formatted and the pre-commit CI tests pass). For some reason, if black can't fix any errors as you mentioned, the flake8 test in pre-commit CI, which is put at the last in the config, will fail, which will fail the whole pre-commit CI run.

Saransh-cpp · 2022-09-13T08:17:52Z

For some reason the pre-commit bot is not working. Should I give it the required access? The access can be provided from - https://results.pre-commit.ci/

Edit: The push is failing due to conflicts -

@priyanshuone6 could you rebase this branch or merge develop?

valentinsulzer · 2022-09-13T16:39:11Z

Both pre-commit ci and test_on_push will run individually on each PR. The pre-commit CI will take care of formatting the code and will push the formatted code directly to the PR regardless of whether pre-commit is installed locally (it will skip pushing if the code is already formatted and the pre-commit CI tests pass).

Sounds good

For some reason, if black can't fix any errors as you mentioned, the flake8 test in pre-commit CI, which is put at the last in the config, will fail, which will fail the whole pre-commit CI run.

Ok, let's make sure that the other tests don't get run unnecessarily if flake8 fails

priyanshuone6 · 2022-09-14T11:06:23Z

pre-commit.ci run

priyanshuone6 · 2022-09-14T11:47:33Z

pre-commit.ci run

Saransh-cpp

The bot is working now, nice! One final comment -

Saransh-cpp · 2022-09-18T18:23:11Z

.pre-commit-config.yaml

+  - repo: https://github.com/PyCQA/isort
+    rev: 5.10.1
+    hooks:
+      - id: isort


Suggested change

- id: isort

- id: isort

args: [".", "--skip", "pybamm/__init__.py"]

Turns out, the order of imports in pybamm's __init__.py file matters (not sure why). The changes made to pybamm/__init__.py should be reverted along with this change.

Order matters because the expression tree classes are defined in a hierarchy, so we have to define pybamm.Symbol before any others. Is there a better way to do this?

There is a way to ignore specific import sections of a file -

import a import b # isort: off import x import y # isort: on import f import g

(ignores x and y imports)

I tried this locally, but the order of imports is also required for battery models and geometry (maybe even for other imports). We can get rid of args here by adding # isort: skip_file to the top of __init__.py. Not sure what is the best thing to here.

Edit: # isort: split exists too, but I think that will still move the imports within a group, rather than moving the complete groups. Source - https://pycqa.github.io/isort/docs/configuration/action_comments.html.

Can we just skip isort altogether? Why is it needed?

I think isort was added through my suggestions, as it is a standard pre-commit hook 😅. Can be removed if not needed!

priyanshuone6 · 2022-09-19T14:36:48Z

Ok, let's make sure that the other tests don't get run unnecessarily if flake8 fails

I found no way to link a GitHub app with a workflow and run workflows only if the app's CI passes; something like needs in the workflow but for apps.

valentinsulzer · 2022-09-19T16:37:30Z

Will the pre-commit app always run before the workflows? i.e. does the test_on_push.yml workflow get run on the code after the pre-commit run has been done?

priyanshuone6 · 2022-09-21T01:36:38Z

They both run at the same time. Both will function separately and independently of one another. The pre-commit and PyBaMM's tests will be rerun for the new commit if the pre-commit ci pushes any additional modifications.

Saransh-cpp · 2022-09-21T16:11:24Z

We can do something like this in the workflows -

jobs:
  pre-commit:
    runs-on: ubuntu-latest
    name: Pre-commit
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-python@v4
        with:
          python-version: 3.x
      - uses: pre-commit/[email protected]

This would run pre-commit twice, once in the workflows (which cannot trigger the bot, but can be used as a dependency for the main tests) and once in the pre-commit CI (which can trigger the bot and automatically fix the PRs; fixing the PRs will trigger the workflow again, making sure the tests run).

Both of these pre-commit jobs will be running on different CI providers; hence, we won't be blocked as they will run parallely.

valentinsulzer · 2022-09-21T20:43:01Z

This would run pre-commit twice, once in the workflows (which cannot trigger the bot, but can be used as a dependency for the main tests) and once in the pre-commit CI (which can trigger the bot and automatically fix the PRs; fixing the PRs will trigger the workflow again, making sure the tests run).

Why not just run it once in the workflow in that case? Sorry if I am being stupid

priyanshuone6 · 2022-09-25T05:47:04Z

Running pre-commit in just the workflow will only pass or fail the style tests and not push the formatting changes to the PR. Should I go ahead with this?

valentinsulzer · 2022-09-25T13:47:10Z

Ok, go for it

valentinsulzer · 2022-09-27T14:53:21Z

Ready to merge once conflicts are resolved

valentinsulzer · 2022-09-27T15:18:24Z

Will also need approval from @Saransh-cpp

Saransh-cpp

Thanks!!

valentinsulzer · 2022-10-04T20:57:10Z

Pre-commit checks are failiing but the other actions still run: #2342

Can we check that pre-commit was successful before we run the other actions?

Saransh-cpp · 2022-10-05T09:03:56Z

We can use this - #2272 (comment). Running the pre-commit hooks in pre-commit.ci enables the bot to fix black related issues, but we cannot link pre-commit.ci with GitHub Actions.

valentinsulzer · 2022-10-05T16:54:32Z

Could we just put the flake8 style check back into the test_on_push action?

Add pre commit config

7557e8a

priyanshuone6 marked this pull request as draft August 29, 2022 17:22

priyanshuone6 force-pushed the black_action branch from d09e823 to 7557e8a Compare September 9, 2022 10:50

priyanshuone6 requested review from brosaplanella, Saransh-cpp and valentinsulzer September 9, 2022 10:57

priyanshuone6 marked this pull request as ready for review September 9, 2022 10:57

Saransh-cpp requested changes Sep 9, 2022

View reviewed changes

priyanshuone6 added 4 commits September 12, 2022 19:40

Update config

e47c639

Update contributing

8224fc9

remove the style check

390b654

Merge branch 'develop' into black_action

5ff3370

priyanshuone6 force-pushed the black_action branch from 939e69c to 5ff3370 Compare September 12, 2022 14:48

empty commit

0717339

valentinsulzer reviewed Sep 12, 2022

View reviewed changes

.github/workflows/test_on_push.yml Outdated Show resolved Hide resolved

Update workflow

05c5fab

Merge branch 'develop' into black_action

0412542

Merge branch 'develop' into black_action

1542815

priyanshuone6 force-pushed the black_action branch from eb456f7 to 051bd3a Compare September 14, 2022 11:43

Remove prettier

c95d238

priyanshuone6 force-pushed the black_action branch from 6ccc001 to b96b15f Compare September 18, 2022 09:47

Merge branch 'develop' into black_action

381aca7

priyanshuone6 force-pushed the black_action branch from b96b15f to 4272295 Compare September 18, 2022 09:53

Saransh-cpp requested changes Sep 18, 2022

View reviewed changes

pre-commit fixes

0054efe

priyanshuone6 force-pushed the black_action branch from b1fe7fd to 0054efe Compare September 19, 2022 13:53

style: pre-commit fixes

e5662a9

valentinsulzer approved these changes Sep 25, 2022

View reviewed changes

priyanshuone6 and others added 2 commits September 27, 2022 20:29

Merge branch 'develop' into black_action

e275bc5

style: pre-commit fixes

200021e

Saransh-cpp approved these changes Sep 27, 2022

View reviewed changes

valentinsulzer merged commit 0c617fc into pybamm-team:develop Sep 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add action to format code with black #2272

Add action to format code with black #2272

priyanshuone6 commented Aug 29, 2022 •

edited

Loading

Saransh-cpp commented Sep 9, 2022 •

edited

Loading

Saransh-cpp commented Sep 9, 2022

priyanshuone6 commented Sep 9, 2022

priyanshuone6 commented Sep 9, 2022

Saransh-cpp left a comment •

edited

Loading

codecov bot commented Sep 9, 2022 •

edited

Loading

valentinsulzer left a comment

priyanshuone6 commented Sep 13, 2022

Saransh-cpp commented Sep 13, 2022 •

edited

Loading

valentinsulzer commented Sep 13, 2022

priyanshuone6 commented Sep 14, 2022

priyanshuone6 commented Sep 14, 2022

Saransh-cpp left a comment

Saransh-cpp Sep 18, 2022 •

edited

Loading

valentinsulzer Sep 18, 2022

Saransh-cpp Sep 18, 2022 •

edited

Loading

valentinsulzer Sep 18, 2022

Saransh-cpp Sep 18, 2022

priyanshuone6 commented Sep 19, 2022

valentinsulzer commented Sep 19, 2022

priyanshuone6 commented Sep 21, 2022

Saransh-cpp commented Sep 21, 2022

valentinsulzer commented Sep 21, 2022

priyanshuone6 commented Sep 25, 2022

valentinsulzer commented Sep 25, 2022

valentinsulzer commented Sep 27, 2022

valentinsulzer commented Sep 27, 2022

Saransh-cpp left a comment

valentinsulzer commented Oct 4, 2022

Saransh-cpp commented Oct 5, 2022

valentinsulzer commented Oct 5, 2022

	style:
	needs: pre_job
	if: ${{ needs.pre_job.outputs.should_skip != 'true' }}
	runs-on: ubuntu-latest
	steps:
	- uses: actions/checkout@v3
	- name: Setup python
	uses: actions/setup-python@v4
	with:
	python-version: 3.7

	- name: Check style
	run: \|
	python -m pip install tox
	tox -e flake8

	- id: isort
	- id: isort
	args: [".", "--skip", "pybamm/__init__.py"]

Add action to format code with black #2272

Add action to format code with black #2272

Conversation

priyanshuone6 commented Aug 29, 2022 • edited Loading

Description

Type of change

Key checklist:

Further checks:

Saransh-cpp commented Sep 9, 2022 • edited Loading

Saransh-cpp commented Sep 9, 2022

priyanshuone6 commented Sep 9, 2022

priyanshuone6 commented Sep 9, 2022

Saransh-cpp left a comment • edited Loading

Choose a reason for hiding this comment

Installing and using pre-commit

codecov bot commented Sep 9, 2022 • edited Loading

Codecov Report

valentinsulzer left a comment

Choose a reason for hiding this comment

priyanshuone6 commented Sep 13, 2022

Saransh-cpp commented Sep 13, 2022 • edited Loading

valentinsulzer commented Sep 13, 2022

priyanshuone6 commented Sep 14, 2022

priyanshuone6 commented Sep 14, 2022

Saransh-cpp left a comment

Choose a reason for hiding this comment

Saransh-cpp Sep 18, 2022 • edited Loading

Choose a reason for hiding this comment

valentinsulzer Sep 18, 2022

Choose a reason for hiding this comment

Saransh-cpp Sep 18, 2022 • edited Loading

Choose a reason for hiding this comment

valentinsulzer Sep 18, 2022

Choose a reason for hiding this comment

Saransh-cpp Sep 18, 2022

Choose a reason for hiding this comment

priyanshuone6 commented Sep 19, 2022

valentinsulzer commented Sep 19, 2022

priyanshuone6 commented Sep 21, 2022

Saransh-cpp commented Sep 21, 2022

valentinsulzer commented Sep 21, 2022

priyanshuone6 commented Sep 25, 2022

valentinsulzer commented Sep 25, 2022

valentinsulzer commented Sep 27, 2022

valentinsulzer commented Sep 27, 2022

Saransh-cpp left a comment

Choose a reason for hiding this comment

valentinsulzer commented Oct 4, 2022

Saransh-cpp commented Oct 5, 2022

valentinsulzer commented Oct 5, 2022

priyanshuone6 commented Aug 29, 2022 •

edited

Loading

Saransh-cpp commented Sep 9, 2022 •

edited

Loading

Saransh-cpp left a comment •

edited

Loading

codecov bot commented Sep 9, 2022 •

edited

Loading

Saransh-cpp commented Sep 13, 2022 •

edited

Loading

Saransh-cpp Sep 18, 2022 •

edited

Loading

Saransh-cpp Sep 18, 2022 •

edited

Loading