feat: modernize Python tooling (pyproject.toml + uv) - #441
feat: modernize Python tooling (pyproject.toml + uv)#441irfanuddinahmad wants to merge 11 commits into
Conversation
openedx-webhooks
added
open-source-contribution
|
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. |
farhan
left a comment
farhan
left a comment
There was a problem hiding this comment.
Tests are failing.
Inline comments and following points needs to be addressed:
1. .coveragerc is still on disk — coverage config not migrated
.coveragerc was not deleted and its contents were not migrated to pyproject.toml. Per the modernization spec, coverage config belongs in [tool.coverage.run] / [tool.coverage.report] / [tool.coverage.html] in pyproject.toml.
pyproject.toml has no [tool.coverage] section at all.
Fix:
- Add the coverage config to
pyproject.toml:[tool.coverage.run] branch = True source_pkgs = ["enterprise_subsidy"] omit = ["*/migrations/*", "*/tests/*"] [tool.coverage.report] # (migrate any existing [report] settings from .coveragerc)
- Delete
.coveragerc.
2. __version__ still present in enterprise_subsidy/__init__.py
__version__ = '1.0.1'|
|
||
| - name: Build package | ||
| run: python setup.py sdist bdist_wheel | ||
|
|
There was a problem hiding this comment.
The pip install -r requirements/pip.txt step was removed but python setup.py sdist bdist_wheel remains — setup.py doesn't exist in this repo, so this workflow was already broken before the PR. Partially modifying a broken workflow makes the state worse, not better. The PR description says the intent was to delete it. Please either delete pypi-publish.yml entirely, or leave it fully untouched — the current half-fix shouldn't merge.
There was a problem hiding this comment.
Done — pypi-publish.yml was deleted entirely in this PR. The file no longer exists on the branch.
There was a problem hiding this comment.
Confirmed — pypi-publish.yml was deleted entirely, file no longer exists on this branch.
| - name: setup python | ||
| uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 | ||
| uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 | ||
| with: |
There was a problem hiding this comment.
Once python-version is passed to astral-sh/setup-uv (see the comment on that step), this actions/setup-python step can be removed — setup-uv provisions Python automatically.
There was a problem hiding this comment.
Done — actions/setup-python was removed. astral-sh/setup-uv now has both python-version and enable-cache: true, so it provisions Python directly.
There was a problem hiding this comment.
Confirmed — actions/setup-python was removed; astral-sh/setup-uv now carries both python-version and enable-cache: true.
| 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.
The setup-uv action has no with: block. Add enable-cache: true to avoid re-resolving dependencies on every CI run. Also add python-version: "${{ matrix.python-version }}" — this is required before actions/setup-python can be removed (see the comment on that step), since setup-uv will then be the only thing pinning the Python version.
with:
enable-cache: true
python-version: "${{ matrix.python-version }}"There was a problem hiding this comment.
This is resolved by the same fix as the setup-python comment above — there's only one setup-uv step in ci.yml now, and it already has both enable-cache: true and python-version set.
….yml) Addresses farhan's review on #441: - Delete .github/workflows/pypi-publish.yml entirely. It called `python setup.py sdist bdist_wheel` (setup.py doesn't exist in this repo) and used token-based auth instead of OIDC -- a stale, already-broken leftover from before this migration. This repo is a deployed service (pyproject.toml's [tool.uv] package = false, no [build-system], no semantic-release), so it was never meant to publish to PyPI at all; the previous partial edit (removing one step but leaving the broken build step) made a dead workflow look alive without fixing it. - pyproject.toml: sync version to 1.0.1 to match master's actual __version__ (this repo has no semantic-release, so this stays a manually-maintained string -- unlike enterprise-access, __version__ itself isn't being removed here, it's still used by apps/core/utils.py's cache-key versioning) - Migrate .coveragerc into [tool.coverage.*] and delete the old file -- also fixes a real pre-existing bug in the process: the old config's `source=enterprise-subsidy` used the hyphenated PyPI project name instead of the actual importable package `enterprise_subsidy` - ci.yml: add enable-cache/python-version to astral-sh/setup-uv and drop the now-redundant actions/setup-python step The currently-failing "tests (3.12, django52)" check farhan flagged is not a real test failure -- verified via the job logs: pytest itself reports "349 passed", and the failure is downstream in the Codecov CLI upload step (GPG signature verification error against Codecov's own infrastructure), unrelated to this repo's code. Verified: pyproject.toml parses, uv sync succeeds with only the expected version-bump lockfile change. Could not run the full tox matrix locally -- same mysqlclient build environment limitation hit on other repos in this batch.
|
Addressed:
On "tests are failing": checked the job logs directly — pytest itself reports `349 passed`. The actual failure is downstream, in the Codecov CLI's own GPG signature verification step (`Can't check signature: No public key`), which is an external Codecov infra issue, not a real test failure in this repo. |
Move isort/pytest settings from setup.cfg/pytest.ini into pyproject.toml. enterprise-subsidy is a deployed Django service, not a published package, so no [build-system]/packaging metadata is added here. Dropped the unused flake8 section from setup.cfg (flake8 was not invoked anywhere in the Makefile or CI). 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. Since this repo has no [project]
packaging metadata, runtime deps live in an explicit `base` group that
test/quality/doc/production/dev all compose via {include-group = "base"}
(directly or transitively through `test`). Update tox.ini to use
tox-uv's uv-venv-lock-runner and map each testenv to a dependency group,
update the Makefile, and switch CI to install uv and run tests via
`uv run tox`. Also dropped the now-dangling `requirements/pip.txt`
install step from pypi-publish.yml, which references a nonexistent
setup.py and appears to already be dead/unused in this repo.
Note: could not fully verify `uv sync`/`uv run tox` locally on this
machine (no libmysqlclient/pkg-config for building mysqlclient from
source) -- `uv lock` resolves cleanly; full sync needs verifying via CI.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s/doc.txt Read the Docs isn't natively uv-aware, so drive the build with a custom build.commands override instead of python.install's requirements: key. Also bumps the RTD Python version to 3.12 to match the rest of this migration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The original requirements/validation.in composed quality.txt + test.txt
(quality is a superset of test in this repo's dependency-groups). The
"test" group has none of isort/pycodestyle/pydocstyle/twine, silently
dropping quality tooling from this target and the ci_requirements
target that depends on it, contradicting its own doc-comment ("sync to
requirements for testing & code quality checking").
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 (and mysqlclient) into every target. Verified with `uv sync --group ci`. Also adds .venv to .gitignore alongside the existing venv/ entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
….yml) Addresses farhan's review on #441: - Delete .github/workflows/pypi-publish.yml entirely. It called `python setup.py sdist bdist_wheel` (setup.py doesn't exist in this repo) and used token-based auth instead of OIDC -- a stale, already-broken leftover from before this migration. This repo is a deployed service (pyproject.toml's [tool.uv] package = false, no [build-system], no semantic-release), so it was never meant to publish to PyPI at all; the previous partial edit (removing one step but leaving the broken build step) made a dead workflow look alive without fixing it. - pyproject.toml: sync version to 1.0.1 to match master's actual __version__ (this repo has no semantic-release, so this stays a manually-maintained string -- unlike enterprise-access, __version__ itself isn't being removed here, it's still used by apps/core/utils.py's cache-key versioning) - Migrate .coveragerc into [tool.coverage.*] and delete the old file -- also fixes a real pre-existing bug in the process: the old config's `source=enterprise-subsidy` used the hyphenated PyPI project name instead of the actual importable package `enterprise_subsidy` - ci.yml: add enable-cache/python-version to astral-sh/setup-uv and drop the now-redundant actions/setup-python step The currently-failing "tests (3.12, django52)" check farhan flagged is not a real test failure -- verified via the job logs: pytest itself reports "349 passed", and the failure is downstream in the Codecov CLI upload step (GPG signature verification error against Codecov's own infrastructure), unrelated to this repo's code. Verified: pyproject.toml parses, uv sync succeeds with only the expected version-bump lockfile change. Could not run the full tox matrix locally -- same mysqlclient build environment limitation hit on other repos in this batch.
c78a1b2 to
f998903
Compare
| @@ -1,5 +1,7 @@ | |||
| [tox] | |||
There was a problem hiding this comment.
Both django42 and django52 factors use dependency_groups = test (same lockfile group) then pin Django via deps =. With uv-venv-lock-runner, the lockfile has a single Django resolution; deps = overrides it at tox time, producing an environment that may differ from what the lockfile guarantees.
The standard pattern for multi-Django testing with uv is separate django42/django52 dependency groups in pyproject.toml with [tool.uv] conflicts, giving each its own lockfile resolution. Is the current hybrid approach intentional?
There was a problem hiding this comment.
Good catch, this was a real gap — fixed by adding a proper django42 dependency-group (separate from the default test group's Django 5.2 pin) with [tool.uv].conflicts declaring them mutually exclusive, so uv.lock now has two independent, correct resolutions instead of a deps= override layered on top of the wrong lockfile group. Matches the pattern used in sibling repos in this same migration effort.
| # ourselves via a custom command (RTD's documented escape hatch for tools | ||
| # it doesn't integrate with directly). | ||
| build: | ||
| os: ubuntu-24.04 |
There was a problem hiding this comment.
Fixed — updated to ubuntu-26.04 (verified via RTD's current schema this is newer than ubuntu-24.04).
| "isort", | ||
| "pycodestyle", | ||
| "pydocstyle", | ||
| "twine", |
There was a problem hiding this comment.
twine is a PyPI publish tool and has no consumer here — pypi-publish.yml was correctly deleted and this repo is a non-PyPI service. Remove it from both the quality and doc groups.
There was a problem hiding this comment.
Fixed — removed twine from both quality and doc groups, confirmed it has no consumer in this non-PyPI service repo.
The django42/django52 tox factors both synced the same `test` group (which resolves against Django 5.2) and only force-overrode the Django package version at tox-run time via deps=. Every other package in the django42 env was still resolved/locked against a Django-5.2-compatible dependency graph, so uv.lock didn't actually guarantee a real Django 4.2 resolution. - Add a `django42` dependency-group (factored out via a shared `test-base` group) with its own Django>=4.2,<4.3 pin, and give the default `test` group an explicit Django>=5.2,<6.0 pin. - Declare `test`/`quality`/`doc`/`dev` as conflicting with `django42` in [tool.uv].conflicts so uv.lock carries two independent, correct resolutions instead of one. - Update tox.ini to map django42 -> django42 group (django52 -> test, unchanged) and remove the deps= override entirely. - Regenerate uv.lock. Verified locally (Ubuntu container, since mysqlclient needs Linux headers) that both py312-django42 and py312-django52 tox envs pass their full test suites with the correct Django version genuinely installed (4.2.30 and 5.2.16 respectively), including divergent transitive versions (e.g. django-filter, social-auth-app-django) that the old deps= override was silently masking. Also: - .readthedocs.yaml: bump build OS from ubuntu-24.04 to ubuntu-26.04. - pyproject.toml: remove twine from the quality and doc groups - this repo is a deployed service (tool.uv.package = false, no pypi-publish workflow) with no other twine consumer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tox.ini's envlist already had py312-django{42,52}, but ci.yml's matrix only
ran toxenv "django52" - django42 had no CI-enforced regression protection at
all, exactly the "newly-added tox env not wired into CI" anti-pattern from
the modernize-python-tooling playbook. Add "django42" to the matrix so both
Django versions are actually exercised in CI. Codecov upload stays gated on
django52 only, to avoid double-counting coverage across the two variants.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Bonus fix (not one of the original review comments): while verifying the |
Conflicts, both resolved the same way as the analogous ones on the other repos in this same migration effort: - .github/workflows/ci.yml: kept our astral-sh/setup-uv (no actions/setup-python needed), adopted main's actions/checkout bump to v7.0.1. - .github/workflows/pypi-publish.yml: modify/delete -- kept deleted. This repo is a deployed service, not a PyPI library; the file was already broken pre-migration and correctly deleted in an earlier commit on this branch. Main's change was only a routine action-version bump on that same already-dead file. Note: the django42/django52 fork-resolution fix I was about to make here turned out to already be on this branch (9f9acba, 1e88f01) -- my local clone was stale. Reset to match the remote before this merge rather than layering a redundant, less-thorough duplicate on top.
Inline tox.ini's [testenv:quality] commands via uv run/uv sync instead of shelling out to `uv run tox -e quality`, matching the no-tox-in- Makefile convention already used elsewhere (edx-enterprise, enterprise-access, openedx-ledger). tox.ini itself is untouched; CI's own matrix testing still uses tox directly.
Flagged by farhan a month ago, missed in the first pass. Matches the pattern already approved on enterprise-access: __version__ was redundant with pyproject.toml's version now that this is a deployed service (package=false), but it was actually load-bearing (cache-key stamping), so moved the hardcoded value to where it's consumed instead of just deleting it.
4 participants
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, replacingsetup.cfg/pytest.ini. This is a deployed Django service, not a published package, so[project]is minimal (name/version/requires-pythononly, no PyPI packaging metadata) and no semantic-release is added.pip-compiletouv:requirements/*.in+*.txtare replaced by PEP 735[dependency-groups]+ a singleuv.lock. Since there's no[project.dependencies]list, runtime deps live in an explicitbasegroup that other groups compose via{include-group = "base"}.tox.ini/Makefileto usetox-uv'suv-venv-lock-runner, switch CI to install uv and run viauv run tox, and fix.readthedocs.yaml, which pointed at the now-deletedrequirements/doc.txt.pypi-publish.ymlthat referenced a nonexistentsetup.py/requirements/pip.txt-- this repo isn't actually published to PyPI.Part of openedx/public-engineering#513.
Test plan
uv lockresolves cleanly (182 packages)uv 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.🤖 Generated with Claude Code