feat: modernize Python tooling (pyproject.toml + uv) - #1015
feat: modernize Python tooling (pyproject.toml + uv)#1015irfanuddinahmad wants to merge 13 commits into
Conversation
openedx-webhooks
added
the
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. |
openedx-webhooks
added
the
core contributor
Move isort/pytest settings from setup.cfg/pytest.ini into pyproject.toml. enterprise-access is a deployed Django service, not a published package, so no [build-system]/packaging metadata is added here. Also drop the now-dead [pytest] section from tox.ini (pytest.ini took precedence over it already). flake8 config from setup.cfg is dropped since flake8 isn't invoked anywhere in this repo's 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"}
(quality and doc compose {include-group = "test"} directly, mirroring
this repo's own requirements/quality.in and doc.in which built on top of
test.txt rather than base.txt). Wire the existing tox.ini up to
uv-venv-lock-runner with per-env dependency_groups, update the Makefile's
requirements targets to their uv equivalents, and switch CI to install uv
and run tests via `uv run tox`. Also add an uv install step to the
scheduled requirements-upgrade workflow so `make upgrade` keeps working.
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>
The comment said "Disable pylint import error because we don't install django-debug-toolbar for CI build" but the actual # pylint: disable pragma was never added. This was previously masked because the old CI ran the quality checks inside the broader dev-dependency environment (which does include django-debug-toolbar); the new tox quality env is correctly scoped to just the quality group, which doesn't include it, exposing the missing pragma. 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: `uv sync --group ci` previously failed trying to build mysqlclient "because enterprise-access:dev depends on mysqlclient"; with default-groups = [] it now installs only the ci group's own tools. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This migration's ci.yml is the first time pylint actually runs in CI for this repo -- the old ci.yml called `make style isort_check pii_check check_keywords` directly, bypassing tox.ini's pre-existing (but dormant) `quality` env entirely. Turning it on surfaced real, previously-invisible findings across the app: - Unused imports (customer_billing.py, test_api.py, test_checkout_intent_views.py, test_enrollment_deadline_strategies.py). - Deliberate lazy imports used to avoid circular imports (utils.py, enrollment_deadline_strategies.py) that were missing their pylint-disable annotations. - A redundant `pass` after a logged except block. - A missing docstring (stripe_event_handlers.py). - `staff_user = self.create_user(...)` in two tests: create_user() has no return value (it sets self.user as a side effect), so this always assigned None; set_jwt_cookie's user=None already falls back to self.user, so behavior is unchanged, but the dead assignment is removed. - A mutable default argument (`roles_and_contexts=[]`) in test_utils. - cyclic-import (R0401) between utils.py and enrollment_deadline_strategies.py: the two modules are legitimately mutually dependent and already broken via deferred imports, so disabled via pylintrc_tweaks like the other intentional suppressions already there. Verified: pylint/pycodestyle/isort all pass; ran the 4 touched test modules directly (136 passed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… gate Same root cause as the preceding pylint fix: pydocstyle runs immediately after pylint in tox.ini's quality env, which this migration is the first to actually wire into CI. ~40 pre-existing findings (missing docstrings, non-imperative wording, D205/D400/D415 summary-line formatting) across utils.py, tasks.py, cache_utils.py, celery.py, and __init__.py were never previously checked. Purely docstring wording/formatting -- no behavior changes. Verified: pydocstyle/pylint/pycodestyle/isort all pass; full test suite (1243 tests) passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The uv migration's unpinned "stripe" dependency resolved to the latest 15.3.0 at lock time, silently upgrading from the 14.1.0 that master's pip-compiled requirements/test.txt pins. stripe 15.x changed StripeObject's attribute/item access behavior in a way that breaks this app's `.get()` calls on nested event payloads -- confirmed via the actual CI logs for this PR's "tests" jobs (KeyError/AttributeError in stripe/_stripe_object.py), and reproduced/fixed locally: 51 failing tests across customer_billing and provisioning all pass again once stripe is constrained back to <15. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
f3fd404 to
de63148
Compare
|
Important Written with the assistance of Claude Code, human-reviewed before posting. Must Fix
We should decide on:
Informational@claude says following, please verify:
|
|
Re: the informational note about non-migration source changes — verified this in detail: This is isolated to enterprise-access; I checked all 10 repos in the modernization effort and none of the others have source-level lint fixes, only tooling/config changes. Root cause: enterprise-access's old CI called Verified each change individually (diffed against master) — all behavior-preserving. Happy to point to the exact commits ( |
|
tests are not running |
farhan
left a comment
farhan
left a comment
There was a problem hiding this comment.
Thanks for the detailed explanation on the source-level lint fixes — the justification makes sense given the previously unexercised CI gate.
The Must Fix items from the earlier review still need to be addressed. I am pinning them inline now.
One change request is related to a file not touched by this PR but I believe should cover within this PR:
Change request:
.coveragerc (not in diff): The file still exists on disk with its original [run] section. I think its contents (branch, data_file, source, omit) should be migrated to [tool.coverage.run] in pyproject.toml and the file deleted. Currently pyproject.toml has no [tool.coverage] section at all. For reference, this has already been done in xblocks-extra — it includes [tool.coverage.run], [tool.coverage.report], and [tool.coverage.html] sections.
|
|
||
| dev_requirements: ## sync to requirements for local development | ||
| pip-sync -q requirements/dev.txt | ||
| uv sync --group dev |
There was a problem hiding this comment.
The dev_requirements, validation_requirements, etc. correctly use uv sync, but test (line 60), isort_check (line 68), style (line 74), and lint (line 77) still call pytest, isort, pycodestyle, pylint directly without uv run. On a clean checkout where only uv is installed, these targets will fail. Prefix each bare invocation with uv run.
There was a problem hiding this comment.
Confirmed already fixed -- test, isort_check, style, lint all use uv run now.
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 | ||
| - name: setup python | ||
| uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 |
There was a problem hiding this comment.
actions/setup-python is redundant here — astral-sh/setup-uv (line 28) already handles Python installation when its python-version input is set. Remove actions/setup-python from both the tests and quality jobs.
There was a problem hiding this comment.
Agreed — will fix. setup-python is redundant here since astral-sh/setup-uv handles Python via its python-version input.
There was a problem hiding this comment.
Fixed — see farhan@2c32f2b (note: commits are on a fork branch since I lack push access to the openedx repo; irfanuddinahmad can cherry-pick or the branch can be merged from the fork).
There was a problem hiding this comment.
Confirmed already fixed (via a separate commit, 15fc242, pushed the same day) -- no actions/setup-python step remains in the tests job; astral-sh/setup-uv carries python-version and enable-cache: true.
| steps: | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| - name: setup python | ||
| uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 |
There was a problem hiding this comment.
Same as above: actions/setup-python is redundant in the quality job too. astral-sh/setup-uv at line 58 handles Python via its python-version input. Remove this step.
There was a problem hiding this comment.
Agreed — will fix, same as the tests job.
There was a problem hiding this comment.
Fixed in the same commit as the tests job: farhan@2c32f2b
There was a problem hiding this comment.
Confirmed already fixed (via a separate commit, 15fc242, pushed the same day) -- no actions/setup-python step remains in the quality job either; astral-sh/setup-uv carries python-version and enable-cache: true.
| matrix: | ||
| python-version: ["3.12"] | ||
| django-version: ["pinned", "5.2"] | ||
| toxenv: |
There was a problem hiding this comment.
🤖 Generated with Claude Code
These matrix values change the check names from the old tests (3.12, pinned) format to tests (py312-django42), tests (py312-django52), tests (translations). If tests (3.12, pinned) is currently a required status check in branch protection, all future PRs will be blocked after this merges. Please confirm with a repo admin that branch protection rules are updated (or were never set to the old name) before merging. The currently failing tests (3.12, pinned) check on this PR likely confirms it was a required check.
There was a problem hiding this comment.
perhaps tests (3.12, pinned) are not running in the PR because of it.
There was a problem hiding this comment.
Checked directly via the GitHub API (repos/.../branches/main/protection and .../rulesets) -- this repo has no branch protection rules or rulesets configured, so there's no required status check that could be broken by the renamed checks. Confirmed, not just assumed.
| @@ -0,0 +1,150 @@ | |||
| [project] | |||
| name = "enterprise-access" | |||
| version = "0.1.0" | |||
There was a problem hiding this comment.
Version mismatch: master's __version__ is 1.0.1 but pyproject.toml declares 0.1.0. Please change to version = "1.0.1" to stay consistent with the current release.
There was a problem hiding this comment.
Agreed — will fix. version = "0.1.0" was a placeholder; will update to "1.0.1" to match the current release.
There was a problem hiding this comment.
Fixed with farhan@03bc3b6 (same commit also migrates .coveragerc contents to [tool.coverage.run] in pyproject.toml and deletes the file).
There was a problem hiding this comment.
Confirmed fixed -- pyproject.toml now declares version = "1.0.1", matching master.
There was a problem hiding this comment.
__version__ = '1.0.1' still exists. With python-semantic-release owning the version going forward, this should be removed
There was a problem hiding this comment.
Agreed — will fix. __version__ will be removed from __init__.py.
There was a problem hiding this comment.
Confirmed fixed -- enterprise_access/__init__.py no longer has __version__.
There was a problem hiding this comment.
Line 9 (from enterprise_access import __version__ as code_version) is outside this diff hunk and wasn't updated. Once __version__ is removed from __init__.py, this import will break at runtime. Replace with importlib.metadata.version("enterprise-access") — either inline or at module level:
from importlib.metadata import version as _get_version
code_version = _get_version("enterprise-access")There was a problem hiding this comment.
Agreed — will fix. Since [tool.uv] package = false, this is not an installed package and importlib.metadata.version() would raise PackageNotFoundError at runtime. Will replace with a hardcoded code_version = '1.0.1' alongside a comment explaining why, consistent with how the version is now tracked in pyproject.toml.
There was a problem hiding this comment.
Fixed with farhan@10d8fe3 — replaced the import with a hardcoded code_version = '1.0.1' with a comment explaining why importlib.metadata is not viable here.
There was a problem hiding this comment.
Confirmed fixed -- replaced with a hardcoded code_version = '1.0.1' and an explanatory comment (since [tool.uv] package = false, importlib.metadata.version() isn't viable here as this isn't an installed package).
Addresses farhan's review on #1015: - Makefile: prefix uv run on all bare tool invocations (test, isort_check, style, lint, plus coverage and isort which have the same gap) -- these fail on a clean checkout where only uv is installed - ci.yml: remove actions/setup-python from both the tests and quality jobs (redundant once astral-sh/setup-uv gets python-version) and add enable-cache/python-version to setup-uv - enterprise_access/__init__.py: remove __version__ -- this repo is a deployed service (pyproject.toml's [tool.uv] package = false), not an installed package, and has no python-semantic-release/release.yml, so a hardcoded version string here was just stale (0.1.0 vs master's 1.0.1) - enterprise_access/cache_utils.py: replace the __version__ import (which would now break) with a hardcoded code_version constant -- importlib.metadata.version() doesn't work here since package = false means this project is never actually installed with dist-info metadata - pyproject.toml: sync version to 1.0.1 to match master's actual __version__; migrate .coveragerc into [tool.coverage.run] and delete the old file Branch protection concern (renamed CI check names) is moot: verified via gh api that openedx/enterprise-access's main branch has no branch protection rules or rulesets configured at all, so there's no required status check to break. Verified: pyproject.toml parses, both edited .py files compile, no other callers of the removed __version__ import (grepped the whole repo). Could not run the full tox matrix locally -- same mysqlclient build environment limitation already documented in the original PR.
|
Addressed the Must-Fix items (Makefile `uv run` prefixes, `.coveragerc` migration, version sync to 1.0.1, `version`/`cache_utils.py` removal) and the redundant `actions/setup-python` steps. On the branch protection concern: checked via `gh api repos/openedx/enterprise-access/branches/main/protection` and `.../rulesets` — both return empty/404. There's no branch protection or ruleset configured on `main` at all, so the renamed CI check names can't break anything here. One thing worth a second look: `cache_utils.py`'s `code_version` can't use `importlib.metadata.version()` since this repo's `[tool.uv] package = false` means it's never actually installed with dist-info metadata — went with a hardcoded string synced to the new `1.0.1`, with a comment explaining why (and that it needs manual bumping going forward, since there's no semantic-release here). |
|
Addressed all review comments. Since I do not have push access to the openedx repo, the fix commits are on a fork branch at https://github.com/farhan/enterprise-access/tree/irfanuddinahmad/uv-migration — @irfanuddinahmad please cherry-pick or apply the 4 commits from there. Summary of fixes:
The branch protection concern (check name change from |
| username='a-staff-user', password='password', is_staff=True, | ||
| ) | ||
| self.set_jwt_cookie([{ | ||
| 'system_wide_role': SYSTEM_ENTERPRISE_LEARNER_ROLE, |
There was a problem hiding this comment.
This is a behavior-preserving cleanup -- create_user() doesn't return a value (it sets self.user internally), so staff_user was always None; set_jwt_cookie(user=None) already falls back to self.user, which create_user() just set to the staff user. Dropping the dead staff_user = assignment and the redundant user=staff_user kwarg doesn't change behavior -- verified by tracing through test_utils/__init__.py.
| username='a-staff-user', password='password', is_staff=True, | ||
| ) | ||
| self.set_jwt_cookie([{ | ||
| 'system_wide_role': SYSTEM_ENTERPRISE_LEARNER_ROLE, |
There was a problem hiding this comment.
This is a behavior-preserving cleanup -- create_user() doesn't return a value (it sets self.user internally), so staff_user was always None; set_jwt_cookie(user=None) already falls back to self.user, which create_user() just set to the staff user. Dropping the dead staff_user = assignment and the redundant user=staff_user kwarg doesn't change behavior -- verified by tracing through test_utils/__init__.py.
| superfluous-parens, | ||
| too-many-positional-arguments, | ||
| cyclic-import, | ||
|
|
There was a problem hiding this comment.
cyclic-import is globally disabled here. The PR correctly adds # pylint: disable=import-outside-toplevel at the specific import sites, but suppressing cyclic-import at the config level hides future circular import regressions across the whole codebase. Can you explain why a global disable is needed rather than per-file or per-module suppressions?
There was a problem hiding this comment.
Fixed -- narrowed to per-file suppression at the actual import sites instead of a global disable, so this doesn't hide future cyclic-import regressions elsewhere in the codebase. Pylint's cyclic-import check excludes a module-to-module edge from its cycle graph when cyclic-import is disabled at the specific import statement's line, so # pylint: disable=cyclic-import at the three deferred-import sites in utils.py/enrollment_deadline_strategies.py is sufficient. Verified pylint enterprise_access test_utils manage.py (what CI's quality job runs) is clean with the global disable removed -- no other cyclic imports exist elsewhere in the codebase.
| useless-option-value, | ||
| superfluous-parens, | ||
| too-many-positional-arguments, | ||
| cyclic-import, |
There was a problem hiding this comment.
The line after this one (+ cyclic-import,) globally disables cyclic-import — same concern as the comment in pylintrc. Suppressing this at the config level hides future circular import regressions across the whole codebase.
There was a problem hiding this comment.
Fixed -- narrowed to per-file suppression at the actual import sites instead of a global disable, so this doesn't hide future cyclic-import regressions elsewhere in the codebase. Same fix as the pylintrc comment above.
| "pycodestyle", | ||
| "pydocstyle", | ||
| "twine", | ||
| ] |
There was a problem hiding this comment.
twine is a PyPI publishing tool. enterprise-access is a deployed Django service (package = false) that never publishes to PyPI. This looks like it was inherited from a shared template and is safe to remove.
There was a problem hiding this comment.
Fixed -- removed, confirmed no consumer in this repo (deployed service, package = false).
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 | ||
|
|
There was a problem hiding this comment.
astral-sh/setup-uv installs Python via its python-version input, making actions/setup-python (a few steps above) redundant in this workflow. Worth removing setup-python here unless a later step explicitly needs the Python it provisions.
There was a problem hiding this comment.
Fixed -- removed the redundant actions/setup-python step; astral-sh/setup-uv now provisions Python via its python-version input.
|
|
||
| - name: Codecov | ||
| if: startsWith(matrix.toxenv, 'py312-django') | ||
| run: uv run codecov |
There was a problem hiding this comment.
This condition does not pin to a specific python-version. If a second Python version is ever added to the matrix, Codecov uploads from both jobs for every Django variant. Suggest:
if: startsWith(matrix.toxenv, 'py312-django') && matrix.python-version == '3.12'There was a problem hiding this comment.
Fixed -- added python-version as an explicit matrix dimension and the condition now also checks matrix.python-version == '3.12', to avoid double-uploads if a second Python version is added to the matrix later.
enterprise-access is a deployed Django service (`[tool.uv] package = false`), never published to PyPI, so twine (a PyPI-publishing tool) has no consumer here -- it was inherited from a shared template. Removing it also drops its PyPI-publish-only transitive deps (rich, keyring, secretstorage, jeepney, jaraco-*, markdown-it-py, mdurl, more-itertools, id, pywin32-ctypes, requests-toolbelt, rfc3986) from uv.lock. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…workflow astral-sh/setup-uv already provisions Python via its python-version input, making the separate actions/setup-python step redundant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The condition only checked matrix.toxenv, with no python-version pin. If a second Python version is ever added to the tests matrix, Codecov would upload for every py312-django* variant across both Python versions. Add python-version as an explicit matrix dimension and gate the Codecov step on it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cyclic-import (R0401) was disabled globally in pylintrc/pylintrc_tweaks, which would silently hide any future circular-import regression anywhere in the codebase -- not just the one legitimate case. The only real cycle is between enterprise_access/utils.py and enterprise_access/apps/content_assignments/enrollment_deadline_strategies.py, both already broken via deferred (function-local) imports. Pylint's cyclic-import check excludes a module-to-module edge from its cycle graph if cyclic-import is disabled at the specific import statement's line (see pylint's ImportsChecker._add_imported_module), so adding `# pylint: disable=cyclic-import` at the three deferred import sites that form this cycle is sufficient to suppress it there without touching the global config. Regenerated pylintrc from pylintrc_tweaks via `edx_lint write pylintrc`. Verified: with the global disable removed and these three per-line disables in place, `pylint enterprise_access test_utils manage.py` (what CI's quality job actually runs) is clean -- no other cyclic imports exist in the codebase that the global disable was masking. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tox.ini's django42/django52 factors both synced the same `test` dependency group (locked against the loose base "Django>=2.2" pin, which resolves to 5.2.x) and then force-overrode just the Django package version via `deps=`. Every other transitively-resolved package -- django-crispy-forms, django-filter, social-auth-app-django, social-auth-core, etc. -- stayed pinned to whatever uv resolved for Django 5.2, even when running "against Django 4.2". This silently masked real Django-4.2-incompatible dependency versions instead of actually testing against them (same bug as enterprise-subsidy#441). Fix, mirroring that PR's approach: - Split `test` into a Django-agnostic `test-base` plus two real groups that each add their own Django pin: `test` (>=5.2,<6.0, the current default) and a new `django42` (>=4.2,<5.0). - Declare `django42` as conflicting with `test`/`quality`/`doc`/`dev` in [tool.uv].conflicts so uv.lock can carry both resolutions at once without them clobbering each other. - Point tox.ini's django42 factor at the new `django42` group instead of `test`, and drop the `deps=` overrides entirely. Verified: `uv lock` now resolves genuinely different versions for django-crispy-forms (2.5 vs 2.6), django-filter (25.1 vs 25.2), and social-auth-app-django/social-auth-core between the two groups. Manually synced each group and confirmed `import django; django.VERSION` reports (4, 2, 30, ...) under `django42` and (5, 2, 16, ...) under `test`, and ran the full test suite under both (1243 passed each). Confirmed uv rejects syncing `test` and `django42` together, per the conflicts declaration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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. 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 thattest/quality/doc/production/devall compose via{include-group = "base"}.tox.ini/Makefileto usetox-uv'suv-venv-lock-runner, and switch CI (includingupgrade-python-requirements.yml, which callsmake upgradedirectly rather than the shared reusable workflow) to install uv and run viauv sync/uv run tox.Part of openedx/public-engineering#513.
Test plan
uv lockresolves cleanly (204 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