Skip to content
Snippets Groups Projects
Commit 9ab7db36 authored by Robert Rosca's avatar Robert Rosca
Browse files

Merge branch 'feat/pre-commit-checks' into 'master' 

Feat/pre commit checks

- Added pre-commit hooks which:
  - isort on `.py` and `.ipynb`
  - flake8 on `.py` and `.ipynb`
  - nbstripout
  - rstcheck
  - check-added-large-files
  - check-ast
  - check-json
  - check-yaml
  - check-toml
  - end-of-file-fixer
  - trailing-whitespace
  - check-docstring-first
  - check-merge-conflict
  - mixed-line-ending
- Hooks run as part of the `check` stage in the CI
- Fixed some of the issues:
  - EoF
  - Trailing whitespace
  - isort (but only for .py)

I included a lot of other changes and fixes in the original MR (https://git.xfel.eu/gitlab/detectors/pycalibration/merge_requests/411) before figuring out how to get the checks to run only on changed files, which meant that the MR implementing pre-commit checks also refactored a decent chunk of code. It would be better to do that refactoring in separate MRs so I've closed the original one and opened this to replace it.

@ahmedk @danilevc @hammerd feel free to review when you have time :smile:

TODO:

- [x] Add pre-commit to requirements.txt
- [x] ~~Don't mutate the code during pre-commit checks~~ - changed my mind about this a few times, left it to mutate the code as that is only done on staged files, so any mutations can be reverted by undoing changes anyway. Added an explanation on this to the readme instead.
- [x] Add info to the readme and to the wiki
- [x] Add explanation of what each of the checks is for in more detail
- [x] Add scripts to execute w/ diff-only (same as the CI pipeline)

See merge request detectors/pycalibration!417
parents 233b8f7e e49dcbcf
No related branches found
No related tags found
1 merge request!417Feat/pre commit checks
Hide whitespace changes
Inline Side-by-side
.git-blame-ignore-revs 0 → 100644
+ 7
− 0
View file @ 9ab7db36
# This file contains info on which commits to ignore for git blame to work
# correctly, you can use either of these see the 'correct' blame results:
#
# - `git blame file.py --ignore-revs-file .git-blame-ignore-revs`
# - `git config blame.ignoreRevsFile .git-blame-ignore-revs`
#
# Second option is a bit better as it'll work on the whole repo all the time
.gitlab-ci.yml
+ 20
− 4
View file @ 9ab7db36
isort:
stage: test
stages:
- check
- test
checks:
stage: check
only: [merge_requests]
allow_failure: true
script:
- python3 -m pip install --user isort==5.6.4
- isort --diff **/*.py && isort -c **/*.py
- export PATH=/home/gitlab-runner/.local/bin:$PATH
# We'd like to run the pre-commit hooks only on files that are being
# modified by this merge request, however
# `CI_MERGE_REQUEST_TARGET_BRANCH_SHA` is a 'premium' feature according to
# GitLab... so this is a workaround for extracting the hash
- export CI_MERGE_REQUEST_TARGET_BRANCH_SHA=$(git ls-remote origin $CI_MERGE_REQUEST_TARGET_BRANCH_NAME | cut -d$'\t' -f1)
- export FILES=$(git diff $CI_COMMIT_SHA $CI_MERGE_REQUEST_TARGET_BRANCH_SHA --name-only | tr '\n' ' ')
- python3 -m pip install --user -r requirements.txt
- echo "Running pre-commit on diff from $CI_COMMIT_SHA to $CI_MERGE_REQUEST_TARGET_BRANCH_SHA ($CI_MERGE_REQUEST_TARGET_BRANCH_NAME)"
# Pass list of modified files to pre-commit so that it only checks them
- echo $FILES | xargs pre-commit run --color=always --files
pytest:
stage: test
only: [merge_requests]
script:
- python3 -m pip install --user -r requirements.txt
- python3 -m pip install --user pytest
......
.pre-commit-config.yaml 0 → 100644
+ 48
− 0
View file @ 9ab7db36
repos:
- repo: meta
hooks:
- id: identity
- repo: https://github.com/nbQA-dev/nbQA
rev: 0.3.6
hooks:
- id: nbqa-isort
additional_dependencies: [isort==5.6.4]
args: [--nbqa-mutate]
- id: nbqa-flake8
additional_dependencies: [flake8==3.8.4]
args: [--nbqa-mutate]
- repo: https://github.com/kynan/nbstripout
rev: 0.3.9
hooks:
- id: nbstripout
- repo: https://github.com/pycqa/isort
rev: 5.6.4
hooks:
- id: isort
- repo: https://gitlab.com/pycqa/flake8
rev: 3.8.4
hooks:
- id: flake8
# If `CI_MERGE_REQUEST_TARGET_BRANCH_SHA` env var is set then this will
# run flake8 on the diff from the current commit to the latest commit of
# the branch being merged into, otherwise it will run flake8 as it would
# usually execute via the pre-commit hook
entry: bash -c 'if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then (flake8 "$@"); else (git diff $CI_MERGE_REQUEST_TARGET_BRANCH_SHA | flake8 --diff); fi' --
- repo: https://github.com/myint/rstcheck
rev: 3f92957478422df87bd730abde66f089cc1ee19b # commit where pre-commit support was added
hooks:
- id: rstcheck
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v2.3.0
hooks:
- id: check-added-large-files
- id: check-ast
- id: check-json
- id: check-yaml
- id: check-toml
- id: end-of-file-fixer
- id: trailing-whitespace
- id: check-docstring-first
- id: check-merge-conflict
- id: mixed-line-ending
args: [--fix=lf]
README.rst
+ 173
− 85
View file @ 9ab7db36
###################
Offline Calibration
===================
###################
The offline calibration is a package that consists of different services,
responsible for applying most of the offline calibration and characterization
for the detectors.
Offline calibration installation
================================
.. contents::
It's recommended to install the offline calibration (pycalibration) package
over maxwell, using anaconda/3 environment.
Installation using Anaconda
---------------------------
Offline Calibration Installation
********************************
First you need to load the anaconda/3 environment through::
It's recommended to install the offline calibration (pycalibration) package over
maxwell, using anaconda/3 environment.
1. module load anaconda/3
If installing into other python enviroments, this step can be skipped.
Installation using python virtual environment - recommended
===========================================================
Then the package for the offline calibration can be obtained from the git repository::
1. ``git clone ssh://git@git.xfel.eu:10022/detectors/pycalibration.git && cd pycalibration`` - clone the offline calibration package from EuXFEL GitLab
2. ``module load anaconda/3`` - load the anaconda/3 environment
3. ``python3 -m venv .venv`` - create the virtual environment
4. ``source .venv/bin/activate`` - activate the virtual environment
5. ``python3 -m pip install --upgrade pip`` - upgrade version of pip
6. ``python3 -m pip install -r requirements.txt`` - install dependencies
7. ``python3 -m pip install .`` - install the pycalibration package (add ``-e`` flag for editable development installation)
8. ``pip install "git+ssh://git@git.xfel.eu:10022/karaboDevices/pyDetLib.git#egg=XFELDetectorAnalysis&subdirectory=lib"``
2. git clone https://git.xfel.eu/gitlab/detectors/pycalibration.git
Copy/paste script:
.. code::
You can then install all requirements of this tool chain in your home directory by running::
git clone ssh://git@git.xfel.eu:10022/detectors/pycalibration.git
cd pycalibration
module load anaconda/3
python3 -m venv .venv
source .venv/bin/activate
python3 -m pip install --upgrade pip
python3 -m pip install -r requirements.txt
python3 -m pip install . # `-e` flag for editable install
python3 -m pip install "git+ssh://git@git.xfel.eu:10022/karaboDevices/pyDetLib.git#egg=XFELDetectorAnalysis&subdirectory=lib/"
3. pip install -r requirements.txt . --user
in pycalibration's root directory.
Installation into user home directory
=====================================
After installation, you should make sure that the home directory is in the PATH environment variable::
1. ``git clone ssh://git@git.xfel.eu:10022/detectors/pycalibration.git && cd pycalibration`` - clone the offline calibration package from EuXFEL GitLab
2. ``module load anaconda/3`` - load the anaconda/3 environment. If installing into other python environments, this step can be skipped
3. ``pip install -r requirements.txt`` - install all requirements of this tool chain in your home directory
4. ``pip install .`` - install the pycalibration package (add ``-e`` flag for editable development installation)
5. ``export PATH=$HOME/.local/bin:$PATH`` - make sure that the home directory is in the PATH environment variable
4. export PATH=$HOME/.local/bin:$PATH
Copy/paste script:
Installation using virtual python environment
---------------------------------------------
.. code::
Create virtual environment::
git clone ssh://git@git.xfel.eu:10022/detectors/pycalibration.git
cd pycalibration
module load anaconda/3
pip install -r requirements.txt --user
pip install . # `-e` flag for editable install, e.g. `pip install -e .`
export PATH=$HOME/.local/bin:$PATH
module load anaconda/3
python -m venv /path/to/new/virtual/environment
source /path/to/new/virtual/environment/bin/activate
Clone from git::
Creating an ipython kernel for virtual environments
===================================================
cd /path/to/packages
git clone https://git.xfel.eu/gitlab/detectors/pycalibration.git
cd pycalibration
To create an ipython kernel with pycalibration available you should (if using a
venv) activate the virtual environment first, and then run:
Install the package::
.. code::
pip install -r requirements.txt
python3 -m pip install ipykernel # If not using a venv add `--user` flag
python3 -m ipykernel install --user --name pycalibration --display-name "pycalibration" # If not using a venv pick different name
In additional install pyDetLib package, which is required for many notebooks::
This can be useful for Jupyter notebook tools as https://max-jhub.desy.de/hub/login
cd /path/to/packages
git clone https://git.xfel.eu/gitlab/karaboDevices/pyDetLib.git
cd pyDetLib/lib
pip install -r requirements.txt
pip install .
++++++++++++++++++++++++++++++++++++++++++++++++++
Setting an ipython kernel for virtual environments
++++++++++++++++++++++++++++++++++++++++++++++++++
Contributing
************
To set a kernel for your virtual environment::
Guidelines
==========
source /path/to/new/virtual/environment/bin/activate
pip install ipykernel
python -m ipykernel install --user --name <virtenv-name> --display-name "virtenv-display-name"
Development guidelines can be found on the GitLab Wiki page here: https://git.xfel.eu/gitlab/detectors/pycalibration/wikis/GitLab-Guidelines
This can be useful for Jupyter notebook tools as "max-jhub.desy.de".
Basics
======
Development Installation
The installation instructions above assume that you have set up SSH keys for use
with GitLab to allow for passwordless clones from GitLab, this way it's possible
to run ``pip install git+ssh...`` commands and install packages directly from
GitLab.
To do this check the settings page here: https://git.xfel.eu/gitlab/profile/keys
Pre-Commit Hooks
================
This repository uses pre-commit hooks automatically run some code quality and
standard checks, this includes the following:
a. ``identity`` - The 'identity' meta hook prints off a list of files that the hooks will execute on
b. 'Standard' file checks
1. ``check-added-large-files`` - Ensures no large files are committed to repo
2. ``check-ast`` - Checks that the python AST is parseable
3. ``check-json`` - Checks json file formatting is parseable
4. ``check-yaml`` - Checks yaml file formatting is parseable
5. ``check-toml`` - Checks toml file formatting is parseable
6. ``rstcheck`` - Checks rst file formatting is parseable
7. ``end-of-file-fixer`` - Fixes EoF to be consistent
8. ``trailing-whitespace`` - Removes trailing whitespaces from lines
9. ``check-merge-conflict`` - Checks no merge conflicts remain in the commit
10. ``mixed-line-ending`` - Fixes mixed line endings
c. Code checks
1. ``flake8`` - Code style checks
2. ``isort`` - Sorts imports in python files
3. ``check-docstring-first`` - Ensures docstrings are in the correct place
d. Notebook checks
1. ``nbqa-flake8`` - Runs flake8 on notebook cells
2. ``nbqa-isort`` - Runs isort on notebook cells
3. ``nbstripoutput`` - Strips output from ipynb files
To install these checks, set up you environment as mentioned above and then run
the command:
.. code::
pre-commit install-hooks
This will set up the hooks in git locally, so that each time you run the command
``git commit`` the hooks get executed on the **staged files only**, beware that
if the pre-commit hooks find required changes some of them will **modify your
files**, however they only modify the current working files, not the ones you
have already staged. This means that you can look at the diff between your
staged files and the ones that were modified to see what changes are suggested.
Run Checks Only On Diffs
------------------------
For a development installation, which automatically
picks up (most) changes, first install the dependencies as above,
but then install the tool-chain separately in development mode (install in home directory using --user, in case of using Anaconda/3)::
Typically ``pre-commit`` is ran on ``--all-files`` within a CI, however as this
is being set up on an existing codebase these checks will always fail with a
substantial number of issues. Using some creative workarounds, the CI has been
set up to only run on files which have changed between a PR and the target
branch.
If you want to run the pre-commit checks as they would run on the CI, then you
can use the ``bin/pre-commit-diff.sh`` to execute the checks as on the CI
pipeline.
A side effect of this is that the checks will run on **all** of the differences
between the 'local' and target branch. This means that if changes have recently
been merged into the target branch, and there is divergence between the two,
then the tests will run on all the differences.
pip install -e .
If this happens and the hooks in the CI (or via the script) run on the wrong
files then you should **rebase onto the target branch** to prevent the checks
from running on the wrong files/diffs.
Activate Offline calibration
============================
Skipping Checks
---------------
For using pycalibration package one needs to activate it through::
If the checks are failing and you want to ignore them on purpose then you have two options:
source activate
- use the ``--no-verify`` flag on your ``git commit`` command to skip them, e.g. ``git commit -m "Commit skipping hooks" --no-verify``
- use the variable ``SKIP=hooks,to,skip`` before the git commit command to list hooks to skip, e.g. ``SKIP=flake8,isort git commit -m "Commit skipping only flake8 and isort hooks"``
from inside of the pycalibration directory. This will automatically load
all needed modules and export the $PATH for the home directory.
In the CI pipeline the pre-commit check stage has ``allow_failure: true`` set so
that it is possible to ignore errors in the checks, and so that subsequent
stages will still run even if the checks have failed. However there should be a
good reason for allowing the checks to fail, e.g. checks failing due to
unmodified sections of code being looked at.
Python Scripted Calibration
===========================
***************************
First: do not run this on the Maxwell gateway. Rather, `salloc`
a node for yourself first::
**Do not run this on the Maxwell gateway**. Rather, ``salloc`` a node for
yourself first:
salloc -p exfel/upex -t 01:00:00
.. code::
where `-p` gives the partition to use: exfel or upex and `-t`
the duration the node should be allocated. Then `ssh` onto
that node.
salloc -p exfel/upex -t 01:00:00
(optionally) Set up the environment::
where `-p` gives the partition to use: exfel or upex and `-t` the duration the
node should be allocated. Then `ssh` onto that node.
module load python3
pip install --user ipython --upgrade
pip install --user ipyparallel --upgrade
pip install --user dill
If running headless (i.e. without X forwarding), be sure to set
`MPLBACKEND=Agg`, via::
Then activate your environment as described above (or just continue if you are
not using a venv).
export MPLBACKEND=Agg
If running headless (i.e. without X forwarding), be sure to set
``MPLBACKEND=Agg``, via:
Then start an `ipcluster`. If you followed the steps above this can be done
via::
.. code::
~/.local/bin/ipcluster start --n=32
export MPLBACKEND=Agg
Then start an ``ipcluster``. If you followed the steps above this can be done
via:
Run the script::
.. code::
ipcluster start --n=32
Finally run the script:
.. code::
python3 calibrate.py --input /gpfs/exfel/exp/SPB/201701/p002012/raw/r0100 \
--output ../../test_out --mem-cells 30 --detector AGIPD --sequences 0,1
--output ../../test_out --mem-cells 30 --detector AGIPD --sequences 0,1
Here `--input` should point to a directory of `RAW` files for the detector you
are calibrating. They will be output into the folder specified by `--output`,
which will have the run number or the last folder in the hiearchy of the input
appended. Additionally, you need to specify the number of `--mem-cells` used
for the run, as well as the `--detector`. Finally, you can optionally
specify to only process certain `--sequences` of files, matching the sequence
numbers of the `RAW` input. These should be given as a comma-separated list.
Finally, there is a `--no-relgain` option, which disables relative gain
are calibrating. They will be output into the folder specified by `--output`,
which will have the run number or the last folder in the hierarchy of the input
appended. Additionally, you need to specify the number of `--mem-cells` used for
the run, as well as the `--detector`. Finally, you can optionally specify to
only process certain `--sequences` of files, matching the sequence numbers of
the `RAW` input. These should be given as a comma-separated list.
Finally, there is a `--no-relgain` option, which disables relative gain
correction. This can be useful while we still further characterize the detectors
to provid accurate relative gain correction constants.
to provide accurate relative gain correction constants.
You'll get a series of plots in the output directory as well.
bin/pre-commit-diff.sh 0 → 100755
+ 12
− 0
View file @ 9ab7db36
#!/bin/bash
# We'd like to run the pre-commit hooks only on files that are being modified by
# this merge request, however `CI_MERGE_REQUEST_TARGET_BRANCH_SHA` is a'premium'
# feature according to GitLab... so this is a workaround for extracting the hash
CI_MERGE_REQUEST_TARGET_BRANCH_NAME="${1:-master}" # Set to master or 1st input
CI_COMMIT_SHA=$(git rev-parse HEAD)
export CI_MERGE_REQUEST_TARGET_BRANCH_SHA=$(git ls-remote origin $CI_MERGE_REQUEST_TARGET_BRANCH_NAME | cut -d$'\t' -f1)
FILES=$(git diff $CI_COMMIT_SHA $CI_MERGE_REQUEST_TARGET_BRANCH_SHA --name-only | tr '\n' ' ')
echo "Running pre-commit on diff from $CI_COMMIT_SHA to $CI_MERGE_REQUEST_TARGET_BRANCH_SHA ($CI_MERGE_REQUEST_TARGET_BRANCH_NAME)"
# Pass list of modified files to pre-commit so that it only checks them
echo $FILES | xargs pre-commit run --color=always --files
requirements.txt
+ 6
− 5
View file @ 9ab7db36
git+file:///gpfs/exfel/sw/calsoft/git/cal_db_interactive@2.0.1
git+file:///gpfs/exfel/sw/calsoft/git/nbparameterise@0.3
git+file:///gpfs/exfel/sw/calsoft/git/pyDetLib@2.5.6-2.10.0#subdirectory=lib
Cython == 0.29.21
Jinja2 == 2.11.2
astcheck == 0.2.5
astsearch == 0.1.3
Cython == 0.29.21
dill == 0.3.0
extra_data == 1.2.0
extra_geom == 1.1.1
......@@ -11,22 +12,22 @@ fabio == 0.9.0
gitpython == 3.1.0
h5py == 2.10.0
iminuit == 1.3.8
ipyparallel == 6.2.4
ipykernel == 5.1.4
ipyparallel == 6.2.4
ipython == 7.12.0
ipython_genutils == 0.2.0
Jinja2 == 2.11.2
jupyter-core == 4.6.1
jupyter_client == 6.1.7
jupyter_console == 6.1.0
jupyter-core == 4.6.1
karabo_data == 0.7.0
lxml == 4.5.0
metadata_client == 3.0.8
nbclient == 0.5.1
nbconvert == 5.6.1
nbformat == 5.0.7
notebook == 6.1.5
notebook == 6.1.5
numpy == 1.19.1
pre-commit == 2.10.0
prettytable == 0.7.2
princess == 0.2
pypandoc == 1.4
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment

Legal Notice | Contacts