Package Management, Linting / Code Quality and GitHub Actions Improvements

[oldfielj-ansto]

This PR makes changes in several places, none of which impact the overall functionality of the MXCuBE Core package. These changes aren't meant to be merged in their current state, rather they are intended to start a discussion around package management tools, linting/code quality tools and package distribution.

Summary of Changes:

• Switch to Poetry "pyproject.toml" over traditional "setup.py" and "requirements.txt" package management.
  • Thanks to PEP 517 this change would have no impact on deployment, pip install mxcubecore will work the same way as it always has.
  • The only real negative that I can see here is that "bump2version" will stop working and will need to be re-implemented, but the good news here is that Poetry has a feature to support project version bumping automatically, so a new GitHub action could be implemented to instruct Poetry to increment the minor version each time a PR is merged into the "develop" branch.
  • Switching to Poetry provides access to two new useful commands poetry build and poetry publish, the build command will automatically build the sdist/wheel archives for distribution, the publish command will update a PyPi repository listing and upload the new version attaching the archives generated by the build command. This simplifies distribution overhead and opens up the opportunity to automate the entire process via GitHub actions.
  • A question that might come up as a result of this PR:

    What effect will this have on MXCuBE3, considering we are using Conda to manage the environment?

    • It will have no impact, as mentioned above pip install mxcubecore will work the same and we can even use the Poetry tool directly with Conda as there is a Poetry package maintained on Conda-Forge and running poetry install from within the MXCuBE3 Conda environment will install the MXCuBE Core project into the existing Python environment create by Conda.

• Changes to linting/code quality tools.
  • Note that none of the changes here are intended to effect facility specific code under "mxcubecore/HardwareObjects", it should be up to each facility to first decide, if following these standards matters to them, which standards they want to enforce and how they want to implement that enforcement.
  • Implement Pre-Commit and associated ".pre-commit-config.yaml" configuration file.
    • Pre-Commit is a Python package that can be installed from PyPi and it's designed to simplify management of GIT Pre-Commit hooks.
    • The use case here is to specify linting/code quality requirements for Black and PEP conformity, minimising the need to manually run multiple commands before raising a new PR, all Black flags are specified here and Pre-Commit can be run from inside the virtual environment by calling pre-commit run -a.
    • Since it can be easy to forget to run the commands associated with linting/code quality tools, Pre-Commit offers the ability to generate and install GIT Pre-Commit hook scripts to your local development repository pre-commit install and once installed these scripts will cause new commits to fail if Pre-Commit has not been run and the problems resolved, this generally acts as a final sanity check before trying to raise a PR and realising there is an issue at that point.
  • Take Black out of "safe-mode", current contributing guidelines specify that Black should be run in safe-mode, this prevents Black from making structural changes to the code in areas such as module imports, making this change will mean Black makes large changes to the codebase, in the order of hundreds of files, but it will mean legacy code features such as star-imports will be cleaned up and removed.
  • Add the Flake8 code quality tool, similar to Black, Flake8 is an opinionated code style enforcement tool, it's introduction here with the current settings will mean a large refactor of the entire codebase would be required, the issues it did complain about are too numerous to list here, we should debate if its introduction is acceptable given the workload of changes it will generate and if so, how that workload can be shared so as not to burden any one person with what is likely weeks of work.
• Introduce new/update existing GitHub actions.
  • Pytest (tests.yml)
    • Executes Pytest test cases for Python [3.7, 3.8, 3.9, 3.10].
    • Tests action has been updated to install the environment using Poetry.
    • Tests will run as status checks against PR's and will provide immediate feedback to reviewers if there is an issue that has been found.
    • Tests are currently configured to only run against a PR when a code change is present in a ".py" file.
  • Linting & Code Quality (pre-commit.yml)
    • Executes pre-commit run -a to run linting/code quality tools, essentially this is a check to tell the reviewer if the contributor has remembered to run these tools before submitting the PR.
  • Coverage Report (coverage_report.yml)
    • Executes the Pytest test cases again, this time with the "pytest-cov" plugin enabled, if the tests are passing the action will post a comment on the PR with total project coverage and a summary of each changed files coverage, listing any lines of code that have been missed while running the test cases.
    • Each time the PR is updated the action will re-run and update the posted comment.
  • Create/Update Tag (create-tag.yml)
    • Creates/Updates GIT tags anytime a new commit is pushed to the "develop" branch, this will trigger for both PR's being merged and commits pushed directly to the branch.
    • Tags are named based on the version specified in the "pyproject.toml" file, so a project version defined as "1.0.0" would result in a tag being created called "v1.0.0".

[oldfielj-ansto]

The "Linting & Code Quality" action fails here due to the aforementioned Pre-Commit settings for Black, etc.

The "Coverage Report" action fails due to the repository GitHub action permissions as they are currently configured, this can be rectified without changing those settings by generating and providing the action with a new token that has sufficient permissions to comment on a PR.

An example of the what the "Coverage Report" action comment looks like can be found here. https://github.com/AustralianSynchrotron/mxcubecore/pull/3#issuecomment-1266292792

[marcus-oscarsson]

Thanks a lot @oldfielj-ansto for this very nice PR, It is very much appreciated :+1:

[marcus-oscarsson]

I like the idea of trying out Poetry, I did actually consider it when we introduced bump2version but did not have the time to investigate it enough. It seems to me to provide a more complete solution over setup.py, I like the overview the pyproject.toml gives and that it handles virtual environments (basically a must when working with Python)

Over all it looks like a good update of our current routines, I suspect that we might need to discuss what to do with the Flake8 warnings/errors. I think we should try to get as much of this as possible in place, perhaps excluding Flake8 to start with. Then come up with a plan on how to perform the necessary changes for Flake8, if we think it will bring us some additional value.

[rhfogh]

It is very good that someone is doing this. It will be painful to incorporate, but then taking your medicine always is.

Could I propose that someone runs the full black/flake8 on a separate branch and makes the linted branch and the linter report available to all of us? We could then look at the kind of changes that would await us, digest it, and then discuss what checks we might want to disable (if any, of course).

[marcus-oscarsson]

Looks great :) thanks !

[marcus-oscarsson]

Ill go a head and merge this, there might be some adjustments later on but it puts a lot of good things in place.

[oldfielj-ansto]

@marcus-oscarsson Cool OK, I'll need to raise another PR to fix an issue I've been troubleshooting with Python3.11.

Seems that the "gipc" package is failing to install as it is not yet compatible with the latest release of Gevent. Since it looks like this package is only used by sites requiring the "TangoLimaVideo" and "TangoLimaVideoLoopback" hardware objects, I should be able to lock it to Python3.10 and below, allowing sites not using these hardware objects to start experimenting with Python3.11.

I've also been playing around with the code quality / linting tool configs and have gotten the number of automatic refactors down to 112, which I think is a more manageable starting point than the 800+ we started with in this PR.

Just working on adjusting the Flake8 configuration to document what the warning being disabled are, and muting some of the more nuisance spam warnings.

[marcus-oscarsson]

@oldfielj-ansto. Ok I see. I had the same problem and simply pinned gevent to "21.12.0" which I think should work fine as well.

We will tune these things a bit as we go, I guess its the easiest way. Sounds great for the linting, 112 errors sounds manageable :),