Skip to content

Review process for contributions

Jump to bottom
Caspar van Leeuwen edited this page Mar 16, 2023 · 36 revisions

Contributions to EasyBuild, made via pull requests, are thoroughly reviewed and evaluated before they are merged in to the develop branch, ready for being part of the next release of EasyBuild.

This process is important to ensure stability of the develop branches, and high quality and timely releases.

Checklist EasyConfig reviewing

  1. Visual review of EasyConfig
    • Does it look ok in general?
    • Is the toolchain not deprecated? (if it is, point to this page, suggest to create PR for newer toolchain, and close current PR)
    • Does it use the right toolchain? (move to lower toolchain if appropriate)
    • Does it have a sanity check command? Typically a --version or --help suffices. (note: not needed if EasyBlock implement a default, e.g. PythonPackage by default will try import %(namelower)s)
    • Does it require a versionsuffix? (e.g. no more -Python since 2021; CUDA should be in suffix since 2021, etc)
    • Is the order of keywords ok? (not a hard requirement, but convention is to list in the order that items are used/executed)
  2. Visual review of patches
    • Does it contain a comment at the top about the issue the patch solves? Preferably, a reference to an EasyBuild Github issue, or an issue page of the software in question
    • Is there an upstream PR? If so, include in comment so it is clear when the patch becomes obsolete.
  3. CI test suite
    • Does it pass?
    • If not: are failures clear? Does the contributor need help interpreting them?
  4. Compare to similar EasyConfig files
    • eb --review-pr <#PR> (see here)
    • Check that (only) relevant parts were changed compared to previous EasyConfigs. E.g. for version update: version and (if a newer toolchain was used) versions of toolchain & dependencies.
  5. Check dependencies (optional)
    • Check if the dependencies match with what is written in the documentation of that particular software
    • Check with ldd on the libraries/binaries that are installed to see if they don't link to (unexpected) system libraries, in order to spot missing dependencies
  6. Test the PR
    • eb --from-pr <#PR> --rebuild --robot --upload-test-report (see here)
    • Ask boegelbot to test: @boegelbot please test @ generoso (optional: pass CORE_CNT=XX or EB_ARGS=--<some_eb_option>) or @boegelbot please test @ jsc-zen2
    • Test on a local system. Preferably with different OS and CPU than Generoso.
    • Mostly, 2 tests (Generoso and local system) are ok. For key software (e.g. toolchain components or very commonly used software such as GROMACS, TensorFlow, OpenFOAM etc) or if you potentially expect issues on other types of systems (CPU/OS), more tests may be warranted.
  7. Merge the PR
    • eb --merge-pr <#PR>
    • Automatically checks target branch (develop), CI (should pass), last test report (should be succesful), review status (no pending reviews, at least one approved review), milestone (is it set?)
    • Check if relevant labels are set (update of existing EasyConfig/bugfix? New version? Etc)

Review process details

Reviews mainly consist of evaluating the technical aspect of contributions, which includes functionality (what is being contributed?) and code (how was it implemented?). Another aspect that is consider is style (how does the code look?).

The following specific attention points are taken into account when contributions are reviewed:

  • implementation
  • unit test suite
  • code style
  • testing result

Depending on the particular EasyBuild repository (framework, easyblocks or easyconfigs) to which the contribution is being made, there is a different focus on each of these.

Implementation

The implementation of the contribution is critically evaluated. The reviewer is encouraged to issue remarks (by means on inline comments in the pull request) with remarks and suggestions for improvements.

Unit test suite

For every pull request, our Jenkins setup will automatically run the unit test suite for that repository, and report back on the outcome in the pull request itself.

Only pull requests for which the unit test suite (still) passes, i.e. for which Jenkins gives a green light, are eligible for merging.

For contributions to the EasyBuild framework, additional unit tests should be added when new functionality is being implemented, or when existing functionality is extended.

Code style

Contributions are also reviewed on code style, with the intention to maintain a uniform feel across the EasyBuild codebase (which significantly lowers the bar for others).

For Python code, we mainly adhere to the PEP008 code style, with a couple of exceptions (e.g. max column width is 120 characters).

A couple of noteworthy code style aspects are:

  • correct indentation: no tabs, 4-space indentation
  • naming:
  • meaningful variable and function/method names
  • no single letter names (except in list comprehensions)
  • no CamelCase (only in class names)
  • docstrings for modules, classes, and functions and methods are considered required
  • code comments for non-trivial blocks of code are desirable

TODO: put together a script to help with code style review, e.g. based on PyLint (where applicable).

Toolchain

There are a few guidelines regarding which toolchain to use when developing EasyConfigs. In general, using lower level toolchains is preferable, since it makes it easier to reuse the software package in various different high level toolchains. E.g. a packaged built with GCCcore can be used as dependency in both foss and intel toolchains. But there are reasons to use higher toolchains:

  • Higher level toolchains (e.g. foss, intel) are needed if your software uses components from those toolchains (e.g. MPI, BLAS, FFTW libraries)
  • Higher level toolchains are needed if dependencies of your software package already use those higher level toolchains
  • GCC should be used instead of GCCcore if the software contains Fortran-compiled code (reuse of Fortran code compiled with GCCcore in Intel toolchains would not work, because gfortran and ifort Fortran modules are not compatible)
  • GCC / iccifort should be used instead of GCCcore if there would be performance benefits when compiling with the Intel toolchains (rather than building with GCCcore and reusing that in both toolchains)

Testing result

Obviously, the contributed functionality or support should work for other people interested in it.

The person reviewing the contribution is expected to test it as good as possible, and report back on the result in the pull request.

Automated testing of easyconfigs pull requests

Since EasyBuild v1.13, the command line options --from-pr and --upload-test-report are available to easily test easyconfigs pull requests (PRs) on your system. When --from-pr is used, eb will:

  1. fetch all patched files from the specified easyconfigs PR on GitHub, i.e. easyconfig files, patches, etc.
  2. build all touched easyconfig files
  3. compose a comprehensive test report (in Markdown format)
    • this includes test results, system information (OS, Python, ...), EasyBuild configuration, session environment, etc.

When --upload-test-report is used in addition, eb will also:

  • upload the test report as a gist on GitHub
  • upload build logs for failed builds as gists
  • post a comment in the respective pull request on GitHub
    • because of this, a GitHub username and token are required

Notes:

  • you are responsible for making sure that the required versions of easybuild-framework and easybuild-easyblocks are being used
    • this may be the latest develop branches, or an update available in a particular PR (especially w.r.t. easyblocks)
  • you can test an easyconfig PR without reporting back a test result using only --from-pr (which doesn't require GitHub credentials)
  • you can generate a test report without uploading it to GitHub using --dump-test-report
  • generating a test report also works with other sources of easyconfigs files, e.g. local files, your EasyBuild installation, etc.
  • you should filter the environment dump that is included in the test report using the -test-report-env-filter configuration option, e.g. --test-report-env-filter='^SSH|USER|HOSTNAME|UID|.*COOKIE.*'
Example usage

For example, to test https://github.com/hpcugent/easybuild-easyconfigs/pull/767 (goolf/1.5.14-no-OFED toolchain + gzip test case), use:

eb --from-pr=767 --github-user=GITHUB_USER --upload-test-report -dfr --test-report-env-filter='^SSH|USER|HOSTNAME|UID|.*COOKIE.*'

A couple of remarks:

  • a valid GitHub username must be supplied via --github-user
    • a GitHub token for that user must be available in your systems keyring
  • --robot (-r) is required to make sure the builds are being executed in the right order, i.e. taking interdependencies into account
  • --force (-f) is required to make sure that modified easyconfig files are being rebuilt if they were built before
  • --debug (-d) is nice to have, especially w.r.t. failed builds for which build logs are also uploaded
  • filtering the environment dump using --test-report-env-filter is advisable
Setting things up

To obtain a GitHub token:

  1. visit https://github.com/settings/tokens/new and log in with the GitHub user account you would like to use with EasyBuild
  2. enter a token description, e.g. "EasyBuild easyconfigs PR testing"
  3. make sure (only) the repo and gist scopes are enabled

To install your GitHub token in your systems keyring:

  1. make sure the keyring Python module is available for the Python version you're using
    • check with python -c "import keyring"
    • note: if you're still using Python 2.4.x, you'll need to use keyring version 0.5.1; more recent are not compatible with Python versions older than 2.6
    • one some systems, you may need to install the keyrings.alt module too
  2. add your GitHub token in your keyring using Python:
# pro tip: copy-paste this, but do replace GITHUB_USER with your own GitHub username
python -c "import getpass, keyring; keyring.set_password('github_token', 'GITHUB_USER', getpass.getpass())"

To check that your GitHub token was installed correctly, use:

python -c "import keyring; print keyring.get_password('github_token', 'GITHUB_USER')"

Note

Depending on your OS configuration and the version of the Python keyring module you're using, you may be requested to use a master password when accessing your keyring. More specific, on desktop systems (GNOME, KDE, OS X, …) the keyring module will integrate with your system keyring.

While storing your GitHub token encrypted and password-protected is important, this limits the usefulness of --upload-test-report since it implies manual intervention.

If you feel strongly about being able to use --upload-test-report fully autonomously you can configure keyring to store your GitHub token differently, thus avoiding a master password.

To do so, create the following text file at $HOME/.local/share/python_keyring/keyringrc.cfg before you store your GitHub token in your keyring:

[backend]
# depending on version of the python-keyring package
# newer versions (supported since python-keyring v1.1)
default-keyring=keyring.backends.file.PlaintextKeyring
# older versions (python-keyring v1.0 or older)
# default-keyring=keyring.backend.UncryptedFileKeyring

Note that this does not imply that your GitHub token is stored in clear text (it’s base64 encoded), see the file named keyring_pass.cfg in $HOME/.local/share/python_keyring, although it is definitely less secure to not use a master password.

Reviewers/testers

Anybody who feels confident enough to review and test a particular contributions can do so.

To help streamline the reviewing and testing process, a couple of EasyBuild community members have agreed with taking up the responsibility of reviewer/tester for the different repositories.

Overview

Name GitHub user id Affiliation Test systems
Stijn De Weirdt @stdweird HPC-UGent Scientific Linux 6
Pablo Escobar @pescobar UniBAS ?
Petar Forai @pforai GMI ?
Fotis Georgatos @fgeorgatos freelance Debian 6, RHEL6
Kenneth Hoste @boegel HPC-UGent Scientific Linux 6, (OS X, OpenSUSE)
Ward Poelmans @wpoely86 UGent Debian 7.2, Fedora 20, SL6
Jens Timmerman @JensTimmerman HPC-UGent Scientific Linux 6

Assignments

  • framework:
  • Kenneth Hoste (HPC-UGent, release manager), @boegel
  • Stijn De Weirdt (HPC-UGent), @stdweird
  • Jens Timmerman (HPC-UGent), @JensTimmerman
  • Ward Poelmans (UGent), @wpoely86
  • easyblocks:
  • Kenneth Hoste (HPC-UGent, release manager), @boegel
  • Jens Timmerman (HPC-UGent), @JensTimmerman
  • Ward Poelmans (UGent), @wpoely86
  • easyconfigs:
  • Kenneth Hoste (HPC-UGent, release manager), @boegel
  • Pablo Escobar (UniBas - SIB), @pescobar
  • Petar Forai (GMI), @pforai
  • Fotis Georgatos (freelancer), @fgeorgatos
  • Ward Poelmans (UGent), @wpoely86

You do not need to be listed as a voluntary reviewer to help out with reviewing pull requests, either occasionally or just once.

If you would like to join the list of volunteers for reviewing contributions, please contact Kenneth Hoste.

Clone this wiki locally