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

Change python packaging system from setuptools to Poetry #384

Merged
merged 68 commits into from
Feb 10, 2022

Conversation

cflynn3
Copy link
Collaborator

@cflynn3 cflynn3 commented Jan 18, 2022

Closes #379

@cflynn3 cflynn3 marked this pull request as ready for review January 18, 2022 15:02
@connortann
Copy link
Collaborator

connortann commented Jan 18, 2022

[ Edited to consolidate comments]

This looks promising. I think there are still hangovers of the previous packaging setup that need to be migrated before this is ready for merging.

Project metadata

As I understand, python poetry is a replacement for setuptools. To avoid being in a messy state with duplication, I think if we switch to Poetry we should also remove the previous setup and use poetry to build the project.

This would mean moving the package metadata from setup.cfg to pyproject.toml , and also editing the relevant github CI workflows that do the build and deploy steps.

Versioning

I think we need a clear plan for versioning. Python Poetry manages the version using pyproject.toml, which is inconsistent with the current setup using setuptools-scm. If we use Poetry, we need to ensure it correctly picks up the version number from version control.

This could be one Poetry-compatible way of ensuring we still have PEP-440 compliant version strings that are single-sourced from version control:

https://pypi.org/project/poetry-dynamic-versioning/

Deployment

If we switch to using Poetry, we need to ensure that the deploy scripts used internally in bp to our scientific computing environments work correctly. At present, there are setuptools-specific build steps that this will break.

Outstanding concerns

  • Complete migration away from setuptools
  • Single source of PEP-440 version from git tags
  • Appropriately open/restrictive version constraints for users
  • CI build step using Poetry
  • CI publish step using Poetry
  • Ensure bp-internal computing environment build scripts work
  • Ensure poetry not in same python env as resqpy

Copy link
Collaborator

@connortann connortann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per comments above, I think there are still setuptools remnants and the migration to poetry is not complete.

@cflynn3 cflynn3 marked this pull request as draft January 20, 2022 15:00
@cflynn3 cflynn3 self-assigned this Jan 20, 2022
pip install poetry
pip install poetry-dynamic-versioning
poetry build
# pip install resqpy[tests] --find-links=dist/
Copy link
Collaborator

@connortann connortann Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this "test" step should be unchanged - as the previous step builds a python wheel, we should be able to install that wheel in the usual way with pip in this step.

@cflynn3 cflynn3 marked this pull request as ready for review January 20, 2022 17:44
setup.cfg Outdated Show resolved Hide resolved
- name: Install from build
run: |
python -m pip install --upgrade pip
pip install resqpy[tests] --find-links=dist/
pip install poetry-dynamic-versioning
poetry install
Copy link
Collaborator

@connortann connortann Feb 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use the existing pip install resqpy --find-links=dist/ here, to ensure we are running the tests against the built python wheel?

This step was originally intended to ensure that the python wheel functions correctly and can be pip-installed by end users.

@connortann connortann changed the title Fix environments setup Change python packaging system from setuptools to Poetry Feb 4, 2022
@connortann
Copy link
Collaborator

FYI, I've made a PR on the "Install Poetry" GitHub Action to allow it to install plugins as well. However, would recommend waiting until Poetry 1.2 is officially released before using this.

snok/install-poetry#69

Copy link
Collaborator

@connortann connortann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for addressing all of my comments. @andy-beer , FYI this changes the release pipeline significantly so would be good to ensure the release process still works after this is merged.

@cflynn3 cflynn3 merged commit 19a7557 into master Feb 10, 2022
@cflynn3 cflynn3 deleted the fix_environments_setup branch February 10, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix dependency managment and setup instructions for the project
3 participants