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

[Resolves #894] Convert the project to use poetry #1035

Closed
wants to merge 19 commits into from

Conversation

tarkatronic
Copy link
Member

@tarkatronic tarkatronic commented May 7, 2021

The intent here is to change the tooling over to use poetry instead of setuptools. This is unfortunately not a small change, especially for a project of this size. However as you can see, it will lead to a great consolidation of tools and files.

Some important things to note:

  • Poetry is smart enough to automatically include data files such as the *.json that had to be manually included by setuptools.
  • twine is no longer necessary as we can simply use poetry publish.
  • Poetry handles all virtualenv matters automatically. So we no longer need to instruct users to construct virtualenvs for development purposes, nor do we need to manually create and manage them for CI.
  • Dependency specifications are a bit different with Poetry, and a bit more "modern", as seen here.
  • I have told git (via the .gitattributes) file to treat the poetry.lock file as binary. This is a fairly standard practice, also followed in the JS community with the package-lock.json file. This does two primary things:
    • Tells GitHub to not attempt to display a diff for this file
    • Prevents frequent merge conflicts, since this tends to be a frequently updated file
  • I have included awscli and all of the Sphinx-related requirements as dev requirements. This means that they will be installed when a developer runs poetry install. If that's not desirable, I can remove awscli, and move the Sphinx requirements to an "extra", as seen here. This latter part would mean that you would have to run poetry install -E docs to get the Sphinx requirements.

Note that I am starting this as a draft PR while I work on it, to show my progress. There will likely need to be significant work done to uplift the CI to work with poetry. I can pretty much guarantee that it WILL NOT pass right now. But luckily, CircleCI may have an easy way to help facilitate this!

Checklist for completion of this change

  • Set up base poetry config in pyproject.toml
  • Update tox to use poetry
  • Update Makefile to use poetry
  • Update Dockerfile to use poetry
  • Update CircleCI to use poetry
  • Remove setuptools config

PR Checklist

  • Wrote a good commit message & description [see guide below].
  • Commit message starts with [Resolve #issue-number].
  • Added/Updated unit tests.
  • Added/Updated integration tests (if applicable).
  • All unit tests (make test) are passing.
  • Used the same coding conventions as the rest of the project.
  • The new code passes pre-commit validations (pre-commit run --all-files).
  • The PR relates to only one subject with a clear title.
    and description in grammatically correct, complete sentences.

Approver/Reviewer Checklist

  • Before merge squash related commits.

Other Information

Guide to writing a good commit

@tarkatronic tarkatronic force-pushed the poetry branch 4 times, most recently from 88a756e to df89b63 Compare May 19, 2021 15:31
zaro0508 pushed a commit to Sceptre/sceptre-circleci that referenced this pull request May 21, 2021
This will have zero impact on existing CI jobs, and will allow progress to move forward on Sceptre/sceptre#1035 without having to continue ugly attempts at path munging to get poetry working properly.

This is the exact approach taken by the base CircleCI Python images:
https://github.com/CircleCI-Public/cimg-python/blob/master/Dockerfile.template#L43
@tarkatronic tarkatronic force-pushed the poetry branch 2 times, most recently from e722a84 to cbf9cd5 Compare May 26, 2021 18:53
@tarkatronic tarkatronic marked this pull request as ready for review August 1, 2021 19:44
@zaro0508
Copy link
Contributor

zaro0508 commented Aug 2, 2021

one of the build failed @tarkatronic

@tarkatronic
Copy link
Member Author

one of the build failed @tarkatronic

Taking a look at it now. Looks like I missed a build step on one of the CircleCI jobs. Should be able to fix it up quickly.

@tarkatronic
Copy link
Member Author

@zaro0508 The CI flow should be fixed now. The code wasn't getting checked out before it attempted to build docs. Whoops!

zxiiro
zxiiro previously approved these changes Aug 3, 2021
@ngfgrant
Copy link
Contributor

ngfgrant commented Aug 3, 2021

I know I have been less involved recently but the changes here are sweeping and just a glance over the request makes me really nervous.

We are swapping setuptools for a dependency on poetry and I would argue gaining very little.

Honestly, I don't think it is worth it. Sorry to be negative and really really value people who are motivated enough to contribute (so thank you).

I will let the others pitch in...

@zaro0508
Copy link
Contributor

I disagree with @ngfgrant. While we may gain very little initially these types of improvements pay off in the long term. I am willing to accept this contribution as long as it gets thorough testing and I'm willing to help with that.

@zaro0508 zaro0508 changed the title Convert the project to use poetry [Resolves #894] Convert the project to use poetry Dec 5, 2021
@zaro0508
Copy link
Contributor

This PR is stale a new attempt has been made in PR #1323

@zaro0508 zaro0508 closed this Apr 11, 2023
zaro0508 added a commit that referenced this pull request Apr 20, 2023
The intent here is to change the tooling over to use [poetry](https://python-poetry.org) instead of `setuptools`.
This is building on the work that was started by @tarkatronic in PR #1035

Some important things to note:

* Poetry will be used for packaging, dependencies, and publishing. 
* Poetry is smart enough to automatically include data files such as the `*.json` that had to be manually included by setuptools.
* `twine` is no longer necessary as we can simply use `poetry publish`.
* Poetry handles all virtualenv matters automatically. So we no longer need to instruct users to construct virtualenvs for development purposes, nor do we need to manually create and manage them for CI.
* Dependency specifications are a bit different with Poetry, and a bit more "modern", [as seen here](https://python-poetry.org/docs/dependency-specification/).
* I have told git (via the `.gitattributes`) file to treat the `poetry.lock` file as binary. This is a fairly standard practice, also followed in the JS community with the `package-lock.json` file. This does two primary things:
  * Tells GitHub to not attempt to display a diff for this file
  * Prevents frequent merge conflicts, since this tends to be a frequently updated file
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.

4 participants