Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernise Python tooling #50

Merged
merged 28 commits into from
May 31, 2023
Merged

Modernise Python tooling #50

merged 28 commits into from
May 31, 2023

Conversation

niksirbi
Copy link
Member

Hey @ClementineDomine and @rodrigcd, me and @adamltyson have worked on updating the Python tooling that underlies the package, with the purpose of bringing it up-to-date with current Python development practices and making it more futureproof.

This PR is quite large, as it makes sweeping changes across many files, so take your time to review it and we can also discuss these changes in person during our next meeting. Below I will try to give a summary of the main changes.

Consider all of these as recommendations. We can roll back some of the changes if you think they will not be useful for this project.

Summary of changes

Port package build to pyproject.toml

This closes #34. The pyproject.toml file replaces setup.py as the main configuration file for specifying how a Python package should be built. It contains most of the information that was previously in setup.py, with some additions.

  • It splits the dependencies into two groups (closes Separate user and dev requirements #45)
    • "Normal" dependencies, which will be installed when running pip install -e ., or - after release on PyPI - pip install NeuralPlayground.
    dependencies = [
      "scipy",
      "pandas",
    • Optional "dev" dependencies, which will be installed when running pip install -e '.[dev]', or - after release on PyPI - pip install NeuralPlayground[dev].
    [project.optional-dependencies]
    dev = [
      "pytest",
    These are meant for developers who want to contribute to the project. The purposes of some of these extra "dev" dependencies will be explained below.
  • It configures many of the developer tools - like black, ruff, pytest - within the same file.

As a consequence of these changes, the existing requirements file is no longer necessary (I removed it).

Placed MIT license in the root folder

This closes #43. For GitHub to automatically recognise the LICENSE it must be in the root folder. MIT is a good, permissive license, and alongside the BSD-3 clause, it's the license we would anyway recommend to most researchers working at SWC/Gatsby. For more information on this, see out Software Licensing Guide

Automated linting and code formatting

This closes #38.

  • Previously, you were using flake8 to check for linting. We prefer ruff for doing the same job, as it is newer, faster, widely adopted, and also does a number of other jobs (for example it can automatically sort the imports at the top of a Python file, to minimise unnecessary diffs in pull requests.)

  • To take this issue one step further, we use black to enforce a uniform code style. Working together, ruff and black will not only flag style violations but also auto-correct them for you. Hopefully, this will eliminate any time you spend on formatting/styling the code and help you focus on the substance. Of course, matters of (code) style are subjective, and you may disagree with some of the stylistic choices made by black. In our opinion, the time saved is worth the occasional aesthetic discomfort (which varies wildly across developers anyway). To quote:

    Black is opinionated so you don’t have to be.

  • We have further automated the above steps, using pre-commit hooks. These are configured in the pre-commit_config.yaml file, and you - as the developers - can use them in the following way:

    • The first time you clone the updated repo, install the package locally via pip install -e '.[dev]' (this will fetch all the dev dependencies)
    • Initialise the pre-commit hooks by running pre-commit install in the repository root.
    • From now on, every time you try to git commit, pre-commit will run the linting checks for you, flag them, and try to automatically fix them for you. If everything is already compliant with PEP8 and your style options, the commit will run to completion. If not, pre-commit will make the changes for you, which you can then stage (git add .) and try to git commit again. This way, every piece of code that gets committed is guaranteed to be free of linting issues and to have a uniform style from the get-go.
    • You can also trigger pre-commit manually at any time, by typing pre-commit run (this will check the staged files only), or pre-commit run --all-files.
  • Most of the changes introduced by this PR are in fact automatic changes applied by the pre-commit hooks to enforce a consistent style (that's why the PR is huge)

Note
Normally, within the Neuroinformatic Unit we also use mypy for static type checking, and this is also part of our usual pre-commit hooks (following ruff and black). However, for mypy to work properly, it expects you to consistently use type hints throughout your codebase. As things currently stand, mypy was flagging too many issues that would require lots of editing of the modules you have written. I've decided to omit mypy for now, but we can keep the option open for adding it in a future PR, if you would find it useful.

Note
black uses 88 as the maximum line length, while we in the Neuroinformatics Unit usually use 79. You were using 127, which I think is too long (but hey, style is subjective). I've left it at 127 for now. Reducing it to 88 using black would require manual intervention, as most of the long lines appear in docstrings, the automatic formatting doesn't apply to those. If you wish to shorten the line length in the future, you have to manually split the long lines in docstrings, and change the line length parameter in the configuration entries for ruff and black in pyproject.toml

Warning
Of all the changes we are proposing, these ones will result in the most shift in your day-to-day development workflow. There may be some "growing pains" associated with these until you get used to the new tools, and of course, all changes are simply proposals, and thus optional. We personally believe that these "growing pains" are more than worth it and will be a huge timesaver in the long run.

Moved tests outside of the module source code

By convention, the folder containing the tests is placed in the root folder, as opposed to within the module.

Automated versioning

Closes #36.
We recommend the use of semantic versioning, which uses a MAJOR.MINOR.PATCH versiong number where these mean:

  • PATCH = small bugfix
  • MINOR = new feature
  • MAJOR = breaking change

We introduced setuptools_scm to automatically version the package. It has been pre-configured in the pyproject.toml file. setuptools_scm will automatically infer the version using git. To manually set a new semantic version, create a tag and make sure the tag is pushed to GitHub. Make sure you commit any changes you wish to be included in this version. E.g. to bump the version to 1.0.0:

git add .
git commit -m "Add new changes"
git tag -a v1.0.0 -m "Bump to version 1.0.0"
git push --follow-tags

N.B. It is also possible to perform this step by using the GitHub web interface or CLI.

Added a MANIFEST.in

We have added a MANIFEST.in file to configure what will be included in the source distribution (when it is published on PyPI).

Overhauled the existing GitHub actions workflow

The new workflow is in .github/workflows/test_and_deploy.yml. This relies on a number of reusable Github actions that we maintain. It does a number of jobs, triggered by pushes and pull requests:

  • Checks for linting issues by executing the aforementioned pre-commit hooks.
  • Checks the MANIFEST.in file
  • Runs the tests on multiple Pythong versions and operating systems. Currently, it's configured to test across Python 3.8-3.11 on Ubuntu, but this is subject to change (for example we can also add Windows/MacOS tests). The orchestration of tests across the multiple images is handled by tox (hence the additional tox.ini file which configures it). While we were working on this, we came across issue Test visualisations #49
  • Build the package and uploads it on PyPI, if a git version tag is pushed. The first release on PyPI has to be done manually, but after that, the releases will be done automatically as long as we tag a new version.

@niksirbi niksirbi marked this pull request as ready for review May 23, 2023 13:33
@niksirbi
Copy link
Member Author

After we merge this, we should write new development guidelines, in line with the changes made, see #46

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@fb81862). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #50   +/-   ##
=======================================
  Coverage        ?   65.91%           
=======================================
  Files           ?       18           
  Lines           ?     1373           
  Branches        ?        0           
=======================================
  Hits            ?      905           
  Misses          ?      468           
  Partials        ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ClementineDomine ClementineDomine merged commit b328a71 into SainsburyWellcomeCentre:main May 31, 2023
@ClementineDomine
Copy link
Collaborator

@all-contributors please add @niksirbi for Infrastructure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants