chore: modernize Python tooling (uv, PEP 621, semantic-release, src layout) by salman2013 · Pull Request #265 · openedx/xblock-image-explorer

salman2013 · 2026-07-17T07:27:23Z

Summary

Phase 1: Migrate package metadata to pyproject.toml (PEP 621) — removed setup.py, added setuptools-scm for dynamic versioning, coverage config, importlib.metadata for __version__
Phase 2: Switch dependency management from pip-tools/requirements/*.txt to uv + uv.lock with [dependency-groups]; update tox.ini to use tox-uv/uv-venv-lock-runner; update CI to use astral-sh/setup-uv
Phase 3: Add python-semantic-release workflow for automated PyPI publishing via OIDC trusted publisher; add commitlint workflow to enforce conventional commits
Phase 4: Migrate to src/ layout — moved image_explorer/ to src/image_explorer/

Test plan

uv run tox -e quality — pylint 9.45/10, passes
uv run tox -e py312-django42 — 6/6 tests passed
uv run tox -e py312-django52 — 6/6 tests passed
Configure OIDC trusted publisher on PyPI for openedx/xblock-image-explorer (publisher: GitHub Actions, workflow: release.yml)

🤖 Generated with Claude Code

openedx-webhooks · 2026-07-17T07:27:29Z

Thanks for the pull request, @salman2013!

This repository is currently maintained by @Agrendalath.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where 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:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

farhan · 2026-07-21T09:27:40Z

 from .image_explorer import ImageExplorerBlock

-__version__ = '3.1.1'
+__version__ = version("xblock-image-explorer")


__version__ is assigned without a fallback. If the package is not installed, importlib.metadata.version() raises PackageNotFoundError and the import of this module fails entirely. Either:

Add a try/except PackageNotFoundError guard with a __version__ = "0.0.0" fallback, or

Remove __version__ entirely if no downstream code imports it (the simpler outcome).

fixed and remove version entirely if no downstream code imports it (the simpler outcome).

farhan · 2026-07-21T09:27:40Z

@@ -0,0 +1,140 @@
+[build-system]
+requires = ["setuptools>=64", "setuptools-scm>=8.0"]


We discussed about it before.

Should we add the minimum bound in setuptools>=64

farhan · 2026-07-21T09:27:40Z

+    "static/**",
+    "templates/**",
+    "public/**",
+    "conf/locale/**",


translations/config.yaml is present in the repo (src/image_explorer/translations/) and was included in the master wheel, but is missing from the PR wheel. Add "translations/**" to [tool.setuptools.package-data]:

[tool.setuptools.package-data] "image_explorer" = [ "static/**", "templates/**", "public/**", "conf/locale/**", "translations/**", ]

Because translations/ in this repo is a symlink (git mode 120000) pointing to conf/locale/. Setuptools cannot copy symlinks as package data — it fails with:

error: can't copy 'src/image_explorer/translations': doesn't exist or not a regular file

The actual files (like config.yaml) live in conf/locale/, which is already covered by the existing "conf/locale/" entry. So adding "translations/" would be redundant and break the wheel build.

Just reviewed the translations setup — translations/config.yaml is the atlas config used to pull translation files into the source repo (via make pull_translations in the Makefile). Atlas reads the source tree, not the installed wheel, so this file doesn't need to be in the package. Closing this comment.

https://github.com/salman2013/xblock-image-explorer/blob/modernize-python-tooling/src/image_explorer/conf/locale/config.yaml
https://github.com/salman2013/xblock-image-explorer/blob/modernize-python-tooling/Makefile

farhan · 2026-07-21T11:40:16Z

 name: Python CI

 on:
-  push:


Is this removal intentional?

@claude generated details:
The push: branches: [master] trigger was removed. Master CI used it to run checks on direct pushes to master; with only workflow_call added, direct pushes no longer trigger CI independently. Consider restoring the push: trigger alongside workflow_call:.

farhan · 2026-07-21T11:40:53Z

+
+      - name: Upload dist artifacts
+        if: steps.release.outputs.released == 'true'
+        uses: actions/upload-artifact@v4


actions/upload-artifact@v4 uses a mutable tag. All other actions in this PR are SHA-pinned — pin this one too.

Resolve the current SHA:

gh api repos/actions/upload-artifact/git/ref/tags/v4 --jq .object.sha

farhan · 2026-07-21T11:41:32Z

+
+    steps:
+      - name: Download dist artifacts
+        uses: actions/download-artifact@v4


same:

actions/download-artifact@v4 uses a mutable tag. Pin to a full SHA with a version comment, matching the pattern used for the other actions in this workflow.

farhan · 2026-07-21T11:42:54Z

-	$(PIP_COMPILE) -o requirements/ci.txt requirements/ci.in
-	sed -i '/^[dD]jango==/d' requirements/test.txt
+upgrade: ## update uv.lock and constraints with the latest packages
+	uv lock --upgrade


The upgrade target runs uv lock --upgrade before edx_lint write_uv_constraints. The standard order is edx_lint write_uv_constraints first (so constraints are updated), then uv lock --upgrade (so the lockfile respects them). Swap the two lines.

farhan · 2026-07-28T12:55:40Z

+[tool.semantic_release]
+# version is dynamic via setuptools-scm; no version_toml needed
+build_command = "python -m pip install --upgrade build && SETUPTOOLS_SCM_PRETEND_VERSION=$NEW_VERSION python -m build"
+allow_zero_version = true


I think repo already has version greater than zero, removing this config will be a great cleanup.

farhan · 2026-07-28T15:18:59Z

+        {group = "django42"},
+    ],
+]
+constraint-dependencies = []


The next line (constraint-dependencies = []) is empty — edx_lint write_uv_constraints was never run.

Without this, global edx-lint constraints (security pins, compatibility caps) are not applied to the lockfile. Run:

uv run --with edx-lint edx_lint write_uv_constraints pyproject.toml

Then commit the updated pyproject.toml and uv.lock.

farhan · 2026-07-28T15:18:59Z

 commands =
    mkdir -p var
    pytest {posargs:tests --cov image_explorer}



pytest is called directly here. Per the tooling flow, the Makefile should define commands and tox should call them:

Add to Makefile:
test: ## run tests mkdir -p var pytest $(posargs)

Change this commands line to make test.

farhan · 2026-07-28T15:19:00Z

 commands =
-    pylint --fail-under=9.0 image_explorer
+    pylint --fail-under=9.0 src/image_explorer



pylint is called directly here. Per the tooling flow, the Makefile should define commands and tox should call them:

Add to Makefile:
quality: ## run linting pylint --fail-under=9.0 src/image_explorer

Change this commands line to make quality.

farhan · 2026-07-28T15:19:00Z

+          path: dist/
+
+      - name: Publish to PyPI
+        uses: pypa/gh-action-pypi-publish@release/v1


The uses: on the next line (pypa/gh-action-pypi-publish@release/v1) is not SHA-pinned — all other actions in this PR use a full commit SHA. Pin it:

uses: pypa/gh-action-pypi-publish@<SHA> # release/v1

Get the current SHA:

gh api repos/pypa/gh-action-pypi-publish/git/ref/heads/release/v1 --jq '.object.sha'

mphilbrick211 · 2026-08-19T21:37:59Z

Hi @salman2013! Some branch conflicts have popped up - would you mind taking a look?

Agrendalath · 2026-09-04T14:12:55Z

Hi @salman2013, it seems like this PR could benefit from the changes applied to other repos as part of their modernization. I'm converting this to a draft; please mark it as "ready for review" once it's ready.

- Migrate package metadata from setup.py/setup.cfg to pyproject.toml (PEP 621) - Switch dependency management from pip-tools to uv with dependency groups - Migrate to src/ layout - Add python-semantic-release and commitlint workflows - Replace pypi-publish.yml with release.yml (PSR + OIDC trusted publisher) - Add uv.lock and drop requirements/ directory Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

farhan

1 request, rest LGTM!

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

farhan

few nits

farhan · 2026-09-23T09:53:44Z

@@ -1,29 +1,32 @@
 [tox]
 envlist = py312-django{42,52},quality,package


docs is missing

docs was not in the original tox.ini on master before the migration.

farhan · 2026-09-23T09:54:42Z

-requirements: piptools  ## install test requirements locally
-	pip-sync requirements/ci.txt
+quality: ## run linting
+	pylint --fail-under=9.0 src/image_explorer


add uv run at start

farhan · 2026-09-23T09:56:54Z

-	pip install -r requirements/pip-tools.txt
+test: ## run tests
+	mkdir -p var
+	pytest $(posargs)


add uv run at start

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

farhan

LGTM!

irfanuddinahmad · 2026-10-07T10:44:24Z

pyproject.toml line 9: license = "AGPL-3.0" is the bare/deprecated SPDX form. setup.py has no license classifier to check against, but src/image_explorer/__init__.py's own copyright header explicitly grants "...or (at your option) any later version of the AGPL..." — that's a real "or later" grant in this repo's own source, so license = "AGPL-3.0-or-later" is the correct value here, not something to leave ambiguous.

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Jul 17, 2026

openedx-webhooks added this to Contributions Jul 17, 2026

github-project-automation Bot moved this to Needs Triage in Contributions Jul 17, 2026

salman2013 mentioned this pull request Jul 17, 2026

Modernize Python repo | XBlocks openedx/public-engineering#511

Open

mphilbrick211 moved this from Needs Triage to Ready for Review in Contributions Jul 20, 2026

mphilbrick211 requested a review from Agrendalath July 20, 2026 20:34

farhan self-requested a review July 21, 2026 09:08

farhan suggested changes Jul 21, 2026

View reviewed changes

salman2013 mentioned this pull request Jul 28, 2026

test: fix flaky video block JS tests openedx/xblocks-core#278

Closed

1 task

farhan suggested changes Jul 28, 2026

View reviewed changes

salman2013 requested a review from farhan July 31, 2026 06:55

mphilbrick211 moved this from Ready for Review to In Eng Review in Contributions Aug 19, 2026

Agrendalath marked this pull request as draft September 4, 2026 14:13

salman2013 force-pushed the modernize-python-tooling branch from f7ae64d to 88e0e8d Compare September 22, 2026 10:25

salman2013 force-pushed the modernize-python-tooling branch from 88e0e8d to b54d7cd Compare September 22, 2026 10:26

salman2013 marked this pull request as ready for review September 22, 2026 10:27

farhan suggested changes Sep 22, 2026

View reviewed changes

Comment thread src/image_explorer/__init__.py

fix: restore __version__ using importlib.metadata

32226d0

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

farhan suggested changes Sep 23, 2026

View reviewed changes

fix: prefix pytest and pylint with uv run in Makefile

5581127

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

salman2013 requested a review from farhan September 23, 2026 11:03

farhan approved these changes Sep 23, 2026

View reviewed changes

		@@ -0,0 +1,140 @@
		[build-system]
		requires = ["setuptools>=64", "setuptools-scm>=8.0"]

		@@ -1,29 +1,32 @@
		[tox]
		envlist = py312-django{42,52},quality,package

Conversation

salman2013 commented Jul 17, 2026

Summary

Test plan

Uh oh!

openedx-webhooks commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mphilbrick211 commented Aug 19, 2026

Uh oh!

Agrendalath commented Sep 4, 2026

Uh oh!

farhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

farhan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farhan left a comment

openedx-webhooks commented Jul 17, 2026 •

edited

Loading