Replace setup.py packaging by Poetry #5266

snejus · 2024-05-29T06:16:35Z

Migrate beets package configuration to Poetry which nowadays seems to be the gold standard.

I have been using Poetry since 2019 and I have mostly been happy a happy user: it makes local dev setup easy and has the tools I need to maintain python packages day to day, including reliable dependency resolution, versioning and publishing to Pypi.

It's a user-friendly tool, so it should make it more straightforward for contributors to setup and navigate the codebase, and ultimately, hopefully facilitate more frequent releases!

Since poetry manages local virtual environment, we do not have much need for tox any more. Therefore, it was replaced by a task runner poethepoet. Type poe in the project directory to see the available commands.

Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
Tests. (Very much encouraged but not strictly required.)

github-actions · 2024-05-29T06:16:50Z

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

Serene-Arc · 2024-06-01T01:51:05Z

I'm fine with this change to poetry in theory, once all the tests and linting errors have been fixed, but a couple of things:

What are the purposes of the changes to release.py? A lot of the things that I removed seem to have been added back in, some of which are definitely not supposed to be there?
What are the purposes of changing the tox file, specifically calling binaries instead of python -m black for instance?
There are some issues with changing version in the release workflow; does this affect any of that?

snejus · 2024-06-01T11:45:45Z

I'm fine with this change to poetry in theory, once all the tests and linting errors have been fixed, but a couple of things:

What are the purposes of the changes to release.py? A lot of the things that I removed seem to have been added back in, some of which are definitely not supposed to be there?

These commits have been made before I noticed that release.py has been adjusted on master - it needs rebasing where it will get removed :)

What are the purposes of changing the tox file, specifically calling binaries instead of python -m black for instance?

Some of the changes have to do with tox upgrade to 4.0.0. Regarding calling the binaries directly - that's the accepted standard, given that every command will be run within the tox environment, where it will be resolved automatically. It is slightly shorter as well.

There are some issues with changing version in the release workflow; does this affect any of that?

When I made changes here, the release workflow did not exist yet, so I made adjustments to release.py instead. When I rebase, I will make sure to update the release workflow instead.

Serene-Arc · 2024-06-02T04:42:44Z

These commits have been made before I noticed that release.py has been adjusted on master - it needs rebasing where it will get removed :)

Fair enough!

Some of the changes have to do with tox upgrade to 4.0.0. Regarding calling the binaries directly - that's the accepted standard, given that every command will be run within the tox environment, where it will be resolved automatically. It is slightly shorter as well.

Great, was just worried with path stuff, but if it's only for the tox environment internally, makes sense.

When I made changes here, the release workflow did not exist yet, so I made adjustments to release.py instead. When I rebase, I will make sure to update the release workflow instead.

Cool.

With regards to the release.py changes, do you want to keep them here or move them to #5274? It might make more sense to get the release workflow working for the current project before doing a big tool change like poetry.

snejus · 2024-06-03T11:52:15Z

do you want to keep them here or move them to #5274? It might make more sense to get the release workflow working for the current project before doing a big tool change like poetry.

I'm completely with you here @Serene-Arc - I'm moving the relevant commits over.

## Description Fixes #5222. Drop Python 3.7. `pyupgrade` is responsible for most of the changes in the code. I undid some of the bits it attempted to update that aren't strictly necessary: 1. Converting `List/Dict/Tuple` -> `list/dict/tuple` in modules that have `from __future__ import annotations` import. This should be done in a separate PR, and for all modules 2. Converting some `.format(` calls to f-strings. It didn't do it consistently, and it should also be done in a separate PR, I believe. Python upgrade unblocks several other PRs, for example #5266 and #5248.

This reverts commit c3b6f07. This commit hardcoded the paths that `isort` and `black` checks. This means that the `check-format` job will act on the entire codebase instead of only changed files. We need to define a `path` argument with a default value in order to achieve the above. Regarding "." vs "beets beetsplug test", the intention behind using "." was to also check python files like `docs/conf.py` and `extra/release.py` which I presume we would also want to format properly.

This reverts commit fc373f5. See CONTRIBUTING.rst which has tools setup guidelines for users. They are expected to install both poetry and poethepoet globally in their system.

This reverts commit af996f4. Since `poethepoet` is installed globally in the workflows, running it does not require `poetry run` suffix. This is actually one of the reasons why it's preferable to have this tool installed globally.

This reverts commit 4550d39. I love this attempt to DRY-up the linting workflow! I remember back in the day also initially assuming that this is how the jobs work. However, I had to meet the harsh reality of each job needing to be set up from zero. :(

This reverts commit 5526bd3. Poetry must be installed before `setup-python` action, weirdly. And we need to install poethepoet globally too!

Serene-Arc · 2024-06-10T23:39:53Z

I'm not sure that checking against the changed files only is the best way to do this. Like when we did the release, it is technically possible for unformatted code to make its way into the codebase, however that happens. Currently, any code will always be found by the next PR because it'll come up with a change to the file or a format error in the CI. If we make it so only changed files are checked, this code can be missed for who-knows-how-long.

Maybe it's paranoid but I prefer the formatting tools checking the entirety of the codebase every time. It makes sure that the formatting rules are actually being enforced at every stage possible.

snejus · 2024-06-11T14:09:20Z

Thanks for clarifying @Serene-Arc, I now see what you mean! Given that the code base is already neat and formatted, I agree that checking everything every time makes more sense! I'm adjusting it now.

I think the only-check-what-has-changed approach makes more sense when the code base is not yet formatted, and is getting tidied up iteratively (if you adjust a file, you must format it properly), which is what we did in most cases in my previous experience. 😅

snejus · 2024-06-11T14:24:59Z

As you predicted @Serene-Arc, there was indeed an unformatted file docs/conf.py that would have been missed! It's fixed now, thanks for your persistence regarding this change! <3

Serene-Arc · 2024-06-12T02:50:22Z

Wonderful! That's all my concerns fixed then, merge whenever you'd like.

Serene-Arc · 2024-06-09T04:37:31Z

extra/release.py

Are there any changes here that aren't in #5274? If not, then it should be reverted in order to stop any merge conflicts when #5274 is merged.

Serene-Arc · 2024-06-09T23:31:56Z

.github/workflows/lint.yml

-      - name: Get changed files
-        id: changed-files
+        with:
+          fetch-depth: 2


If you go to the page for the tool, it says that you need either fetch depth 0 or 2. This makes the fetching 2, which is faster than the entire history (which is what fetch-depth: 0 does)

Serene-Arc · 2024-06-09T23:34:32Z

.github/workflows/ci.yaml

+          cache: 'poetry'
+      
+      - name: Install PyGobject dependencies on Ubuntu
+        if: startsWith(matrix.platform, 'ubuntu-')


Fair, the goal with that was because while we don't currently test it, we could add testing the LTS version of ubuntu as well, rather than just the latest.

Serene-Arc · 2024-06-09T23:36:04Z

.github/workflows/ci.yaml

-
-      - name: Install PyGobject dependencies
-        if: matrix.platform == 'ubuntu-latest'
+          cache: 'poetry'


Ah, okay. Seemed redundant to me because pipx and poetry perform the same role, don't they?

Why does poetry need to be installed before actions/setup-python@v5?

Serene-Arc · 2024-06-09T23:36:57Z

.github/workflows/lint.yml

@@ -35,11 +35,9 @@ jobs:
          files: |
            **.py

-  format:
-    if: needs.changed-files.outputs.any_python_changed == 'true'
+  python-setup:


I'd be happy to write that, the fact that we need it and would use it means that others probably would too. Could be a good addition to the ecosystem.

Serene-Arc · 2024-06-09T23:42:35Z

.github/workflows/lint.yml

      - uses: actions/setup-python@v5
        with:
          python-version: ${{ env.PYTHON_VERSION }}
          cache: poetry
-
+      - uses: abatilo/actions-poetry@v2


I use Arch Linux, which requires a different way of managing packages. Installing global packages without using the package manager is discouraged.

Serene-Arc · 2024-06-09T23:47:26Z

pyproject.toml

@@ -152,13 +152,11 @@ poetry = ">=1.8"

 [tool.poe.tasks._black]
 help = "Run black"
-cmd = "black $OPTS $path"
-args = { path = { help = "Path to blacken", positional = true, multiple = true, default = "." } }
+cmd = "black $OPTS beets beetsplug test"


It would be, but the way it's done now changes every python file in the directory. For those that use virtual environments (like me) it means that black actually goes and formats every single python file of every installed package in the virtual environment. I think a better approach would be to add docs and extra to the list of approved formatting directories, rather than just specify the entire directory.

Doing it that way instead of trying to specify the files that have changed also removes a bit of added complexity, and means that it's less likely to go wrong. The added time cost of checking every file rather than a subset is negligible, and it'll catch files that have been changed in strange ways that haven't been caught by the system, like our recent thing with the version number being in single quotes instead of double quotes. Since that wasn't changed in subsequent commits, the tool wouldn't have caught it. I think the goal should be to keep the repository in compliant code at every step possible, rather than trying to keep individual commits compliant.

Serene-Arc · 2024-06-09T23:49:23Z

.github/workflows/ci.yaml

  PY_COLORS: 1
+  IS_MAIN_PYTHON: ${{ matrix.python-version == env.MIN_PYTHON_VERSION && matrix.platform == 'ubuntu-latest' }}


Ah, damn. I'll have to do some testing. The goal with some of these changes was more of a tentative 'what if we tried it this way', so I'll need to do some testing to get it working if you approve of the methods/approach.

Serene-Arc · 2024-06-09T23:50:34Z

.github/workflows/ci.yaml

+        run: |
+          if [ "${{ env.IS_MAIN_PYTHON }}" = "true" ]; then
+            poe test
+          else
+            poe test --no-cov
+          fi


That's very reasonable! It just seemed like the goal of both was to test, and the coverage was a dependent ancillary condition. But definitely makes sense to have it as two actions.

Serene-Arc · 2024-06-09T23:53:47Z

.github/workflows/lint.yml

@@ -35,11 +35,9 @@ jobs:
          files: |
            **.py

-  format:
-    if: needs.changed-files.outputs.any_python_changed == 'true'
+  python-setup:


Right...I admit it's been a while since I thought about GH actions, not since I did a two day deep dive for the release workflow. State isn't kept between jobs but is inside jobs, so a new action is probably the way to go. The repeated setup does seem to be a thing easily abstracted away to another job, and adding a cache might be a good thing too.

snejus · 2024-06-12T09:48:03Z

Ah, I'm only seeing your review comments now, but I guess they are outdated now! 😅 I'm just looking at the last bit to do with the docs linting job, and will merge once it's working as expected.

snejus self-assigned this May 29, 2024

snejus requested a review from wisp3rwind May 29, 2024 06:16

Serene-Arc self-requested a review June 3, 2024 02:56

snejus mentioned this pull request Jun 3, 2024

We should drop Python 3.7 #5222

Closed

snejus force-pushed the introduce-poetry branch 4 times, most recently from 5b0c665 to fa3def3 Compare June 4, 2024 14:08

snejus mentioned this pull request Jun 5, 2024

Upgrade to Python 3.8 / drop Python 3.7 #5283

Merged

3 tasks

snejus force-pushed the introduce-poetry branch 15 times, most recently from aea7ad2 to 5668014 Compare June 7, 2024 15:03

snejus added 8 commits June 10, 2024 13:46

Revert "Add poethepoet to dependencies for devs"

f3df90c

This reverts commit fc373f5. See CONTRIBUTING.rst which has tools setup guidelines for users. They are expected to install both poetry and poethepoet globally in their system.

Revert "Use poetry prefix"

fee65ba

This reverts commit af996f4. Since `poethepoet` is installed globally in the workflows, running it does not require `poetry run` suffix. This is actually one of the reasons why it's preferable to have this tool installed globally.

Revert "Simplify workflow"

17ec590

This reverts commit 4550d39. I love this attempt to DRY-up the linting workflow! I remember back in the day also initially assuming that this is how the jobs work. However, I had to meet the harsh reality of each job needing to be set up from zero. :(

Revert "Use poetry action instead of pipx"

6763722

This reverts commit 5526bd3. Poetry must be installed before `setup-python` action, weirdly. And we need to install poethepoet globally too!

Do not use fetch-depth: 2 to get changed-files, and fix small typo

4752fb0

Update dependencies

6d3b76d

Document the changes

a4ed6ab

snejus force-pushed the introduce-poetry branch from 940299b to a4ed6ab Compare June 10, 2024 12:46

snejus added 2 commits June 11, 2024 15:21

Check all python files for formatting issues

50cf70e

Format docs/conf.py

c2a13ee

snejus force-pushed the introduce-poetry branch from b370179 to c2a13ee Compare June 11, 2024 14:23

snejus force-pushed the introduce-poetry branch 5 times, most recently from c8e8f58 to a4b32ac Compare June 11, 2024 15:28

Serene-Arc approved these changes Jun 12, 2024

View reviewed changes

snejus force-pushed the introduce-poetry branch from a4b32ac to 9cdf51a Compare June 12, 2024 09:49

Fix sphinx docs linting

0507f6f

snejus force-pushed the introduce-poetry branch from 9cdf51a to 0507f6f Compare June 12, 2024 09:52

snejus merged commit d7bf28d into master Jun 12, 2024
9 checks passed

snejus deleted the introduce-poetry branch June 12, 2024 09:59

bal-e mentioned this pull request Jun 13, 2024

feat(ci): proper caching #3615

Closed

snejus mentioned this pull request Jun 15, 2024

Update release workflow with poetry #5308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace setup.py packaging by Poetry #5266

Replace setup.py packaging by Poetry #5266

snejus commented May 29, 2024 •

edited

Loading

github-actions bot commented May 29, 2024

Serene-Arc commented Jun 1, 2024

snejus commented Jun 1, 2024

Serene-Arc commented Jun 2, 2024

snejus commented Jun 3, 2024

Serene-Arc commented Jun 10, 2024

snejus commented Jun 11, 2024

snejus commented Jun 11, 2024

Serene-Arc commented Jun 12, 2024

Serene-Arc Jun 9, 2024

Serene-Arc Jun 9, 2024

Serene-Arc Jun 9, 2024

Serene-Arc Jun 9, 2024

Serene-Arc Jun 9, 2024

Serene-Arc Jun 9, 2024

Serene-Arc Jun 9, 2024

Serene-Arc Jun 9, 2024

Serene-Arc Jun 9, 2024

Serene-Arc Jun 9, 2024

snejus commented Jun 12, 2024

		PY_COLORS: 1
		IS_MAIN_PYTHON: ${{ matrix.python-version == env.MIN_PYTHON_VERSION && matrix.platform == 'ubuntu-latest' }}

Replace setup.py packaging by Poetry #5266

Replace setup.py packaging by Poetry #5266

Conversation

snejus commented May 29, 2024 • edited Loading

github-actions bot commented May 29, 2024

Serene-Arc commented Jun 1, 2024

snejus commented Jun 1, 2024

Serene-Arc commented Jun 2, 2024

snejus commented Jun 3, 2024

Serene-Arc commented Jun 10, 2024

snejus commented Jun 11, 2024

snejus commented Jun 11, 2024

Serene-Arc commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snejus commented Jun 12, 2024

snejus commented May 29, 2024 •

edited

Loading