Switch API package management from Pipenv to PDM #4107

dhruvkb · 2024-04-14T17:34:19Z

Description

This PR replaces Pipenv with PDM. While PDM has monorepo support, we have opted not to use this feature in this PR. Instead we will use a path dependency to depend on subpackages located at packages/ in the monorepo. To see this in action, the license and attribution related functionality in the API has been separated into its own package at packages/ov-attribution.

The ov-attribution subpackage is listed as a dependency as well as an editable dev-dependency of the API. This means that in dev mode, the package will not actually be copied into the virtualenv but rather only point to ../packages/ov-attribution. When installed with --no-editable it will be copied into the virtualenv. Thanks @sarayourfriend for suggesting this and making usage of PDM possible.

Testing Instructions

Install PDM. I recommend using the pipx approach, I am too.
Build the new Docker image with just dc build web.
Explore the editable local package by changing something in py-packages/openverse-attribution to see the change immediately in the API.
Run the tests. (Or don't, CI has you covered.)
- Test openverse-attribution with:
```
cd py-packages/openverse-attribution && pdm install && pdm run pytest`
```
- Test the API with just api/test.

Checklist

My pull request has a descriptive title (not a vague title likeUpdate index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.
I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

github-actions · 2024-04-14T18:33:04Z

Full-stack documentation: https://docs.openverse.org/_preview/4107

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

New files ➕:

https://docs.openverse.org/_preview/4107/packages/openverse_attribution/index.html

Changed files 🔄:

https://docs.openverse.org/_preview/4107/packages/index.html

AetherUnbound · 2024-04-15T23:17:04Z

I'm testing this out locally and tried the instructions - I made some changes to packages/ov-attribution/ov_attribution/attribution.py and the API did not reload 🤔 In fact, it took rebuilding the web image to get the changes to appear. Is that intended?

dhruvkb · 2024-04-16T04:45:29Z

It should have reflected immediately because the ov-attribution package is mounted into the API image during development and changes are synced with the container. I suspect it's because the Django API is not reloading when the package changes because it's not watching the packages/ directory. Should be fixed now.

sarayourfriend · 2024-04-18T05:37:21Z

Maybe py_packages and js_packages

Works for me!

sarayourfriend

This is so exciting! Really looking forward to a world without pipenv, however we get there.

.github/renovate.json

AetherUnbound · 2024-04-19T00:40:49Z

@dhruvkb should this be drafted while we review #4128? I'm a bit confused at which I should look at first.

dhruvkb · 2024-04-19T05:13:47Z

@AetherUnbound I undrafted it because I felt quite confident that PDM would be our pick and also so I could also receive feedback on the actual implementation of the migration like that from @sarayourfriend regarding using an src/ layout and naming the dirs py-packages.

Now that #4128 is merged, feel free to review this PR as deeply as you like. Thanks!

Also opened a discussion-type issue #4165 so that we can finalise what to name and where to place our Python packages directory.

AetherUnbound

This is great! I have a few comments/questions, and per the discussion in #4165, I think we should have packages/python/ instead of py-packages/ at the top level (to mirror what we have in automation/).

AetherUnbound · 2024-04-19T22:04:33Z

api/Dockerfile

+# Copy subpackages from additional build-context 'packages'
+COPY --from=packages openverse-attribution ./py-packages/openverse-attribution


I wonder here if there's a way for us to "wildcard" this 🤔 As in, not have to be explicit about every package.

It is possible to make this a wildcard, I only didn't do that because we have only one package right now (and that too, is more of a PoC than a real library worth distributing).

Best not to wildcard it either, if we can avoid it; it would be nice for it to be "automatic" if new packages are added (I'm sure they will be, eh?) but we can look into ways to derive it from the pyproject.toml rather than a catch-all wild card. There might eventually be dev dependencies in the packages, or packages only used by catalog and ingestion worker (for example) that we would not want copied into the context of the API container (and vice-versa re ingestion workers).

api/pyproject.toml

AetherUnbound · 2024-04-19T22:21:08Z

api/pyproject.toml

+dev = [
+  "ipython >=8.22.1, <9",
+  "pgcli >=3.5.0, <4",
+  "remote-pdb >=2.1.0, <3",
+]
+test = [


Is there a functional difference between dev and test here, or is the separation only semantic?

This allows us to selectively build smaller and smaller images from dev (all deps) to CI (main + test) to prod (main only).

Gotcha! I hadn't seen the test install happening anywhere, so I wasn't sure.

py-packages/openverse-attribution/src/openverse_attribution/attribution.py

obulat

Awesome! Can't wait to see the PRs that Renovate will open.

I tried changing the attribution package code, and it immediately showed up in the API response.

My request for change is to add the testing of the openverse-attribution to the CI tests.

dhruvkb · 2024-04-22T05:20:49Z

@obulat to prevent the PR from going even beyond the already changed 35 files, I've made a separate issue to cover CI changes #4167.

obulat

Thank you for all the work on selecting the package manager and implementing the changes, @dhruvkb . Makes sense to split the changes into several PRs.
Can we create a milestone for all the pdm-related accompanying issues?

dhruvkb · 2024-04-22T09:35:17Z

@obulat good call, done.

sarayourfriend · 2024-04-22T09:54:22Z

I think we should have packages/python/ instead of py-packages/ at the top level (to mirror what we have in automation/).

I love this, Madison! 💯

dhruvkb · 2024-04-22T16:46:08Z

@sarayourfriend I'll update #4165 to indicate that general support for this naming scheme.

Also I'm preparing for Python packages to go in packages/python by moving the existing JS package one level deeper in #4174.

krysal

This is exciting! The transition looks smoother than I anticipated ✨ I haven't reviewed all the PR, but I noticed some things I want to comment on.

Also, I updated the testing instructions since they mentioned an abbreviated name for the new Python package and were missing an installation step.

krysal · 2024-04-22T17:21:14Z

py-packages/openverse-attribution/README.md

+
+This library is a part of the Openverse project. For more information, refer to
+the
+[documentation](https://docs.openverse.org/packages/ov_attribution/index.html).


Suggested change

[documentation](https://docs.openverse.org/packages/ov_attribution/index.html).

[documentation](https://docs.openverse.org/packages/openverse_attribution/index.html).

krysal · 2024-04-22T17:46:15Z

api/justfile

+# These commands use standard GNU tools instead of `pdm` because we do not
+# require PDM on the host machine for API development.


This conflicts with the testing instructions for the openverse-attribution package. Maybe there are alternative instructions using docker containers?

This statement still holds. If one is working on the API (and even if they are making changes to the attribution package), they do not need PDM on the host (because the package will be mounted into the API container directly and be usable via the API).

PDM is only needed on the host if one is purely working outside the context of the API, like the attribution package directly.

krysal · 2024-04-22T17:56:54Z

api/pyproject.toml

@@ -0,0 +1,65 @@
+[project]
+name = "api"


Nit: more specific would be...

Suggested change

name = "api"

name = "openverse-api"

or

Suggested change

name = "api"

name = "openverse_api"

api/Dockerfile

sarayourfriend · 2024-04-22T23:45:00Z

Install PDM. I recommend using the pipx approach, I am too.

FWIW, this isn't the official recommendation from PDM, and I think I've run into weird virtual environment issues sometimes when using the pipx approach that I never had happen when using the project's own recommendation: https://pdm-project.org/en/latest/#recommended-installation-method

If you're not having issues, no worried, but if anyone does run into things, I was able to get everything working without a hitch by avoiding pipx for this. PDM's installer isolates itself and pdm is self-updating, so pipx's specific benefits are elusive (other than being consistent for one's own personal preferences, which I appreciate can matter very much).

sarayourfriend

This LGMT, Dhruv, keeping in mind my opinion that implementation details of the openverse-attribution package are out of scope for this PR and should be pushed to a separate issue dedicated that package.

I'm approving this personally now despite the py-packages directory, because I don't think it's strictly necessary to block this PR on the location of those files, and it'll be trivial to move them after the related JS package PR is merged.

One thing that could be nice is to have a fast-follow to implement a just recipe like the p recipe, that runs pdm for all Python packages, optionally following a pattern. If we don't use pdm workspaces, I suppose we can just use find to identify pyproject.toml files and use PDM's -P flag to point to them and run the command in each one. To that end, we should have consistent script names as well (cf. test:unit, types for the JS packages).

That, to me, is a nice to have in a follow up issue though, and not necessary for this first PR, which is already interesting enough as it is. I agree with many of the things others have brought up as potential avenues to explore in follow up issues, but do not believe any to be blockers for this specific PR, which we'll want to get out and able to build upon for the rest of the -> PDM migration. Feel free to correct me @krysal and @AetherUnbound, just leave a request changes, but in the spirit of expedient decision making, it'd be great to be able to move forward with this without accidentally expanding the scope, especially for anything that don't have lasting harmful implications for the project or its team members (cf. the definition of "blocker" in our decision making process.

Not trying to override anyone else's intuition, just want to encourage making sure we carefully evaluate and respect a defined scope for the PR.

sarayourfriend · 2024-04-22T23:27:23Z

api/Dockerfile

+# Copy subpackages from additional build-context 'packages'
+COPY --from=packages openverse-attribution ./py-packages/openverse-attribution


Best not to wildcard it either, if we can avoid it; it would be nice for it to be "automatic" if new packages are added (I'm sure they will be, eh?) but we can look into ways to derive it from the pyproject.toml rather than a catch-all wild card. There might eventually be dev dependencies in the packages, or packages only used by catalog and ingestion worker (for example) that we would not want copied into the context of the API container (and vice-versa re ingestion workers).

sarayourfriend · 2024-04-22T23:29:33Z

api/Dockerfile

@@ -96,6 +98,7 @@ RUN useradd --create-home opener \
 USER opener

 # Copy code into the final image
+COPY --chown=opener --from=packages openverse-attribution /py-packages/openverse-attribution/


This shouldn't be necessary, I thought, because it should be copied into the .venv for production (non-editable). Unless this is explicitly to support the development-mode of this target? I wish it were possible to conditionally copy, if that is the reason.

Bit of a nit, though, we can iterate on this without issue.

Edit (after reading the compose file again): Even in development mode, it shouldn't be necessary, because we map the python packages directory in the compose file, so the editable path reference should still be valid.

sarayourfriend · 2024-04-22T23:30:31Z

api/api/models/media.py

@@ -8,12 +8,12 @@
 from django.utils.html import format_html

 from elasticsearch import Elasticsearch, NotFoundError
+from openverse_attribution.attribution import get_attribution_text


I know we've gone back and forth on the name, but it ocurred to me this doesn't even need openverse specified in the package name. Nothing about it is actually Openverse specific. It is, however, CC-specific! cc_attribution would be a good name for it.

Just a nit though, don't bother spending time on this, we can bikeshed the name if we ever get around to publishing it.

api/pyproject.toml

sarayourfriend · 2024-04-22T23:36:41Z

documentation/packages/openverse_attribution/index.md

@@ -0,0 +1,79 @@
+# `openverse-attribution`


Actually, occurring to me that it'd be important for package README's to be in the package itself, maybe just copied to documentation during the build step, otherwise it isn't available for inclusion in the build output of the package. npm and pypi require a README (twine check will fail on build outputs that don't have it, and you won't be allowed to upload it).

Not an issue for this PR, but something we will need to address in the future when we publish packages from the packages directories.

py-packages/openverse-attribution/src/openverse_attribution/attribution.py

api/Dockerfile

…w_pkg_man

dhruvkb · 2024-04-23T04:51:44Z

Since these changes are working and the PR's core ideas have 2 approvals, I will merge this PR. Not dismissing any of the comments from the reviews, I'll make separate issues for each of them and add them to the milestone.

openverse-bot added 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work 🏷 status: label work required Needs proper labelling before it can be worked on labels Apr 14, 2024

dhruvkb changed the title ~~Switch to Poetry from PIpenv~~ Switch to Poetry from Pipenv Apr 14, 2024

dhruvkb mentioned this pull request Apr 14, 2024

Implementation Plan: Switch Python package management away from Pipenv #286

Closed

1 task

dhruvkb mentioned this pull request Apr 16, 2024

Add implementation plan for new Python package manager PDM #4128

Merged

8 tasks

dhruvkb added 3 commits April 17, 2024 16:20

Limit scope of Node subpackages

60add84

Fix licenses in API tests

393795a

Create PDM package for managing licenses and attribution

6eb956d

dhruvkb force-pushed the new_pkg_man branch from cc9b915 to e10ea98 Compare April 17, 2024 13:39

dhruvkb changed the title ~~Switch to Poetry from Pipenv~~ Switch API package management from Pipenv to PDM Apr 17, 2024

dhruvkb force-pushed the new_pkg_man branch from e10ea98 to 05d32e0 Compare April 17, 2024 14:00

Switch API to PDM with dependency on ov-attribution

01f4c42

Update documentation

f9fe06e

sarayourfriend reviewed Apr 18, 2024

View reviewed changes

.github/renovate.json Outdated Show resolved Hide resolved

dhruvkb added 4 commits April 18, 2024 10:07

Use hyphens in py-packages

6fbcb9e

Add new root directory to CODEOWNERS

7d16563

Comment why the _VERSION exports do not use pdm

6f44cdd

Fix content_hash

e780e2d

dhruvkb mentioned this pull request Apr 19, 2024

Convert usages of Pipenv to PDM #4166

Open

14 tasks

Update references to Pipfile in comments

6e47eca

This was referenced Apr 19, 2024

Run CI tests for Python sub-packages #4167

Closed

Move JS packages to packages/js to match automations layout #4165

Closed

AetherUnbound reviewed Apr 19, 2024

View reviewed changes

obulat requested changes Apr 22, 2024

View reviewed changes

obulat approved these changes Apr 22, 2024

View reviewed changes

krysal reviewed Apr 22, 2024

View reviewed changes

sarayourfriend approved these changes Apr 22, 2024

View reviewed changes

dhruvkb added 3 commits April 23, 2024 07:48

Merge branch 'main' of https://github.com/WordPress/openverse into ne…

755022a

…w_pkg_man

Move Python package into packages/python

5f7fc94

Specify default value for build arg differently

e3e8d96

dhruvkb merged commit 7dac3ad into main Apr 23, 2024
42 checks passed

dhruvkb deleted the new_pkg_man branch April 23, 2024 04:52

dhruvkb mentioned this pull request Apr 23, 2024

Refine openverse-attribution as a standalone pacakge #4182

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch API package management from Pipenv to PDM #4107

Switch API package management from Pipenv to PDM #4107

dhruvkb commented Apr 14, 2024 •

edited by krysal

github-actions bot commented Apr 14, 2024 •

edited

AetherUnbound commented Apr 15, 2024

dhruvkb commented Apr 16, 2024 •

edited

sarayourfriend commented Apr 18, 2024

sarayourfriend left a comment

AetherUnbound commented Apr 19, 2024 •

edited

dhruvkb commented Apr 19, 2024 •

edited

AetherUnbound left a comment

AetherUnbound Apr 19, 2024

dhruvkb Apr 20, 2024

sarayourfriend Apr 22, 2024

AetherUnbound Apr 19, 2024

dhruvkb Apr 20, 2024

AetherUnbound Apr 22, 2024

obulat left a comment

dhruvkb commented Apr 22, 2024 •

edited

obulat left a comment

dhruvkb commented Apr 22, 2024

sarayourfriend commented Apr 22, 2024

dhruvkb commented Apr 22, 2024 •

edited

krysal left a comment •

edited

krysal Apr 22, 2024

krysal Apr 22, 2024

dhruvkb Apr 23, 2024

krysal Apr 22, 2024

sarayourfriend commented Apr 22, 2024 •

edited

sarayourfriend left a comment

sarayourfriend Apr 22, 2024

sarayourfriend Apr 22, 2024

sarayourfriend Apr 22, 2024

sarayourfriend Apr 22, 2024

dhruvkb commented Apr 23, 2024 •

edited

		# Copy subpackages from additional build-context 'packages'
		COPY --from=packages openverse-attribution ./py-packages/openverse-attribution

	[documentation](https://docs.openverse.org/packages/ov_attribution/index.html).
	[documentation](https://docs.openverse.org/packages/openverse_attribution/index.html).

		# These commands use standard GNU tools instead of `pdm` because we do not
		# require PDM on the host machine for API development.

Switch API package management from Pipenv to PDM #4107

Switch API package management from Pipenv to PDM #4107

Conversation

dhruvkb commented Apr 14, 2024 • edited by krysal

Related

Description

Testing Instructions

Checklist

Developer Certificate of Origin

github-actions bot commented Apr 14, 2024 • edited

AetherUnbound commented Apr 15, 2024

dhruvkb commented Apr 16, 2024 • edited

sarayourfriend commented Apr 18, 2024

sarayourfriend left a comment

Choose a reason for hiding this comment

AetherUnbound commented Apr 19, 2024 • edited

dhruvkb commented Apr 19, 2024 • edited

AetherUnbound left a 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

obulat left a comment

Choose a reason for hiding this comment

dhruvkb commented Apr 22, 2024 • edited

obulat left a comment

Choose a reason for hiding this comment

dhruvkb commented Apr 22, 2024

sarayourfriend commented Apr 22, 2024

dhruvkb commented Apr 22, 2024 • edited

krysal left a comment • edited

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

sarayourfriend commented Apr 22, 2024 • edited

sarayourfriend left a 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

dhruvkb commented Apr 23, 2024 • edited

dhruvkb commented Apr 14, 2024 •

edited by krysal

github-actions bot commented Apr 14, 2024 •

edited

dhruvkb commented Apr 16, 2024 •

edited

AetherUnbound commented Apr 19, 2024 •

edited

dhruvkb commented Apr 19, 2024 •

edited

dhruvkb commented Apr 22, 2024 •

edited

dhruvkb commented Apr 22, 2024 •

edited

krysal left a comment •

edited

sarayourfriend commented Apr 22, 2024 •

edited

dhruvkb commented Apr 23, 2024 •

edited