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

Upgrade package and repo infrastructure #156

Open
wants to merge 45 commits into
base: dev
Choose a base branch
from
Open

Conversation

AAriam
Copy link
Collaborator

@AAriam AAriam commented May 11, 2023

Description

This PR upgrades the OpenCADD package and repository infrastructure, according to latest guidelines, standards, and software best practices.

Content

1. Upgrade build system (setup.pypyproject.toml)

  • Status: Done

The packaging build system is upgraded in compliance with PEP 621:

  • All project metadata are written in TOML format in pyproject.toml.
  • setup.py is removed.
  • setuptools version is updated from <60 to >=61.

References:

2. Upgrade supported Python versions ([3.7, 3.8] → [3.9, 3.10, 3.11])

  • Status: Done

According to Python packaging guidelines (e.g. see conda-forge guidelines), package builds must be available for the current Python version (3.11), and (at least) two preceding minor versions (3.9 and 3.10).

New functionalities that are to be added to OpenCADD require at least Python 3.9, in compliance with the guidelines. Therefore, the supported Python versions were updated as follows:

  • Added minimum Python version (i.e. 3.9) in pyproject.toml.
  • Updated GitHub Action workflows to run tests on Python versions 3.9, 3.10, and 3.11, instead of 3.7 and 3.8.
  • Removed dependency pin (for biopython), which was preventing the new environments (with update Python versions) to solve.

References:

3. Change versioning tool (versioneerversioningit)

  • Status: Done

versioningit is a modern alternative to versioneer, which is now used as default in the new Cookiecutter-CMS template.
It integrates well with pyproject.toml, is more straightforward to use and maintain, and has all the required functionalities.

To replace versioneer with versioningit:

  • versioneer.py and opencadd/_version.py are removed, along with references in opencadd/__init__.py, setup.cfg and MANIFEST.in.
  • versioningit configs are added in pyproject.toml.
  • new dynamic variable opencadd.__version__ is referenced in opencadd/__init__.py and .gitattributes.

References:

4. Upgrade GitHub Action workflows

  • Status: Done
  • Python version matrix for testing is changed from [3.7, 3.8] to [3.9, 3.10, 3.11].
  • windows-latest is added to OS matrix for testing.
  • For setting up conda environments, provision-with-micromamba is now used instead of setup-miniconda. This is the default recommended Action used in the new Cookiecutter-CMS template, which uses micromamba instead of conda, which among others, has the advantage of being much faster (CI workflow now runs in less than half the time).
  • The workflow for testing documentation build (.github/workflows/docs.yaml) is merged into the CI as a new job, to reduce boilerplate, and increase maintainability.
  • All jobs and steps in the CI workflow are properly named, to facilitate CI investigations.

References

5. Organize repo structure

  • Status: Done

According to the PyPA guidelines, the directory for test files should be placed in the root directory of the repository, named tests. More importantly, it should not be placed inside the package directory, since

  • it is not a part of the package, and should not be deployed with the package content, and
  • it may interfere with some processes (e.g. see this issue).

Therefore, the tests folder was moved from within the package directory into the root, and all paths and imports were corrected accordingly.

References

6. Update pylint configurations

  • Status: Done

The configurations for static code checking using pylint during the CI workflow are stored in the root, under .pylintrc. The current configurations were incompatible with updated pylint versions based on the updated Python versions. Moreover, they were strictly limited, meaning most checking criteria were disabled. Therefore, the configuration file was replaced with the current up-to-date recommended version.

References

7. Other small improvements

  • Status: Done
  • Updated .gitignore list: added PyCharm and MacOS-specific files, plus temporary _version.py files that are built during build by versioningit.
  • Deleted leftover dummy files from the initial Cookiecutter template.
  • Updated email address and year in CODE_OF_CONDUCT and LICENSE, respectively.
  • Added marker file according to PEP 561.

Issues

Incompatible dependencies and failing unit-tests

After changing the supported Python versions from (3.7, 3.8) to (3.9, 3.10, 3.11), the CI workflow indicated that the current defined environment (in devtools/conda-envs/test_env.yaml) could not be solved with the new Python version specifications, due to the package biopython being pinned at version <1.77 , which requires Python <=3.8.

Thus, the pin was removed from the environment file. As a result, OpenCADD can now be built and run on the three latest Python versions. However, eight of the test functions are now failing. As expected, most of them relate to functionalities using the biopython package. However, some functions related to mmligner are also failing (although it was not pinned before). Moreover, since the code coverage of the tests is not complete, there may be other functionalities that are failing as well.

  • Fix biopython related issues (mostly in superposition module, see here) <- @pipaj97 can you take care of this?

Incompatibility with Windows OS

Adding testing for Windows OS reveled that the current OpenCADD package is not compatible with Windows and cannot be installed, due to following Windows-incompatible dependencies, which are all used in opencadd.structure.superposition package: clustalo, mmligner, muscle, theseus.

  • Exclude windows OS from test suite for now
  • Later, tackle this problem with split environments or so.

Errors due to Pylint configuration change

Investigating the CI workflow results after updating Pylint and its configuration revealed potential errors (see here), which were previously suppressed by the more restrictive analysis configurations of Pylint.

  • Fix in klifs related code <- @dominiquesydow can you take this over?
  • Fix the remaining part

Status

  • Ready to go

All necessary changes to the package infrastructure and repository are successfully applied. The PR can be merged into the dev branch. However, before continuing and merging into main, a strategy to address the issues mentioned above should be discussed.

AAriam added 30 commits May 10, 2023 19:56
Add PyCharm (`.idea`), MacOS (`.DS_Store`) and `_version.py`
To be replaced by `versioningit`
Remove `versioneer.py`.
The manifest itself should not be included either.
`_version.py` will be automatically created by `versioningit` during build. See [cookiecutter docs](https://github.com/MolSSI/cookiecutter-cms#versioningit) for more detail.
Bump Python versions to 3.9, 3.10, 3.11
No need for `__init__` in tests folder when using `pytest`; but may confuse other tools.
`pylint` and `black` are already declared in `test_env.yaml` and are installed in the previous step
@AndreaVolkamer AndreaVolkamer added the enhancement New feature or request label May 31, 2023
@AndreaVolkamer
Copy link
Member

@dominiquesydow and @pipaj97 I tagged you both in the description above to potentially help to solve one of the issues related to your code bases. Please let us know if you have time to look at it?

@dominiquesydow
Copy link
Contributor

Hi @AndreaVolkamer, could you please point me to the OpenCADD-KLIFS problem you are referring to?

@AndreaVolkamer
Copy link
Member

@dominiquesydow
Copy link
Contributor

Thanks, @AndreaVolkamer, I am good with the pylint config changes.
I won't update the klifs related code right now in this PR as I am short on time & this has not a high priority. Happy for you to go ahead with a PR merge even if pylint fails for some of the packages.
Two options in my view: (a) Feel free to apply the updates yourself and tag me as a reviewer or (b) I am also happy to make the updates myself in a follow-up PR.
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants