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

GitHub action to setup poetry #1

Merged
merged 6 commits into from Feb 8, 2022
Merged

GitHub action to setup poetry #1

merged 6 commits into from Feb 8, 2022

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Feb 2, 2022

In getting Sydent's dependencies to be managed by poetry I have composed a reusable action setup-python-poetry which sets up poetry for CI to use. Other parts of the WIP changes in Sydent make use of this action, but this is the "lowest" build block with no dependencies. Therefore I'm putting it up for review first.

I did look at prior art here, mainly snok/install-poetry. But that action required the user to manually setup caching; I wanted that to Just Work (TM). There's also a PR for actions/setup-python which proposes to add support for caching poetry installations. However, I think that is concerned with caching the poetry-managed venv only; I also wanted to cache the installation of poetry itself.

Explanation

To see this in action, visit https://github.com/matrix-org/sydent/actions/runs/1752681044, which is part of matrix-org/sydent#488. That is a Sydent PR which tests three layers of GitHub actions config:

  1. In the Sydent PR itself, there is a single "pipeline" workflow which uses two callable workflows in "backend-meta" and the setup-python-poetry action.
  2. My branch in backend-meta contains two "reusable workflows": one for linting a poetry project, and one for building wheels and sdists. It's not necessary to put these in another repo, but I want to try and provide reusable building blocks for other projects. The linter workflow uses the setup-python-poetry action too.
  3. setup-python-poetry is the action defined in this repo and proposed in the current PR.

setup-python-poetry is a "composite action". It pip-installs poetry, then uses poetry to create a new virtual environment according to pyproject.toml. actions/setup-python does the hard work for us, but we make use ofactions/cache to avoid having to re-install poetry and the project unless the python version, poetry version, or the lockfile has changed between CI runs.

After using this action, subsequent job steps can use poetry run foo [...] to run foo [...] in the virtual environment.

If accepted, best practice for actions says we should make a release branch and tag the appropriate version as v1.

actions/setup-python does the hard work for us, but we make use of
actions/cache to avoid having to re-install poetry and the project
unless the poetry version we're using has changed, or the hash of
the lockfile has changed.

After using this action, subsequent job steps can use `poetry run foo
[...]` to run `foo [...]` in the virtual environment.
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

Is having one repo per action the thing we're meant to do? It feels very heavyweight but I suppose you can't help it if you can't help it.

action.yml Outdated
run: echo "::set-output name=dir::$(python -m poetry config virtualenvs.path)"
shell: bash

- name: Restore poetry venv
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling this 'Restore' is a little bit confusing because I think this is also saving to the cache afterwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In one of the final steps, yes (but only if there wasn't a cache hit in the first place)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about Restore/cache foo bar?

action.yml Outdated
Comment on lines 78 to 83
- name: Dump virtual environment
if: "${{ inputs.debug == 'true' }}"
run: |
poetry env info
poetry show
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

If this output is only small I would be tempted to emit it unconditionally — when CI fails it's a nuisance to have to restart it all up in debug mode (?) to find out why, when it could just have been in the logs.

Comment on lines +25 to +27
- name: Check for poetry lock
run: "test -f poetry.lock || (echo No lock file! && false)"
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the poetry lock file a necessary requirement? What about for libraries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm proposing that we only use this action for projects with a lockfile.

The conventional wisdom for libraries is not to commit a lockfile. That means each CI run would end formally resolving dependencies, which can be sluggish. See for instance this older attempt in signedjson: poetry install --no-interaction took 22 seconds.

I suspect that building a wheel and installing it in editable mode with pip would be quicker. But more generally I've just kicked the "what do we do about libraries" can down the road.

@@ -0,0 +1,83 @@
name: Setup Python and Poetry
description: Setup Python and Poetry. Cribbed from snok/install-poetry.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to carry any licensing info across, and/or perhaps link to it in a hypothetical README?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Cribbed" is maybe the wrong word. I looked to see how that action worked, but in the end decided I wanted different behaviour and wrote most of the yaml from scratch. snok's action was very useful for understanding what is/was going on with poetry though.

I'll acknowledge this and mark this as MIT licensed to match snok/install-poetry. I know we normally use Apache 2, but given that this is CI config rather than a core software component I think that's fine.

@DMRobertson
Copy link
Contributor Author

Is having one repo per action the thing we're meant to do? It feels very heavyweight but I suppose you can't help it if you can't help it.

You can have actions kept within a project (they recommend a subdirectory of .github/actions) and refer to them by path. I tried putting this action into backend-meta, but when I called backend-meta's reusable workflow from the sydent PR, the CI was looking for the action in the sydent repo, not the backend-meta repo. I tried a few other things and got syntax errors for my trouble. In the end I decided to stick with the thing that works, even if I didn't like it.

David Robertson added 4 commits February 3, 2022 16:22
Given that I've cited snok/install-poetry I've used theirs but stuck in
m.org as a secondary author
@DMRobertson DMRobertson merged commit 354f85b into main Feb 8, 2022
@DMRobertson DMRobertson deleted the dmr/wip branch February 8, 2022 10:45
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.

None yet

2 participants