Repository navigation
feat: modernize Python repo to uv + pyproject.toml - #1340
Conversation
8bc6212 to
81b40aa
Compare
69a6924 to
8761f6b
Compare
irfanuddinahmad
left a comment
There was a problem hiding this comment.
Before merging, a PyPI owner needs to add a trusted publisher for this repo with workflow release.yml. Earlier releases used an API token, so the first OIDC publish will fail without it.
Also needs a maintainer decision before merge: the v6.0.0 tag and GitHub release exist, but the PyPI and npm publish runs failed, so both are still on 5.2.0. Either re-cut 6.0.0 or accept that the first automated release will be 6.0.1/6.1.0.
| - name: Create GitHub Release with Assets | ||
| if: steps.release.outputs.released == 'true' | ||
| env: | ||
| GH_TOKEN: ${{ secrets.REQUIREMENTS_BOT_GITHUB_TOKEN }} |
There was a problem hiding this comment.
Please move npm publishing into release.yml as a publish_to_npm job, as sample-plugin does, and delete publish-npm.yml. Then gh release create can go back to secrets.GITHUB_TOKEN; we don't use custom org tokens in release.yml.
| exclude conftest.py | ||
| exclude .gitignore | ||
| exclude tox.ini | ||
| exclude CHANGELOG.rst |
There was a problem hiding this comment.
Please remove exclude CHANGELOG.rst from MANIFEST.in. The 5.2.0 sdist ships CHANGELOG.rst; dropping it from the long description is fine, but the file should keep shipping.
| @@ -0,0 +1,84 @@ | |||
| name: Release | |||
There was a problem hiding this comment.
Please replace release.yml with sample-plugin's current release.yml, dropping only the monorepo lines (directory:, the tutor build step, the extra publish jobs, packages-dir:). The pins and logic already match, but the workflow name, step names, run: | style and comments differ; keeping it identical makes future syncs a plain diff.
e357d3c to
290a72e
Compare
irfanuddinahmad
left a comment
There was a problem hiding this comment.
This needs a rebase: master has 3 new commits (including the 6.1.1 bump), and uv lock --check fails on this branch, so please re-lock after rebasing.
| runs-on: ${{ matrix.os }} | ||
| name: ${{ matrix.toxenv }} | ||
| runs-on: ubuntu-latest | ||
| strategy: |
There was a problem hiding this comment.
Please restore fail-fast: false under strategy:. It was on master, and this migration should keep CI behaviour unchanged.
| - name: Install requirements | ||
| run: pip install -r requirements/ci.txt | ||
| - name: Run tox | ||
| run: uv run tox -e ${{ matrix.toxenv }} |
There was a problem hiding this comment.
Please use uv run --locked tox -e ${{ matrix.toxenv }}. Without --locked, uv run silently re-locks when uv.lock is stale instead of failing. It also matches i18n-tools, codejail-includes and platform-plugin-aspects.
There was a problem hiding this comment.
I think --locked with sync commands is enough.
64da666 to
62bfbb0
Compare
|
@irfanuddinahmad The PR is available for the next pass. |
|
Before merging, an npm owner of |
62bfbb0 to
5b81113
Compare
|
|
||
| - name: Setup node | ||
| uses: actions/setup-node@f1f314fca9dfce2769ece7d933488f076716723e # v1.4.6 | ||
| uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 |
There was a problem hiding this comment.
This needs a rebase, it conflicts with master in this file and publish-npm.yml after #1344 moved actions/setup-node to v7.1.0. Taking master's setup-node pin here and keeping publish-npm.yml deleted should resolve it, release.yml already uses the newer pin.
There was a problem hiding this comment.
Fixed in 78fa600. Rebased; ci.yml and release.yml use the setup-node v7.1.0 pin, publish-npm.yml stays deleted.
| # When running on a PR branch without the release tag, setuptools-scm returns | ||
| # a dev version (e.g. "0.0.0.dev0"). Skip the check in that case — there is | ||
| # nothing to validate until the release tag is reachable. | ||
| if python_version.endswith('.dev0') or '.dev' in python_version: |
There was a problem hiding this comment.
With the release setting the npm version from the tag, package.json never gets bumped in git, so this check can't catch anything anymore. In CI it always skips:
Version check skipped (dev version detected: 0.0.0.dev0).
and locally, once there's a release tag after this lands, it fails for anyone with a full clone:
ERROR: Version mismatch...
Delete the script along with the version_check tox env and its entry in the CI matrix. Then fallback_version in pyproject.toml can go back to 0.0.0, since this script was the only thing that needed the .dev0.
There was a problem hiding this comment.
Fixed in 78fa600. Removed version_check.py, its tox env and CI entry; fallback_version is back to "0.0.0".
| To install edx-proctoring (requires `uv <https://docs.astral.sh/uv/>`_), create and activate a virtualenv, | ||
| then install. Run the ``make`` commands below with the virtualenv activated: |
There was a problem hiding this comment.
uv sync ignores an activated virtualenv unless it's .venv, so with any other venv active make install warns VIRTUAL_ENV=... does not match the project environment path .venv and will be ignored, creates .venv, and then ./manage.py migrate fails with ImportError: Couldn't import Django. Point people at uv venv and source .venv/bin/activate before make install instead.
There was a problem hiding this comment.
Fixed in 78fa600. README now runs uv venv and source .venv/bin/activate before make install.
8580f27 to
78fa600
Compare
Move packaging to pyproject.toml with a src/ layout, uv dependency groups and python-semantic-release, per public-engineering#506. - The wheel keeps master's contents, including the test modules edx-platform imports. - publish-pypi.yml is renamed to release.yml and runs ci.yml before releasing, since ci.yml no longer runs on push to master. - Coverage measures src/edx_proctoring by path and uses */ omit globs, so the measured files match master. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
78fa600 to
d2f2b2e
Compare
Important
Written with the assistance of Claude Code, human-reviewed before posting.
Modernize
edx-proctoringPart of openedx/public-engineering#506
Note
Translations: after the
src/move, the openedx-translations extraction job will commit new source strings undertranslations/edx-proctoring/src/..., whichtransifex.ymland edx-platform'satlas pulldon't read. Existing translations are unaffected; only new or changed strings pause until openedx/openedx-translations#82365 is fixed (the workflow will stripsrc/). Same situation as the already-merged DoneXBlock and RecommenderXBlock.Summary
@edx/edx-proctoring)src/layout (src/edx_proctoring/)setup.py/setup.cfgwithpyproject.toml(PEP 621 static metadata)uvwith PEP 735 dependency groups; commituv.lock.coveragercinto[tool.coverage.run](pyproject.toml:189)ci.yml,migrations-check.yml) to useastral-sh/setup-uv; SHA-pin all actionspublish-pypi.ymlwithrelease.yml(python-semantic-release+ OIDC trusted publishing), gated onci.ymlrequires-python = ">=3.12"(pyproject.toml:8)Removed/Updated
Deleted files:
setup.py,setup.cfg,requirements/,.coveragerc,openedx.yaml(legacy OEP-2 metadata, removed rather than ported)Renamed/replaced:
.github/workflows/publish-pypi.yml→.github/workflows/release.ymlWhere
setup.cfg/.coveragercsettings went:[isort][tool.isort](pyproject.toml:203)[pycodestyle],[pydocstyle](full ignore list),[doc8]tox.ini:72-85[tool:pytest][tool.pytest.ini_options](pyproject.toml:199).coveragerc[tool.coverage.run](pyproject.toml:189)[bdist_wheel] universal = 1py3instead ofpy2.py3Removed Makefile targets:
$(COMMON_CONSTRAINTS_TXT)common_constraints.txt; constraints now come fromedx_lint write_uv_constraintsUpdated Makefile targets:
upgradeedx_lint write_uv_constraints pyproject.toml+uv lock --upgrade(no--locked, because this target is meant to change the lockfile)requirementsuv sync --locked --group devinstead of pip-syncrequirements-testuv sync --locked --group testinstead ofpip install -rquality-pythontox -e quality; pylint runs withPYTHONPATH=.somock_appsresolves undersrc/test-pythonpython -Wd -m pytest ...directly (nopip install -e .step)coverage,lint-js, translation targetssrc/edx_proctoringPython 3.12 only
Master's CI matrix and classifiers were already 3.12-only, but master had no
python_requiresand still listedpy311in the tox envlist. This PR dropspy311from the envlist and setsrequires-python = ">=3.12".Versioning
[Dynamic]
setuptools-scmwithdynamic = ["readme", "version"]. Master had a PyPI publish workflow, andpython-semantic-releasenow sets the version at release time from git tags. PSR only creates the tag and never pushes a commit;publish-npm.ymlsets the npm version from the release tag (npm version --no-git-tag-version) beforenpm publish, sopackage.json:versionis not bumped in git.Important Notes
package.json:5(main) andpackage.json:14(files) now point atsrc/edx_proctoring/static. Without this change,npm packproduced only 3 files and no JS. Verified:npm pack --dry-runlists the same 47 files as master, andrequire.resolve('@edx/edx-proctoring')resolves tosrc/edx_proctoring/static/index.js, which exports thehandlerWrapperthat mockprock uses.release.ymlfollows the sample-plugin standard with one exception.gh release createusesGH_TOKEN: ${{ secrets.REQUIREMENTS_BOT_GITHUB_TOKEN }}(release.yml:44). Releases created withGITHUB_TOKENdon't start other workflows, and.github/workflows/publish-npm.ymlruns onrelease: published. There is nosecrets: inheritonrun_ciand nofetch-depth: 0.ci.ymlno longer has apush: mastertrigger.release.ymlcalls it as its first job (run_ci), so CI still runs on every merge, and nothing is released unless CI passes. The workflow namePython CI, the matrix env namesdjango42-drflatest/django52-drflatest, and the Codecov upload fromdjango42-drflatest(ci.yml:53) match master.version_variables, so it never pushes a version-bump commit tomaster. Such a push would be rejected, becausemasterrequires theopenedx/clacheck. The npm version comes from the release tag inpublish-npm.ymlinstead, the same approach as openedx-atlas.v6.0.0tag and GitHub release exist, but PyPI and npm are still on 5.2.0.python-semantic-releasereads tags, so it will treat 6.0.0 as released, and the first automated release will be 6.0.1 or 6.1.0. Maintainers: if 6.0.0 should ship as-is, the tag and release need to be re-cut before this merges (publish-npm.ymluses--allow-same-version, so a re-cutv6.0.0still publishes).version_check:src/edx_proctoring/scripts/version_check.py:13now skips.devversions. CI uses a shallow checkout, so setuptools-scm resolves to0.0.0.dev0there and the check is skipped.fail-fast:fail-fast: falsewas dropped from the CI matrix, per the modernization convention. One failing job now cancels the rest.uv syncuses--locked(ci.yml:47,migrations-check.yml:32,Makefile:37,Makefile:41), so a staleuv.lockfails fast.--lockedis never used onuv run.djangorestframework<3.17andedx-lint<6.2.0are pinned in[tool.edx_lint] uv_constraints(pyproject.toml:181,pyproject.toml:186), with the reason for each inline.Requires-Dist, and a byte-identicalentry_points.txtcompared with master.src/edx_proctoring/localeis a git-tracked symlink to./conf/locale, and the wheel still builds cleanly because package data is listed explicitly (include-package-data = false). The long description is now the README only:CHANGELOG.rstis deprecated and has a note prepended.src/:.pii_annotations.yml:1source_path,.tx/configfile paths,.eslintignore,karma.conf.js, and the GitHub source links indocs/.docs/developing.rstnow describes the PSR flow but keeps the manual edx-platform upgrade steps. Indocs/pull_request_template.md, the conventional-commit item (linking OEP-0051) replaces the version-bump/changelog items in the pre-merge checklist, and the Post-Merge tag step is removed. Themkvirtualenvstep is dropped fromREADME.rst.docstox env:docs/has RST files but no Sphinxconf.pyand nomake docstarget, so there is no[testenv:docs]and no docs job in CI.Out of Scope
celery50/drflatesttox factorscelery{50}-django{42,52}-drflatest(tox.ini:3) is kept for parity with master's env names, but these factors are now labels only. Celery and DRF come fromuv.lock, not per-factor deps.<3.17ListSerializer.errorsto an index-keyed dict, and several tests assert the old list shape..tx/configlegacyedx-proctoring-jsresource'ssource_filepoints atdjango.po(.tx/config:12), the same as on master. This PR only updated the path..tx/configis still used; fix or remove it separatelytranslations-django42/52tox envs[testenv:translations], so they run the unit tests rather thanmake validate_translations. Same on master.publish-npm.ymlstill needs the bot token described above. Publishing npm fromrelease.ymlwould let the release useGITHUB_TOKEN.Testing Notes
Note: PR creator has tested the PR via CI and building the packages manually. No manual testing has been done to test the repo features. It will be great if the repo owner could test it.
Important
npm release flow needs critical review.
publish-npm.ymlnow runs on the release created byrelease.ymland sets the npm version from the release tag (npm version --no-git-tag-version) instead of a version-bump commit. Please review this flow carefully, and verify after merge that the first release publishes@edx/edx-proctoringto npm with the correct version.🤖 Generated with Claude Code