DEV-5295 | Replace poetry with uv #1638

benjaminEwhite · 2024-10-04T15:55:37Z

Please fill out each section even if it's just with "N/A"

Plan? (if this draft/incomplete indicate what you intend to do and how)

What's this PR do?

This PR replaces poetry with uv which is a Python package and project manager written in Rust that is very fast and well documented.

This PR makes the following changes:

Updates the pre-commit GH workflow, removing dependencies on Python and a poetry install step that do not appear to be needed.
Updates the test-python.yml GH workflow to use uv over poetry.
Updates .pre-commit-config.yaml to use a UV pre-commit action.
Updates Dockerfile to use uv instead of poetry (taking advantage of a uv docker image). Did a small amount of cleanup on Dockerfile to make more readable (and removed an unneeded step).
Updates Makefile to use uv instead of poetry
Updates README.md to reflect new package/project manager.

Why are we doing this? How does it help us?

uv is very fast, improves dev experience, and reduces complexity around virtual env management.

Are there detailed, specific, step-by-step testing instructions in the associated ticket?

Yes

Did this PR make changes to the user interface that may require changes to the user-facing docs?

No

Have automated unit tests been added? If not, why?

No. Not appropriate for this kind of change.

How should this change be communicated to end users?

N/A

Are there any smells or added technical debt to note?

No

Has this been documented? If so, where?

I created a confluence page to document the performance improvements vs. poetry

I also have drafted an updated version of our confluence page on updating revengine dependencies

What are the relevant tickets? Add a link to any relevant ones.

DEV-5295

Do any changes need to be made before deployment to production (adding environment variables, for example)? If so, open a ticket and link it to the ticket for this PR and list it here:

No

Are there next steps? If so, what? Have tickets been opened for them? List next-step tickets here:

No

.envrc.example

…t-for-poetry-pyenv' of github.com:newsrevenuehub/rev-engine into DEV-5295-5-project-proof-of-concept-of-uv-as-replacement-for-poetry-pyenv

benjaminEwhite · 2024-10-08T12:58:45Z

.github/workflows/test-python.yml

      - name: Run tests
        run: |
          source .venv/bin/activate
-          touch .env


This doesn't seem to have been needed.

benjaminEwhite · 2024-10-08T13:00:04Z

Dockerfile

    " \
-    && seq 1 8 | xargs -I{} mkdir -p /usr/share/man/man{} \


This step does not appear to be required for our setup.

…-replacement-for-poetry-pyenv

njharman · 2024-10-09T20:52:03Z

.envrc.example

@@ -23,3 +23,4 @@ export MAILCHIMP_CLIENT_SECRET=
 export REACT_APP_HUB_GOOGLE_MAPS_API_KEY=
 export REACT_APP_NRE_MAILCHIMP_CLIENT_ID=$MAILCHIMP_CLIENT_ID
 export RP_MAILCHIMP_LIST_CONFIGURATION_COMPLETE_TOPIC=rp-mailchimp-list-configuration-complete
+export VIRTUAL_ENV=".venv"


Maybe a comment as to what this is used by? [I don't see VIRTUAL_ENV referenced anywhere else]

njharman · 2024-10-09T20:52:47Z

README.md


 ##### Initial installation

+Ensure you have a system version of Python and uv installed.


nm, it's clear from general uv link above

Consider adding description or link to how to install uv

njharman · 2024-10-09T21:03:49Z

Makefile

@@ -1,4 +1,6 @@

+VENV_DIR="./.venv"


Prepending superfluous "./" is weird.

njharman · 2024-10-09T21:10:20Z

Makefile


 setup:
 	@echo 'Setting up the environment...'
+	@test -d $(VENV_DIR) || uv venv $(VENV_DIR) && echo "Virtual environment created at $(VENV_DIR)"
+	source $(VENV_DIR)/bin/activate


source is bash built in (for me under ubuntu) and make is using sh. Thus

source "./.venv"/bin/activate /bin/sh: 1: source: not found

Setting default shell wfm, copied from

https://stackoverflow.com/questions/7507810/how-to-source-a-script-in-a-makefile

Makefile default shell is /bin/sh which does not implement source.
Changing shell to /bin/bash makes it possible:

# Makefile SHELL := /bin/bash

njharman · 2024-10-09T21:13:00Z

README.md

 ```

-Adding or removing dependencies will automatically update the `pyproject.toml` file and the `poetry.lock` file.
+Note that unlike Poetry, uv does not automatically update the `pyproject.toml` file and `uv.lock` files after adding/removing dependencies. You'll want to manually add/remove from `pyproject.toml` and you'll want to run `uv lock` to update the lockfile.


I would cut "unlike Poetry, "

Also consider moving this to top of section, under ### How to... [reckon it's harder to miss there].

njharman · 2024-10-09T21:34:16Z

Makefile


 install_requirements:
 	@echo 'Installing project requirements...'
-	poetry install --no-root
+	uv sync --frozen

 setup:


I believe the method to "start work" is source $(VENV_DIR)/bin/activate. It would be nice to have make entry for this

activate:
source $(VENV_DIR)/bin/activate
python --version

I miss poetry shell (or rather vex) there's workarounds astral-sh/uv#1910

njharman · 2024-10-09T21:41:35Z

.github/workflows/test-python.yml

+
+      - name: Set up Python and dependencies
+        run: |
+          uv venv create --python=python3.10


Do we have "upgrade python docs"? If so should add mention to look here and in Dockerfile to update python versions.

njharman · 2024-10-09T21:43:34Z

.pre-commit-config.yaml

@@ -48,15 +48,31 @@ repos:
    hooks:
      - id: ruff
        args: [ --fix ]
+  - repo: https://github.com/astral-sh/uv-pre-commit
+    # uv version.
+    rev: 0.4.18


The installer gave me version 0.4.20.

Leo and I have this problem with ruff. They upgrade it so frequently, often occurs between dev and review.

njharman · 2024-10-09T21:45:49Z

Dockerfile

-# We need to recreate the /usr/share/man/man{1..8} directories first because
-# they were clobbered by a parent image.
+# Copy uv tool
+COPY --from=ghcr.io/astral-sh/uv:latest /uv /bin/uv


latest is probably easier than updating specific version all the time?

njharman · 2024-10-09T21:53:44Z

pyproject.toml

+    "responses>=0.25.3",
+    "ruff>=0.6.8",
+]
+
 # TODO @njh: reduce complexity to 10 or less


This comment should go with [tool.ruff.lint.mccabe] section

[tool.uv] was inserted in-between

benjaminEwhite · 2024-10-17T16:05:19Z

Closing this PR because it's served purpose as PoC and we've decided not to move forward with uv in revengine just yet.

first pass

d4055bb

x110dc temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 4, 2024 15:59 Inactive

update github action to use uv

1eb98fd

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 4, 2024 17:40 Inactive

remove errantly committed file

a62a919

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 4, 2024 20:51 Inactive

fix uv sync

668c309

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 4, 2024 21:12 Inactive

benjaminEwhite added 2 commits October 4, 2024 17:43

install dev deps

a6db76c

update readme.md

67455be

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 4, 2024 21:44 Inactive

try to fix pre-commit config

87645bc

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 4, 2024 21:50 Inactive

dry up github workflows

0d9637a

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 4, 2024 23:27 Inactive

fix workflow ref

bb74cc2

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 4, 2024 23:38 Inactive

try checking out

06621e0

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 4, 2024 23:44 Inactive

move to job from step

db0ce8e

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 4, 2024 23:55 Inactive

try removing touch .env

3f490ee

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 5, 2024 00:02 Inactive

remove from steps

af2f7da

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 5, 2024 00:05 Inactive

fix sec issue and us uv sync to create requirements.txt

c70280a

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 5, 2024 00:51 Inactive

try installing make

444fd2a

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 5, 2024 01:02 Inactive

log make location temporarily

121a8a2

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 7, 2024 21:14 Inactive

think this may work for test-python

a0ea6b3

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 7, 2024 21:29 Inactive

add correct option

764ebf2

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 7, 2024 21:32 Inactive

fix typo

94eea40

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 7, 2024 21:34 Inactive

try with virtual env again

32ad897

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 7, 2024 23:42 Inactive

benjaminEwhite added 2 commits October 7, 2024 20:18

pass frozen option

401f54a

add env var for VIRTUAL_ENV

20ee60d

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 8, 2024 00:39 Inactive

benjaminEwhite changed the title ~~DEV-5295 | First pass at using uv in revengine~~ DEV-5295 | Replace poetry with uv Oct 8, 2024

benjaminEwhite marked this pull request as ready for review October 8, 2024 00:42

benjaminEwhite requested a review from nrh-cklimas as a code owner October 8, 2024 00:42

benjaminEwhite commented Oct 8, 2024

View reviewed changes

.envrc.example Outdated Show resolved Hide resolved

Update .envrc.example

4422f6e

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 8, 2024 00:42 Inactive

benjaminEwhite added 2 commits October 8, 2024 08:00

cleanup dockerfile

26e448b

Merge branch 'DEV-5295-5-project-proof-of-concept-of-uv-as-replacemen…

f44add8

…t-for-poetry-pyenv' of github.com:newsrevenuehub/rev-engine into DEV-5295-5-project-proof-of-concept-of-uv-as-replacement-for-poetry-pyenv

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 8, 2024 12:00 Inactive

benjaminEwhite commented Oct 8, 2024

View reviewed changes

cleanup

96beef6

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 8, 2024 13:09 Inactive

Merge branch 'main' into DEV-5295-5-project-proof-of-concept-of-uv-as…

76a2640

…-replacement-for-poetry-pyenv

benjaminEwhite temporarily deployed to rev-engine-dev-5295-5-p-lwko47 October 8, 2024 17:26 Inactive

njharman requested changes Oct 9, 2024

View reviewed changes

benjaminEwhite closed this Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEV-5295 | Replace poetry with uv #1638

DEV-5295 | Replace poetry with uv #1638

benjaminEwhite commented Oct 4, 2024 •

edited

Loading

benjaminEwhite Oct 8, 2024

benjaminEwhite Oct 8, 2024

njharman Oct 9, 2024

njharman Oct 9, 2024

njharman Oct 9, 2024

njharman Oct 9, 2024

njharman Oct 9, 2024

njharman Oct 9, 2024

njharman Oct 9, 2024

njharman Oct 9, 2024

njharman Oct 9, 2024

njharman Oct 9, 2024

benjaminEwhite commented Oct 17, 2024


		##### Initial installation

		Ensure you have a system version of Python and uv installed.

DEV-5295 | Replace poetry with uv #1638

DEV-5295 | Replace poetry with uv #1638

Conversation

benjaminEwhite commented Oct 4, 2024 • edited Loading

Plan? (if this draft/incomplete indicate what you intend to do and how)

What's this PR do?

Why are we doing this? How does it help us?

Are there detailed, specific, step-by-step testing instructions in the associated ticket?

Did this PR make changes to the user interface that may require changes to the user-facing docs?

Have automated unit tests been added? If not, why?

How should this change be communicated to end users?

Are there any smells or added technical debt to note?

Has this been documented? If so, where?

What are the relevant tickets? Add a link to any relevant ones.

Do any changes need to be made before deployment to production (adding environment variables, for example)? If so, open a ticket and link it to the ticket for this PR and list it here:

Are there next steps? If so, what? Have tickets been opened for them? List next-step tickets here:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminEwhite commented Oct 17, 2024

benjaminEwhite commented Oct 4, 2024 •

edited

Loading