feat: modernize Python tooling (pyproject.toml + uv) - #1136
feat: modernize Python tooling (pyproject.toml + uv)#1136irfanuddinahmad wants to merge 13 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. |
Move isort/pytest settings from setup.cfg into pyproject.toml. enterprise-catalog is a deployed Django service, not a published package, so no [build-system]/packaging metadata is added here. The [flake8] section in setup.cfg is dropped since flake8 isn't invoked anywhere in this repo (Makefile/tox.ini/CI); pycodestyle config moves to tox.ini in the next commit alongside the dependency-groups migration. 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"}.
A dedicated `ci` group holds tox/tox-uv, composed into `dev`. Update the
existing tox.ini to use the uv-venv-lock-runner, update the Makefile, and
switch the MySQL8 migrations-check workflow to install uv and run
`uv sync`/`uv run`.
The main Django CI workflow (ci.yml) runs tests inside the
`edxops/enterprise-catalog-dev` devstack image via validate.sh -> `make
requirements && make validate`; since that image is built in a separate
repo, it isn't touched here, but it will need `uv` preinstalled before
this workflow will pass again. Also dropped requirements/optional.txt
and requirements/monitoring/, which were unused by any Makefile target
or workflow, and the pip<25.3 constraint, which was a pip-tools/pip
build-compatibility pin with no bearing under uv.
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>
CI runs this script inside the edxops/enterprise-catalog-dev Docker image, which doesn't have uv preinstalled -- "make: uv: No such file or directory". Installing it here unblocks CI without needing to update that image separately, though updating the image directly (so this doesn't need to pip install uv on every run) would still be worth doing separately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t dirs
pylint scans the whole enterprise_catalog tree, which includes per-app
tests/ subdirectories that import ddt/pytest -- caught proactively after
finding the same gap in license-manager's quality group (CI failure
there: "Unable to import 'ddt'"). Compose {include-group = "test"}
instead of just "base".
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
quality already gets edx-lint transitively via {include-group = "test"},
which lists it directly.
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. 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>
70602cd to
64f835f
Compare
Same category of fixes farhan flagged on the batch's other reviewed PRs, adapted to this repo's shape (deployed service, package = false, CI runs tests inside Docker via validate.sh rather than directly via tox): - Makefile: extend the existing $(TOX) variable (uv run tox --, already used by the `test` target, active whenever TOXENV is set -- which it is in the real CI path via `docker exec -e TOXENV=...`) to the coverage, isort_check, isort, style, lint, pii_check, and html_coverage targets, which previously called their tools completely unwrapped. Verified with TOXENV set locally that these now correctly resolve to `uv run tox -- <tool>` matching how CI actually invokes them. - mysql-migrations-check.yml: add python-version to astral-sh/setup-uv (already had enable-cache) and drop the now-redundant actions/setup-python step. ci.yml itself has no setup-uv step at all (tests run inside a Docker container via validate.sh), so it's untouched. - Migrate .coveragerc into [tool.coverage.run] and delete the old file package = false and no CHANGELOG.rst already correctly reflect this being a deployed service, not a published package -- no changes needed there. Verified: pyproject.toml parses, Makefile dry-run with TOXENV set confirms the wrapped commands resolve correctly. Could not run the full suite locally -- same mysqlclient build environment limitation as other repos in this batch (and this repo's tests only really run via the Docker-based validate.sh path in CI anyway).
The previous commit's Makefile fix (wrapping lint with $(TOX)) made pylint actually run inside the correctly-scoped, tox-managed quality-group environment for the first time via this exact path -- surfacing a real pre-existing gap: enterprise_catalog/urls.py imports debug_toolbar without a pylint-disable guard, and django-debug-toolbar is only in the dev dependency group, not quality (it's a local-dev-only package, correctly excluded from CI's quality group). Same pattern already fixed on enterprise-access earlier in this batch. Confirmed via the actual CI failure log: E0401 Unable to import 'debug_toolbar' (import-error) at urls.py:65.
…rgets
My earlier fix wrapping isort_check/isort/style/lint/pii_check/
html_coverage with \$(TOX) (uv run tox --) was wrong: tox.ini's only
testenv here is `commands = {posargs:pytest}`, scoped to the `test`
dependency group (django52: test). Routing quality tools through it
via posargs made them run inside that test-scoped venv instead of the
dev/quality-scoped one they actually need -- and surfaced a spurious
E5110 django-not-configured pylint error that doesn't happen when
these tools run in their correct environment (confirmed via the actual
CI failure log: this exact error only appeared once lint started
routing through tox's test env).
`test` and `coverage` correctly keep \$(TOX) -- they genuinely need
pytest run against the tox-selected Django version. The other targets
now use a plain `uv run <tool>`, matching the venv that
`make requirements`/`uv sync --group dev` already provisions with
quality tools (pyproject.toml's dev group includes the quality group),
which is what CI's validate.sh actually does before calling `make
validate` -> quality -> lint.
…rom test/coverage) Cross-checked against review comments/fixes from openedx-ledger#242, edx-enterprise-subsidy-client#222, enterprise-access#1015, and enterprise-subsidy#441. - Add the [tool.coverage.report]/[html] sections that were missing (only [tool.coverage.run] existed). - Change the test/coverage Makefile targets to use a plain `uv run` prefix instead of the conditional $(TOX) variable, matching the quality/lint targets fixed in a previous pass; drop the now-unused TOX variable and ifdef block. - Wrap the bare `coverage erase` in the clean target with `uv run`.
Make requires recipe lines to be indented with a literal tab character. An earlier edit in this migration accidentally used spaces for the test and coverage targets, breaking 'make test' and CI.
| # addition* to whatever --group is passed, silently pulling the entire | ||
| # dev/test/quality/ci superset (and mysqlclient) into every target, defeating | ||
| # the point of having separate groups. | ||
| default-groups = [] |
There was a problem hiding this comment.
is this empty group needed?
There was a problem hiding this comment.
This isn't an empty dependency group — it's the [tool.uv] default-groups = [] setting (not a [dependency-groups] entry), and it's intentional.
Every uv sync/uv run invocation in this repo names an explicit --group. Without this setting, uv's implicit default group (named dev) would get synced in addition to whatever --group is passed, silently pulling in the whole dev/test/quality/ci superset (plus heavier deps) on every target — which would defeat the purpose of having separate groups in the first place.
There's a comment directly above the line in the file explaining this — happy to expand it further if it'd help clarify for future readers.
There was a problem hiding this comment.
Checked this — the repo's own pycodestyle/flake8 config is already at 120, so bumping isort's line_length to match is reasonable and matches the request. Heads up on scope though: running isort with the new setting reformats the actual import wrapping across ~90 files (mostly collapsing multi-line imports that now fit under 120 chars), not just the one config line. Want me to go ahead with the full reformat, or would you rather keep it scoped to this migration PR being config-only and handle the reformat separately?
There was a problem hiding this comment.
@irfanuddinahmad thanks for this. Yes, I think that is a good idea to handle the changes separately.
|
|
||
| [tool.isort] | ||
| indent = ' ' | ||
| line_length = 80 |
There was a problem hiding this comment.
What would you think of changing this threshold to 120? I understand that we use 120 limit based on other repos.
|
Thanks @irfanuddinahmad, let me know if you have time to resolve the conflicts to proceed with the merge, if not, I can resolve them. |
- mysql-migrations-check.yml: replaced uv pip uninstall/install --no-binary (x2) with a single native `uv sync --group production --group mysql8 --no-binary-package mysqlclient --no-binary-package xmlsec`. mysqlclient is already a real project dependency (installs via `production`); xmlsec wasn't declared anywhere, so added a dedicated `mysql8` group for it -- same as master's pre-migration behavior, where it came in only via this same job's own pip uninstall+reinstall step. Verified both genuinely trigger source builds (--no-cache, to bypass any stale cached wheel). - tox.ini: dropped the `deps = django52: Django>=5.2,<5.3` line. Verified it was fully redundant, not the recurring shared-group+deps= bug: this repo's tox only has one Django factor (django52), and the `test` group it maps to already resolves to Django 5.2.16 by default -- the deps= override never changed anything, since there's no second factor whose divergence it could be silently masking. Found while auditing this repo for uv pip usage per the lessons learned on openedx-platform#38915.
- mysql-migrations-check.yml: kept our astral-sh/setup-uv (no actions/setup-python needed), adopted master's actions/checkout bump to v7.0.1. Same conflict pattern as the other repos in this migration effort. - ci.yml, catalog-info.yaml: auto-merged cleanly, no conflict.
5 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. This is a deployed Django service, not a published package, so no[project]/packaging metadata or semantic-release is added.pip-compiletouv:requirements/*.in+*.txtare replaced by PEP 735[dependency-groups]+ a singleuv.lock. Since there's no[project]table, runtime deps live in an explicitbasegroup that other groups compose via{include-group = "base"}.tox.ini/Makefileto usetox-uv'suv-venv-lock-runner, and switchmysql-migrations-check.ymlto install uv and run viauv sync/uv run tox.Heads up: this repo's
ci.ymlruns tests inside a prebuiltedxops/enterprise-catalog-devDocker image (viavalidate.sh→make requirements && make validate), built in a separate repo. That image will needuvinstalled before this branch's CI can pass -- I didn't have access to fix that from here.Part of openedx/public-engineering#513.
Test plan
uv lockresolves cleanly (193 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, along with the Docker image update noted above.🤖 Generated with Claude Code