feat: modernize Python tooling (pyproject.toml + uv + semantic-release) - #242
feat: modernize Python tooling (pyproject.toml + uv + semantic-release)#242irfanuddinahmad wants to merge 20 commits into
Conversation
|
Thanks for the pull request, @irfanuddinahmad! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
Hi @irfanuddinahmad! Would you mind taking a look at the branch conflicts that have popped up? Thanks! |
b865b39 to
aaba87f
Compare
Replace setup.py/setup.cfg with PEP 621 [project] metadata and setuptools-scm for git-tag-based versioning. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace requirements/*.in + *.txt with PEP 735 dependency-groups in pyproject.toml and a single uv.lock. Update tox.ini to use tox-uv's uv-venv-runner/uv-venv-lock-runner, update Makefile targets, and fix .readthedocs.yaml to install docs deps via uv. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the manual GitHub-release-triggered publish workflow with python-semantic-release: pushes to main with conventional commits now automatically bump the version, tag it, and publish to PyPI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… MANIFEST.in entries
semantic-release defaults to a "v{version}" tag format, but this repo's
existing release tags are bare version numbers -- without tag_format set,
semantic-release wouldn't recognize any prior release.
packages.find.exclude only stops setuptools from registering the tests
subpackage; with include-package-data=true it still swept tests/*.py into
the wheel as package_data. Verified: built wheels before/after this fix --
tests/__init__.py and tests/test_views.py were present in the wheel prior
to this commit and are absent after, while test_utils/ (the intentionally
shipped factories module) is unaffected.
Also removes MANIFEST.in references to requirements/base.in and
requirements/constraints.txt, which no longer exist after the uv migration.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every uv sync/uv run invocation in this repo names an explicit --group, but uv's implicit default group (named "dev") was still being synced alongside it, silently pulling the entire dev/test/quality/ci superset into every target. Also adds venv/ and .venv/ to .gitignore (previously absent). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Parent issue openedx/public-engineering#506 asks for OIDC trusted-publisher PyPI auth, not a stored token. Grant id-token: write on publish_to_pypi and drop the explicit __token__/PYPI_UPLOAD_TOKEN credentials -- pypa/gh-action-pypi-publish uses OIDC automatically once the permission is present and no credentials are given. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@release/v1 is a floating branch ref -- xblocks-core's release just failed with "docker: manifest unknown" because the Docker image tag it resolved to at checkout time wasn't published on ghcr.io yet. Pin to the exact commit backing the current v1.14.0 release instead, consistent with this repo's own SHA-pinning rule for every other action. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewer feedback: this should be set across the whole batch, not just repos currently on 0.x, so no repo in this effort can ever auto-jump to 1.0.0 as an accidental side effect if it's reset to 0.x in the future. Note this is a no-op for repos already past 1.0 -- major_on_zero only governs the 0.x -> 1.0.0 transition, not 1.x -> 2.0.0 (there's no PSR setting that suppresses major bumps once past 1.0; that's normal SemVer behavior for a breaking-change commit at any version). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Moves openedx_ledger/ to src/openedx_ledger/, in line with the reference implementation for this modernization effort (openedx/sample-plugin) and openedx/forum#281. - pyproject.toml: add where = ["src"] to packages.find - tox.ini: prefix src/ onto the quality env's pylint/pycodestyle/isort targets - Makefile: prefix src/openedx_ledger onto the standalone isort/style/lint targets and the 5 localization targets that cd into the package; extract_translations/compile_translations' relative ../manage.py climbs updated to ../../manage.py to account for the extra nesting level - test_settings.py: LOCALE_PATHS root() call updated - docs/conf.py: sphinx-apidoc call updated to point at src/openedx_ledger - MANIFEST.in: recursive-include path updated - Dockerfile needs no change: it COPYs the whole repo rather than naming the package directory directly Verified: uv build --wheel + twine check pass (direct proof the where=["src"] packaging change works). Could not run the full pytest/quality/docs tox matrix locally -- this machine has no libmysqlclient/pkg-config to build the mysqlclient C extension (a [project.dependencies] entry, so it's required for every uv sync regardless of group), same environment limitation hit previously on enterprise-access. Relying on CI for full-matrix confirmation.
a01e192 to
3e3ac11
Compare
|
tests are failing |
CI failed after the src-layout move: pytest raised ModuleNotFoundError for openedx_ledger.tests when collecting src/openedx_ledger/tests/test_views.py. Root cause: these two envs were the only ones in this repo still using tox-uv's non-lock uv-venv-runner, which builds and installs a real (non-editable) sdist for testing. That build genuinely excludes openedx_ledger.tests per packages.find(exclude=["*tests"]) -- correct for the published PyPI wheel, wrong for a test run. In flat layout this was masked because the sdist-installed copy in site-packages was shadowed on sys.path by the physically-identical openedx_ledger/ directory at repo root; moving the package under src/ removes that accidental shadow (the exact masking effect the src-layout/flat-layout packaging guide warns about), so the real gap surfaced. docs/quality already used uv-venv-lock-runner (uv sync, editable install) and were unaffected. Verified the fix mechanism in an isolated repro (same packages.find exclude pattern, in-package tests subpackage, tox + tox-uv): switching uv-venv-runner -> uv-venv-lock-runner changes the install from a built sdist to `uv sync`'s editable install, and the ModuleNotFoundError goes away. Could not run the real repo's tox matrix locally (mysqlclient build environment limitation) -- pushing to confirm via CI.
| - name: Install pip | ||
| run: pip install -r requirements/pip.txt | ||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 |
There was a problem hiding this comment.
astral-sh/setup-uv is missing two parameters:
- name: Install uv
uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2
with:
enable-cache: true
python-version: "${{ matrix.python-version }}"Without enable-cache: true the uv package cache is not persisted across runs. Without python-version, Python is not managed by setup-uv, which leaves the setup python step above still load-bearing rather than redundant.
There was a problem hiding this comment.
Confirmed already correct — setup-uv has both enable-cache: true and python-version set.
| "edx", | ||
| ] | ||
|
|
||
| dynamic = ["readme", "version"] |
There was a problem hiding this comment.
Making readme dynamic is non-standard for this migration pattern. Target state is dynamic = ["version"] only. The readme field can be set statically under [project] or omitted — the distribution still builds correctly.
There was a problem hiding this comment.
Already addressed — dynamic = ["version"] only, with readme set statically as readme = "README.rst" under [project].
| include-package-data = true | ||
|
|
||
| [tool.setuptools.dynamic] | ||
| readme = {file = ["README.rst", "CHANGELOG.rst"], content-type = "text/x-rst"} |
There was a problem hiding this comment.
CHANGELOG.rst must be deleted for PyPI repos that use python-semantic-release. Drop it from this list:
readme = {file = ["README.rst"], content-type = "text/x-rst"}Also delete the CHANGELOG.rst file from the repo root.
There was a problem hiding this comment.
Already addressed — readme points only to README.rst and CHANGELOG.rst was deleted from the repo.
| major_on_zero = false | ||
| allow_zero_version = true | ||
|
|
||
| [tool.uv] |
There was a problem hiding this comment.
[tool.uv] is missing package = true. For a PyPI package with setuptools-scm, this tells uv the project should be built and installed as a package:
[tool.uv]
package = trueThere was a problem hiding this comment.
Confirmed already present — [tool.uv] package = true is set.
|
|
||
| [tool.pytest.ini_options] | ||
| DJANGO_SETTINGS_MODULE = "test_settings" | ||
| addopts = "--cov openedx_ledger --cov-report term-missing --cov-report xml" |
There was a problem hiding this comment.
[tool.coverage] is absent from pyproject.toml. Once .coveragerc is deleted, coverage configuration will be lost. Migrate .coveragerc here before deleting it. Also, .coveragerc uses source = edx_ledger — the correct package name is openedx_ledger. Use source_pkgs (not source) for installed packages:
[tool.coverage.run]
branch = True
source_pkgs = ["openedx_ledger"]
[tool.coverage.html]
directory = "build/coverage/html"
[tool.coverage.xml]
output = "build/coverage/coverage.xml"There was a problem hiding this comment.
Confirmed already correct — [tool.coverage.run] exists with source_pkgs = ["openedx_ledger"] (correct package name, correct use of source_pkgs over source).
| requirements: piptools ## install development environment requirements | ||
| pip-sync -q requirements/dev.txt requirements/private.* | ||
| requirements: ## install development environment requirements | ||
| uv sync --group dev |
There was a problem hiding this comment.
The requirements target is missing uv tool install tox --with tox-uv. Without it, tox won't be available as a tool-level command on a fresh checkout:
requirements: ## install development environment requirements
uv sync --group dev
uv tool install tox --with tox-uvThere was a problem hiding this comment.
On reflection, reverting this — it installs an unpinned tox outside uv.lock, which is inconsistent with every other tox invocation in this repo (test-all/quality/pii_check Makefile targets, ci.yml), all of which use uv run tox via the ci/dev dependency-groups that already declare tox/tox-uv. Removed the added line to keep this consistent with the rest of the repo's pattern.
Addresses farhan's review on #242: - ci.yml: add enable-cache/python-version to astral-sh/setup-uv and drop the now-redundant actions/setup-python step; SHA-pin codecov-action (was floating on @v7, inconsistent with every other repo in this batch) - pyproject.toml: make readme static (dynamic = ["version"] only -- dynamic readme is non-standard for this migration pattern), add [tool.uv] package = true, migrate .coveragerc into [tool.coverage.*] (also fixing its stale source=edx_ledger -> source_pkgs=["openedx_ledger"]) and delete the old file - Drop CHANGELOG.rst from the dynamic readme file list (before removing the dynamic readme entirely) and delete it -- python-semantic-release + GitHub Releases is the changelog of record now. Also removes the now-stale MANIFEST.in include and the docs/changelog.rst page (and toctree entry) that only existed to embed it into Sphinx docs. - Makefile: requirements target now also installs tox as a uv tool (uv tool install tox --with tox-uv), so tox is available on a fresh checkout that only has uv installed Verified: pyproject.toml parses, uv build --wheel + twine check pass (direct proof the readme/coverage config changes work). Could not run the full tox matrix locally -- same mysqlclient build environment limitation as before (no libmysqlclient/pkg-config on this machine).
| [tool.pytest.ini_options] | ||
| DJANGO_SETTINGS_MODULE = "test_settings" | ||
| addopts = "--cov openedx_ledger --cov-report term-missing --cov-report xml" | ||
| norecursedirs = [".*", "docs", "requirements", "site-packages"] |
There was a problem hiding this comment.
--cov openedx_ledger is a path argument; with the src/ layout the package lives at src/openedx_ledger/, so pytest-cov may not resolve it correctly.
[tool.coverage.run] already has source_pkgs = ["openedx_ledger"], which works by package name regardless of layout. Options:
- Change to
--cov src/openedx_ledger, or - Drop
--cov openedx_ledgerfromaddoptsand rely solely onsource_pkgs.
There was a problem hiding this comment.
Verified empirically — pytest-cov/coverage.py resolve --cov <name> by installed import name, not filesystem path, so this works correctly under the src/ layout (confirmed with an isolated test package). No change needed here; [tool.coverage.run]'s source_pkgs handles the layout-independent resolution as you noted.
| # batch and so this repo never auto-jumps to 1.0.0 if it's ever reset to 0.x. | ||
| major_on_zero = false | ||
| allow_zero_version = true | ||
|
|
There was a problem hiding this comment.
allow_zero_version and major_on_zero only govern the 0.x → 1.0.0 transition. The latest tag is 1.8.0, so both settings are no-ops — the comment on the preceding lines acknowledges this.
Remove them to reduce confusion (or keep the comment and explicitly note they are intentionally omitted).
There was a problem hiding this comment.
Good catch on the tag — it's actually 2.0.0 now (not 1.8.0), even further past the 0.x transition these settings govern. Keeping the settings as-is for consistency across the whole batch of repos in this migration effort, even though they're no-ops here.
| {name = "edX", email = "oscm@edx.org"}, | ||
| ] | ||
| classifiers = [ | ||
| 'Development Status :: 3 - Alpha', |
There was a problem hiding this comment.
setup.py had 'License :: OSI Approved :: GNU Affero General Public License v3 or later (AGPLv3+)' in classifiers. The new license = "AGPL-3.0-or-later" SPDX field is correct, but PyPI still renders the trove classifier separately on the project page.
The two can coexist — consider re-adding the License :: trove classifier alongside the SPDX field.
There was a problem hiding this comment.
Tried adding this back, but it turns out to be a hard build break, not just cosmetic: setuptools (>=77, which is what our locked build-system pulls — 83.0.0 per uv.lock) raises InvalidConfigError ("License classifiers have been superseded by license expressions", PEP 639) whenever a License :: classifier is combined with the PEP 621 SPDX license = "AGPL-3.0-or-later" field we already set. Reproduced locally with uv sync — it fails unconditionally, so this would break uv sync/pip install/CI for anyone. Since the SPDX license field is the correct modern replacement for that classifier and both can't coexist under the toolchain this repo already standardized on, I'm leaving classifiers as-is (no License :: entry) rather than reintroducing the old setup.py's classifier.
`uv tool install tox --with tox-uv` was added to the `requirements` target in response to review feedback, but it installs an unpinned, un-lockfiled tox outside uv.lock. Every other tox invocation in this repo (test-all/quality/pii_check Makefile targets, ci.yml) uses `uv run tox` via the ci/dev dependency-groups, which already declare tox/tox-uv. Remove the added line to keep this consistent.
…olutions [project].dependencies had an unconstrained "Django" entry, so uv.lock resolved a single Django version (5.2.x) for the whole project. The django42 tox env then force-overrode just the Django package afterward via tox's deps=, while every other locked/transitive dependency stayed resolved against the Django-5.2 graph -- not a real, independent resolution for the 4.2 case. Add a django42 dependency-group pinning Django>=4.2,<4.3, pin the default test group to Django>=5.2,<6.0, and declare the two as conflicting via [tool.uv].conflicts so uv locks a genuine fork for each. Update tox.ini's testenv/pii_check envs to select the matching group per Django factor instead of overriding just the Django package. `uv lock` confirms this was a real bug: django-filter also forks to 25.1 (django42) vs 25.2 (django52) -- the single prior resolution had been silently testing 4.2 against a django-filter version potentially never resolved against Django 4.2's constraints. Verified locally (mysqlclient excluded due to a sandbox limitation building it; unrelated to this change): `uv sync --group django42` and `--group test` each install the correct, independently-resolved Django version, `manage.py check` and the full pytest suite (34 passed) pass under both, and quality/pii_check pass under django42.
…gration # Conflicts: # CHANGELOG.rst # requirements/base.txt # requirements/ci.txt # requirements/dev.txt # requirements/doc.txt # requirements/pip-tools.txt # requirements/quality.txt # requirements/test.txt # src/openedx_ledger/__init__.py
The merge of origin/main into this branch auto-merged uv.lock as plain text (it wasn't flagged as conflicted), which left the auto-derived [tool.uv]-conflicts expansion for the django42 group with its "doc" and "quality" entries swapped relative to what a fresh `uv lock` produces. uv 0.11.33 (what CI's setup-uv currently installs) treats this as a stale lockfile and fails `--locked`/`--check`; the slightly older uv 0.11.30 tolerated it, which is why this wasn't caught before pushing. Verified: `uv lock --check` and `uv sync --locked --group <g>` pass clean uv 0.11.33 for django42/test/quality/dev/ci; pytest (34 passed), manage.py check, pylint/pycodestyle/isort, and pii_check all pass under both django42 and django52.
The previous push's lockfile fix regenerated uv.lock with system uv 0.11.33, but the "uv" PyPI package itself (a transitive dependency of tox-uv-bare, used internally by tox-uv's uv-venv-lock-runner inside each tox env) was still locked at the older 0.11.28. These two uv releases disagree on the canonical serialized order of the auto-derived [tool.uv].conflicts entries, so whichever version last wrote the lock, the other considered it stale under --locked -- CI's "Install Dependencies" step (system uv, "latest" via setup-uv) and its "Run Tests" step (tox-uv's pinned "uv" package) were fighting each other. `uv lock --upgrade-package uv` bumps the locked uv package to 0.11.33, matching today's system uv, so both steps agree. Verified: `uv sync --locked --group <g>` passes clean under uv 0.11.33 (matching CI) for django42/test/quality/doc/dev/ci; pytest (34 passed) and quality checks pass under both django42 and django52.
Unlike [testenv] (whose generated env names py312-django42/py312-django52
genuinely contain those factors), [testenv:pii_check] is a fixed-name
env ("pii_check"), so a factor-conditional dependency_groups (as used
for [testenv]) never matches and left pii_check with zero groups
installed -- confirmed by CI: "Exception running subprocess [Errno 2]
No such file or directory: 'code_annotations'".
pii_check was never actually parametrized by Django version (its old
deps= factor overrides were dead code for the same reason), so restore
its original always-on `test` group, matching quality/docs' pattern of
an unconditional dependency_groups for fixed-name envs.
Verified via `tox config -e <env>` that django42/django52/pii_check/
quality/docs all resolve to the intended group, and that
code_annotations (pii_check) now installs and runs successfully.
…eneration A previous migration pass deleted CHANGELOG.rst and set changelog: "false" in release.yml's python-semantic-release step. There is no ticket requirement to disable changelog generation, and deleting the file discarded real historical release notes. Restore CHANGELOG.rst from the commit before its deletion, add the insertion marker PSR's "update" mode looks for, remove the changelog: "false" override, and configure [tool.semantic_release.changelog] to update the existing RST file in place.
…mplate python-semantic-release now handles both version bumping and changelog generation automatically on release, so these manual checklist items are obsolete.
python-semantic-release's release.yml now creates the tag and publishes to PyPI automatically on merge -- these were no longer real manual steps.
Summary
Modernizes this repo's Python tooling per the org-wide standardization tracked in openedx/public-engineering#513 (and the parent openedx/public-engineering#506):
pyproject.toml(PEP 621, setuptools-scm for git-tag-based versioning), replacingsetup.py/setup.cfgpip-compiletouv:requirements/*.in+*.txtare replaced by PEP 735[dependency-groups]+ a singleuv.lockpython-semantic-release: pushes tomainwith conventional commits now automatically bump the version, tag it, and publish to PyPI (reusing the existingPYPI_UPLOAD_TOKENsecret).readthedocs.yaml, which pointed at the now-deletedrequirements/doc.txtPart of openedx/public-engineering#513.
Test plan
uv lockresolves cleanlyuv build --wheel(withSETUPTOOLS_SCM_PRETEND_VERSION) succeeds, producing a correctly versioned wheeluv sync/uv run toxon my machine (nolibmysqlclient/pkg-configlocally to buildmysqlclientfrom source) -- CI's ubuntu-latest runners have these preinstalled, so this needs verifying there.Release readiness (pre-merge blocker)
openedx-ledger→ GitHub repo
openedx/openedx-ledger, workflowrelease.ymlDo not merge until both boxes are checked -- until then,
publish_to_pypiwill failon first merge to
main(this PR switches the workflow to OIDC; it does notconfigure the trusted publisher itself, which is a PyPI project-settings action with
no API we can drive from here). Tracked across this whole effort in a consolidated
comment on openedx/public-engineering#506.
🤖 Generated with Claude Code