Repository navigation
feat: modernize Python tooling (pyproject.toml + uv + semantic-release) - #2654
irfanuddinahmad wants to merge 46 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2654 +/- ##
==========================================
+ Coverage 87.06% 87.62% +0.56%
==========================================
Files 265 280 +15
Lines 17280 19580 +2300
Branches 1709 1958 +249
==========================================
+ Hits 15044 17157 +2113
- Misses 1898 2038 +140
- Partials 338 385 +47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Three related CI failures found after opening this PR:
- pyproject.toml: fallback_version was "0.0.0.dev0" -- data.EventData.sourcelib
parses __version__ via tuple(map(int, __version__.split("."))), and "dev0"
isn't a valid int, so any environment without visible tags crashed 17 tests
with ValueError. Changed to "0.0.0" (still a placeholder, but int-parseable).
- ci.yml: actions/checkout defaults to a shallow, tag-less clone, so
setuptools-scm could never see real tags in the tests job and always fell
back regardless of the fallback_version fix above. Added fetch-depth: 0.
(release.yml's checkout doesn't need this -- python-semantic-release's own
action auto-deepens a shallow clone before evaluating version history.)
- .readthedocs.yaml: still pointed at requirements/doc.txt, deleted in the
prior commit. Switched to RTD's custom-build-command escape hatch (uv sync
+ sphinx-build), matching the pattern already used in openedx/edx-enterprise#2654.
Verified by simulating CI's actual shallow/no-tags checkout locally
(git clone --depth 1 --no-tags) and confirming all 109 tests pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
8891b80 to
cad21e1
Compare
Replace setup.py/setup.cfg with PEP 621 [project] metadata and setuptools-scm for git-tag-based versioning. The `braze` extras_require becomes a real [project.optional-dependencies] extra. enterprise/__init__.py's __version__ (used at runtime by cache_utils.versioned_cache_key) now derives from importlib.metadata instead of a hand-maintained string. 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-lock-runner, update the Makefile and Dockerfile, and switch ci.yml/mysql8-migrations.yml/postgresql-migrations.yml to install uv and run via `uv run tox` / `uv sync`. The openedx-platform pin-matching mechanism (requirements/edx-platform-constraints.txt + check_pins.py, refreshed via curl in `make upgrade`) is replaced by requirements/sync_platform_constraints.py, which merges edx-platform's compiled pins directly into [tool.uv].constraint-dependencies (run after `edx_lint write_uv_constraints`, which owns/overwrites that same list, and before `uv lock --upgrade`). pyyaml is excluded from that merge: the js_test group's jasmine dependency hard-pins pyyaml far below what edx-platform pins, so js_test is declared a conflicting resolution fork via [tool.uv].conflicts rather than forced into one unresolvable graph. Verified locally: `uv lock` resolves cleanly (253 packages), and `uv sync --group test --no-default-groups` succeeds without needing any native build (no mysqlclient dependency in this repo). Running the actual test suite hits a local-machine-only lxml/xmlsec libxml2 version mismatch unrelated to this migration (the Dockerfile's apt-installed toolchain avoids this; CI should verify cleanly). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the manual GitHub-release-triggered publish workflow with python-semantic-release: pushes to master with conventional commits now automatically bump the version, tag it, and publish to PyPI. The old publish.yml also ran `make pull_translations` before building -- that Transifex pull is dropped here since automating it would need new release-pipeline secrets; translations continue to be refreshed via the existing separate mechanisms (pull_translations/push_translations Makefile targets), just no longer implicitly re-run on every release. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…raint sync CI surfaced two real bugs from the initial migration: 1. The `pylint<4` constraint under-specified a floor, so uv's fresh resolution (unconstrained by the old pip-compile lock's specific pins) picked ancient pylint==2.3.0 + modern astroid==4.1.2 -- an incompatible pair (astroid.TryExcept was removed years ago). Tightened to `pylint>=3,<4` + a new `astroid>=3,<4` constraint, matching what the old lock actually resolved (pylint==3.3.9, astroid==3.3.11). 2. sync_platform_constraints.py's regex didn't handle extras syntax (`lxml[html-clean]==5.3.2`), so it silently skipped openedx-platform's lxml pin. uv then resolved lxml to 6.1.1, which bundles a newer libxml2 than xmlsec==1.3.14 was built against, causing "xmlsec.InternalError: lxml & xmlsec libxml2 library version mismatch" at runtime (anything importing python3-saml/social_core's SAML backend). Fixed the regex to allow an optional `[...]` extras group before `==`. Verified locally: pylint passes clean (exit 0), and the full test suite passes (2835 passed, 12 skipped). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tools)
Three more issues surfaced once the astroid/lxml fixes let the "docs"
toxenv actually run python -m build + twine check for the first time
(the old setup.py-based system never exercised this exact check):
1. enterprise/locale is a tracked symlink to enterprise/conf/locale
(needed for Django's i18n app-discovery at runtime). Wheel building
choked on it ("can't copy 'enterprise/locale': doesn't exist or not
a regular file") since MANIFEST.in's recursive-include picked it up
as a file, not a directory to descend into. `exclude enterprise/locale`
drops the symlink from the file list while the real files under
enterprise/conf/locale/ (which the symlink points to) still get
packaged via the same recursive-include pattern.
2. The dev group hard-pinned twine==1.11.0 (carried over verbatim from
the original dev.in, never previously exercised for `twine check`
since the old publish.yml uploaded via a GitHub Action, not the
repo's own twine). twine 1.11.0 predates the `check` subcommand
entirely. Removed the redundant pin -- dev already gets twine
unpinned via its existing {include-group = "doc"}.
3. setuptools>=82 dropped pkg_resources, which twine still imports --
same issue openedx-platform already hit and documented. Added
"setuptools<82" alongside the other repo-specific constraints.
Verified locally end-to-end: python -m build --wheel succeeds and
twine check dist/* reports PASSED.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI failed with pycodestyle reporting hundreds of E501 line-too-long violations at the default 79-char limit. The [pycodestyle] section originally lived in setup.cfg (not tox.ini, unlike most of the other repos in this migration), and I dropped it entirely when deleting setup.cfg instead of carrying it into tox.ini. Restored verbatim: ignore=E501,W503,W504 (this repo doesn't enforce a line-length limit) and the settings/migrations/static excludes. Also added the missing known_edx = [] to [tool.isort] to silence an isort warning about an empty "EDX" section. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…heck Removes MANIFEST.in references to requirements/*.txt/*.in and requirements/constraints.txt, which no longer exist after the uv migration. Also removes a no-op filter in sync_platform_constraints.py: pip-compile puts a package's "# via edx-enterprise" annotation on the line *after* its pin, so the same-line endswith() check never actually matched anything (verified against the live platform base.txt). The 8 affected packages were already being merged in regardless; this just removes the dead code and its now-inaccurate comment rather than changing behavior. 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>
constraint-dependencies mixes edx_lint-managed constraints with openedx-platform pins as plain strings, with no way to tell them apart once merged. Fence the platform block with begin/end comment markers so a reviewer scanning the array can see which entries are auto-synced and shouldn't be hand-edited. 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 all four top-level packages (enterprise, consent, integrated_channels, enterprise_learner_portal) to src/, in line with the reference implementation for this modernization effort (openedx/sample-plugin) and openedx/forum#281. Most complex repo in this batch: 4 packages, an in-package path helper with real call sites, and JS tooling (webpack, jasmine) alongside the Python side. - pyproject.toml: add where = ["src"] to packages.find - tox.ini: prefix src/ onto the pycodestyle exclude list and onto every package name in the isort/isort-check/quality env commands - Makefile: prefix src/ onto clean.static, dummy_translations, pull_translations (local destination paths only -- the Transifex/Atlas remote path identifiers are left unchanged), jshint, pylint, pycodestyle, isort, and isort-check targets - docs/conf.py: sphinx-apidoc call updated to point at src/enterprise - webpack.config.js: build context path updated to src/enterprise/static/enterprise - spec/javascripts/support/jasmine.yml: src_files glob updated to src/enterprise/static/enterprise/js/*.js - MANIFEST.in: all four recursive-include paths and the enterprise/locale symlink exclusion updated - enterprise/settings/test.py: this repo's in-package here()/root() path helper climbs from src/enterprise/settings/ back to the repo root -- fixed the climb from '../..' (2 levels) to '../../..' (3 levels), and updated its 3 real call sites (LOCALE_PATHS, REPO_ROOT, STATIC_ROOT) to prepend 'src' so they still resolve inside the package. The symlink enterprise/locale -> conf/locale is relative and moved as a unit with git mv, so it still resolves correctly post-move -- verified. Verified: uv sync (no lock drift), full py312-django52 test run (2849 passed), quality (pylint/pycodestyle/isort), isort-check, and pii_check envs pass, docs env passes end to end -- sphinx-apidoc generates real API docs from the new path, uv build --wheel + twine check pass. A pylint useless-suppression info note in integrated_channels/cornerstone/models.py is pre-existing (confirmed identical on the pre-migration commit via git stash), unrelated to this change.
Same category of fixes farhan flagged on the batch's other reviewed PRs, applied here proactively since this repo has the identical modernization shape: - ci.yml: add enable-cache/python-version to astral-sh/setup-uv and drop the now-redundant actions/setup-python step - pyproject.toml: add [tool.uv] package = true - Drop CHANGELOG.rst from the dynamic readme file list and delete it -- python-semantic-release + GitHub Releases is the changelog of record. Also removes the stale MANIFEST.in include and the docs/changelog.rst page (its own dedicated toctree block in docs/index.rst). - Migrate .coveragerc into [tool.coverage.*] and delete the old file -- also fixed a real gap in the process: the original only listed source=enterprise and enterprise/settings,enterprise/conf in omit, silently missing coverage scoping for the other 3 packages (enterprise_learner_portal, consent, integrated_channels) even though addopts already covers all 4 with --cov. Generalized the omit patterns to */settings/*, */conf/* etc. so they apply consistently across all four packages, matching what's actually being measured. Verified: uv sync (no lock drift), full py312-django52 test run (2849 passed), quality and docs envs pass end to end -- docs env's build + twine check confirms the readme/coverage config changes work, and apidoc still finds the package correctly.
Both referenced files/processes that no longer exist post-migration: CHANGELOG.rst was deleted (python-semantic-release + GitHub Releases is the changelog of record now), and __version__ in enterprise/__init__.py is no longer manually bumped (it's resolved via importlib.metadata at runtime, with versioning itself automated by semantic-release based on conventional commits). Neither is a manual per-PR task anymore.
a1aa66e to
ce5c672
Compare
…thedocs os) Cross-checked against review comments/fixes from openedx-ledger#242, edx-enterprise-subsidy-client#222, enterprise-access#1015, and enterprise-subsidy#441. - Wrap remaining bare tool invocations in the Makefile (py.test, jasmine, diff-cover, pylint, pycodestyle, code_annotations, isort) with `uv run` so they resolve against the uv-managed venv instead of falling back to whatever happens to be on PATH. - Drop the redundant dynamic readme field now that pyproject.toml can set it statically; keep only version as dynamic (populated by setuptools-scm). - Bump Read the Docs build image to ubuntu-26.04.
Resolved: kept our deletion of the old pip-tools requirements/*.txt files (superseded by uv/dependency-groups).
…migration # Conflicts: # enterprise/__init__.py # requirements/ci.txt # requirements/constraints.txt # requirements/dev.txt # requirements/doc.txt # requirements/edx-platform-constraints.txt # requirements/js_test.txt # requirements/pip-tools.txt # requirements/pip.txt # requirements/test.txt # src/enterprise/overrides/programs.py
Master's pylintrc gained a [PII] pii-terms block (edx-lint 6.2.0+ feature) via the merge; our lockfile was still resolving edx-lint 6.1.0, which doesn't recognize that option and made pylint hard-fail with unrecognized-option. Same fix already applied on openedx-ledger.
This repo has immutable releases enabled, which freezes a release's assets the moment it's published. The old flow (main PSR step publishes the release, a separate publish-action step attaches assets afterward) can never work under that constraint -- it would 422 on the first real release. Matches the fix already proven and merged on openedx/sample-plugin#57 and validated end-to-end on openedx/event-tracking#434: build without publishing (vcs_release: false), then create the release with dist/* attached in one gh release create call.
…v-migration # Conflicts: # enterprise/__init__.py # src/enterprise/migrations/0252_alter_enterprisecustomer_enable_demo_data_for_analytics_and_lpr_and_more.py
All five pinned actions in release.yml (checkout, python-semantic-release, upload-artifact, download-artifact, pypa/gh-action-pypi-publish) had drifted behind openedx/sample-plugin's verified reference pins: - actions/checkout: v7.0.0 -> v7.0.1 - python-semantic-release/python-semantic-release: v10.5.3 -> v10.6.2 - actions/upload-artifact: v4.6.2 -> v7.0.1 - actions/download-artifact: v4.3.0 -> v8.0.1 - pypa/gh-action-pypi-publish: @release/v1 (floating tag) -> SHA-pinned v1.14.2 The pypa/gh-action-pypi-publish change specifically reverses an earlier, explicit ask from salman2013 on this PR (review comment r3701784182 / reply r3746916720) to move off the SHA pin back to the floating @release/v1 tag because the SHA pin had previously caused a publish failure. That guidance has since been superseded org-wide: the newer direction (given on a sibling PR in this same modernization effort) is to match openedx/sample-plugin's release.yml exactly, which uses the SHA pin. All five SHAs were independently verified against both the commit history and the release tag before pinning. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
__init__.py and docs/conf.py aliased the same import to two different names (plain `version` vs `get_distribution_version`/`get_installed_version`, both leftover from the pkg_resources.get_distribution() era). Renamed both to `get_version`, matching sample-plugin's convention exactly (it uses `version as get_version` in both files). No behavioral change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- description was still the literal "Your project description goes
here" placeholder, carried forward unchanged from setup.py.
- tag_format was set to bare "{version}" based on an investigation
that missed the most recent tag: v8.8.0 (2026-08-07, the actual
latest) uses the "v" prefix, contradicting the "no v prefix" claim
the bare format was based on. Switched to "v{version}" (PSR's
default) so the first automated release correctly recognizes v8.8.0
as the prior release.
Per salman2013's review comments on PR #2654.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior config drove the whole build via a custom `commands:` block under the (incorrect) assumption that Read the Docs doesn't support uv natively -- that gave up RTD's own PDF/EPUB format generation and fail_on_warning handling, per salman2013's review. RTD does support uv natively via `python.install.method: uv`, which lets RTD run its own build (formats, fail_on_warning, LaTeX toolchain provisioning) after installing deps through uv instead of pip. Same pattern already used elsewhere in this migration batch: opaque-keys (same batch, keeps formats: [pdf, epub]) and openedx-platform#38915 (merged; its own PR description states this was verified against a real Read the Docs build, not just locally). Verified locally: `uv sync --group doc` (what RTD's install step runs) and `sphinx-build -W -b html` (RTD's fail_on_warning equivalent) both succeed cleanly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…migration # Conflicts: # enterprise/__init__.py # src/enterprise/filters/support.py
Bare \`python\` isn't guaranteed to be on PATH once a repo is uv-managed (a dev machine may only have python3, or none at all outside the uv venv) -- salman2013 asked to verify \`make docs\` works; the Sphinx build itself always succeeded, but the final auto-open-browser step failed with "command not found" here. \`uv run python\` guarantees the interpreter exists regardless of the host PATH, matching this migration's existing "prefix tool invocations with uv run" convention. Verified: \`make docs\` now completes end-to-end (exit 0), Sphinx build succeeded as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@irfanuddinahmad Please create a ticket for this update: #2654 (comment) This way, once the Tutor update is available in the future, we can make these changes here. |
…migration # Conflicts: # enterprise/__init__.py
…e job Matches openedx/sample-plugin's current release.yml; the custom OPENEDX_SEMANTIC_RELEASE_GITHUB_TOKEN secret isn't needed.
Makefile targets were fully inlining commands that tox.ini already defines as standalone envs. pylint and pycodestyle are left inlined since they have no matching standalone tox env (only bundled inside the quality env), and the top-level quality target is left as-is since it was already a thin alias over other make targets, not inlined commands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Makefile now assumes an already-synced/activated local env; uv run stays in CI workflow steps only. See openedx/ccx-keys#190 (review comment r4097501942). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…migration # Conflicts: # enterprise/__init__.py # src/enterprise/filters/enrollment.py # src/enterprise/overrides/branding.py # src/enterprise/overrides/program_nudge_email.py
Match the pin used by openedx/sample-plugin. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| addopts = "--cov enterprise --cov enterprise_learner_portal --cov consent --cov integrated_channels --cov-report term-missing --cov-report xml" | ||
| norecursedirs = [".*", "docs", "requirements", "site-packages", "scripts", "node_modules"] | ||
| testpaths = ["tests"] | ||
|
|
||
| [tool.coverage.run] | ||
| branch = true | ||
| source = ["enterprise", "consent", "integrated_channels", "enterprise_learner_portal"] |
There was a problem hiding this comment.
Both reference package directories by name at the repo root, but this is a src/ layout repo — all four packages live under src/, not at the root. Coverage will silently measure nothing.
Fix:
pyproject.toml:555 — replace with:
source = ["src"]
pyproject.toml:549 — replace with bare --cov:
addopts = "--cov --cov-report term-missing --cov-report xml"
…migration # Conflicts: # requirements/ci.txt # requirements/dev.txt # requirements/doc.txt # requirements/edx-platform-constraints.txt # requirements/js_test.txt # requirements/pip-tools.txt # requirements/test-master.txt # requirements/test.txt
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.cfg.src/enterprise/__init__.py's__version__(used at runtime bycache_utils.versioned_cache_key) now derives fromimportlib.metadatainstead of a hand-maintained string.pip-compiletouv:requirements/*.in+*.txtare replaced by PEP 735[dependency-groups]+ a singleuv.lock.tox.ini/Makefile/Dockerfileand CI workflows install uv and run viauv run tox/uv sync.requirements/edx-platform-constraints.txt+check_pins.pyare replaced byrequirements/sync_platform_constraints.py, merging edx-platform's compiled pins into[tool.uv].constraint-dependencies(run afteredx_lint write_uv_constraints, beforeuv lock --upgrade).pyyamlis excluded from that merge since thejs_testgroup'sjasminedependency hard-pins it far below edx-platform's pin;js_testis its own conflicting resolution fork via[tool.uv].conflicts.python-semantic-release, publishing to PyPI via OIDC. The oldpublish.ymlalso ranmake pull_translationsbefore building; that Transifex pull is dropped (would need new release-pipeline secrets) -- translations still refresh via the existing Makefile targets, just not implicitly on every release.Part of openedx/public-engineering#513.
Test plan
uv lockresolves cleanly (262 packages)uv run tox -e py312-django52(2938 passed, 12 skipped)make docsbuilds cleanly end-to-endRelease readiness (pre-merge blocker)
edx-enterprise→ GitHub repo
openedx/edx-enterprise, workflowrelease.yml→ Confirmed working by:
Do not merge until the box above is checked -- until then,
publish_to_pypiwill failon first merge to
master(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