Modernize python tooling by salman2013 · Pull Request #519 · openedx/xblock-sdk

salman2013 · 2026-07-14T19:19:24Z

Modernizes the Python tooling for this repo in three phases, aligning with the Open edX org standard established in openedx/sample-plugin.

Ticket: openedx/public-engineering#511
Parent ticket: openedx/public-engineering#506

Changes generated by Claude Sonnet 4.6 using the modernize-python-tooling skill, reviewed by a human.

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

openedx-webhooks · 2026-07-14T19:19:30Z

Thanks for the pull request, @salman2013!

This repository is currently maintained by @openedx/axim-engineering.

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.

Populates [tool.uv].constraint-dependencies with setuptools<82.0 and common constraints so uv.lock pins setuptools to <82 (required by openedx-django-pyfs which uses pkg_resources). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

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

mphilbrick211 · 2026-07-15T14:44:10Z

@openedx/axim-engineering this is ready for review. Thanks!

irfanuddinahmad · 2026-07-16T12:33:39Z

👋 Reviewed this against the same checklist we've been applying across the modernization effort (openedx/public-engineering#506/#511/#513/#514). A couple of things worth fixing before merge:

1. Old pypi-publish.yml is still active and will race with the new release.yml. This PR adds release.yml (OIDC-based, triggered by conventional commits on master) but doesn't remove the pre-existing .github/workflows/pypi-publish.yml, which triggers on push: tags: '*'. Once this merges and semantic-release pushes its first version tag, both workflows will fire and race to publish the same version via two different auth mechanisms (old token vs. new OIDC) — recommend deleting pypi-publish.yml as part of this PR.

2. pypa/gh-action-pypi-publish@release/v1 is a floating branch ref, not a pinned SHA. This is exactly what caused openedx/xblocks-core's release to fail with docker: manifest unknown (https://github.com/openedx/xblocks-core/actions/runs/29327012856/job/87066591905) — the ref resolved to a commit whose Docker image wasn't published on ghcr.io yet. Worth pinning to a commit SHA like the rest of this workflow's actions already are. (This traces back to openedx/sample-plugin's own reference release.yml, which has the same gap — filed openedx/sample-plugin#54 and openedx/xblocks-core#280 for that.)

Minor/non-blocking: codecov/project is currently failing — real tests and codecov/patch (100%) pass, looks like a coverage-threshold comparison issue rather than a functional problem.

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

farhan

Improvements:

CHANGELOG.rst not deleted — for PyPI repos (release gate passed), CHANGELOG.rst should be deleted because python-semantic-release takes over release notes. The file is also still listed in MANIFEST.in. — this is an absent deletion.
.github/workflows/pypi-publish.yml not deleted — this workflow fires on every tag push and will conflict with release.yml, causing duplicate publish attempts.

farhan · 2026-07-21T09:35:47Z

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


We had a discussion on it.
It's not a blocker though.

farhan · 2026-07-21T09:35:47Z

+"workbench" = ["static/**/*", "templates/**/*", "test/**/*"]
+
+[tool.setuptools.exclude-package-data]
+"*" = ["tests*", "*.tests*", "spec*", "*.spec*"]


The pattern "tests*" does not match directories named test/ (singular). The test dirs in this repo (workbench/test/, sample_xblocks/basic/test/) use the singular form, so they appear in the built wheel. Add "test*" and "*.test*":

"*" = ["test*", "tests*", "*.test*", "*.tests*", "spec*", "*.spec*"]

Confirmed still outstanding at the current head (6bad88d) -- line is now unchanged: "*" = ["tests*", "*.tests*", "spec*", "*.spec*"], which still won't match the singular test/ dirs (workbench/test/, sample_xblocks/basic/test/). Minimal fix that matches the actual dir names without over-broadening to any file merely starting with "test":

Suggested change

"*" = ["tests*", "*.tests*", "spec*", "*.spec*"]

"*" = ["tests*", "*.tests*", "spec*", "*.spec*", "test/*", "*/test/*"]

farhan · 2026-07-21T09:35:47Z

+
+[tool.coverage.run]
+branch = true
+source = ["."]


After the src/ move, source = ["."] measures coverage from the repo root rather than the installed packages. Use source_pkgs instead:

source_pkgs = ["workbench", "sample_xblocks"]

This ensures pytest-cov targets the installed package under src/ rather than unrelated root-level files.

Confirmed still source = ["."] at the current head. Suggested fix:

Suggested change

source = ["."]

source_pkgs = ["workbench", "sample_xblocks"]

farhan · 2026-07-21T09:35:48Z

+branch = true
+source = ["."]
+omit = [
+    "workbench/test/*",


The omit paths here (and on the next line) use the pre-src/ layout. After the move, test files live at src/workbench/test/* and src/sample_xblocks/*/test/*, so these patterns will not match and test-helper code will be included in coverage:

omit = [ "src/workbench/test/*", "src/sample_xblocks/*/test/*", "*/migrations/*", "*/__pycache__/*", "*/settings.py", ]

Confirmed still unfixed -- both omit lines are unchanged (workbench/test/*, sample_xblocks/*/test/*), pre-src/ layout. Since this comment spans two lines I'm not using a suggestion block here (a single-line suggestion on a two-line comment risks a partial apply that duplicates the other line) -- but the fix is:

omit = [ "*/workbench/test/*", "*/sample_xblocks/*/test/*", "*/migrations/*", "*/__pycache__/*", "*/settings.py", ]

Using a */ prefix rather than hardcoding src/ so it still matches regardless of whether coverage sees the editable-install path or a site-packages path.

farhan · 2026-07-21T09:35:48Z

-__version__ = '0.14.0'
+from importlib.metadata import version
+
+__version__ = version("xblock-sdk")


importlib.metadata.version() raises PackageNotFoundError on any checkout that hasn't been pip install -e .'d. Wrap with a fallback:

from importlib.metadata import version, PackageNotFoundError try: __version__ = version("xblock-sdk") except PackageNotFoundError: __version__ = "0.0.0"

Confirmed still unwrapped at the current head:

from importlib.metadata import version __version__ = version("xblock-sdk")

Same real risk as the sibling repos in this effort -- any environment where xblock-sdk's dist-info isn't discoverable at import time raises unhandled and takes down the whole package on import.

This comment is anchored on the __version__ = ... line only, and the fix also touches the import line above it, so a single-line suggestion here would leave a duplicate import if applied as-is. Full fix:

from importlib.metadata import PackageNotFoundError, version try: __version__ = version("xblock-sdk") except PackageNotFoundError: __version__ = "unknown"

farhan · 2026-07-21T09:35:48Z

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


actions/upload-artifact@v4 is a floating tag. Pin to a SHA with a version comment for supply-chain security, consistent with actions/checkout and setup-uv elsewhere in this PR.

Confirmed still @v4 (floating) at the current head. Verified real SHA via the tags API (git/refs/tags/v4 on actions/upload-artifact resolves to this commit):

Suggested change

uses: actions/upload-artifact@v4

uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4

farhan · 2026-07-21T09:35:48Z

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


actions/download-artifact@v4 is a floating tag. Pin to a SHA with a version comment, same as the upload step above.

Confirmed still @v4 (floating) at the current head. Verified real SHA the same way (git/refs/tags/v4 on actions/download-artifact):

Suggested change

uses: actions/download-artifact@v4

uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4

farhan · 2026-07-21T13:03:50Z

@@ -1,33 +1,33 @@
 name: Python CI
 on:
-  push:


The push: branches: [master] trigger was removed.

Is this intentional?

Checked -- this is fine, not a regression. ci.yml dropped its standalone push: branches: [master] trigger, but release.yml (which itself triggers on: push: branches: [master]) calls ci.yml via workflow_call as its run_tests job and gates release on needs: run_tests. So the full test suite still runs on every push to master -- it's just invoked through release.yml now instead of ci.yml triggering itself redundantly. Net behavior is unchanged; this matches the pattern used elsewhere in this migration (tests-gate-release via workflow_call).

farhan · 2026-07-28T13:03:21Z

 install: requirements

 test: clean ## run tests in the current virtualenv
-	pip install -qr requirements/local.txt --exists-action w ## Install sample xblocks


local dependency group is missing in the pyproject.toml
its also missing here while testing.
Is this intentional?

Confirmed intentional on the dependency-group side: the old requirements/local.txt only did -e . (installing the package itself), and [tool.uv] package = true in pyproject.toml means every uv sync already installs this package in editable mode -- so there's no need for a separate "local" group.

But the Makefile line itself (now at line 47, shifted since your comment) is a live bug, not just a stale reference: requirements/local.txt no longer exists in this PR at all (confirmed -- the whole requirements/ dir is deleted), so make coverage fails outright today.

Suggested change

pip install -qr requirements/local.txt --exists-action w ## Install sample xblocks

(deletes just that one pip install line -- pytest --cov-report html right below already runs against whatever's installed via uv sync, no separate install step needed.)

The local group is intentionally absent. The old requirements/local.txt only contained -e . (install the package in editable mode), which is now
handled automatically by package = true in [tool.uv] — every uv sync call installs the package in editable mode without a separate group.

irfanuddinahmad

Second pass -- replied to farhan's 9 outstanding threads above with current-code confirmation (all still unfixed as of this head) plus SHA-pins/suggestions. A few new things beyond those threads:

gh pr view currently shows mergeable: CONFLICTING against master. CI is green but predates that conflict, so it hasn't actually validated this code against current master.
Two now-dead workflow files are still present and weren't in farhan's earlier pass: .github/workflows/pypi-publish.yml references requirements/pip.txt and calls python setup.py sdist bdist_wheel -- both deleted by this PR, so this workflow would fail outright if it ever ran (a tag push), and it's fully superseded by release.yml's publish_to_pypi job anyway. Worth deleting rather than leaving a broken duplicate publish path around.
.github/workflows/upgrade-python-requirements.yml calls the org's pip-tools-oriented reusable workflow (openedx/.github/.github/workflows/upgrade-python-requirements.yml@master), which doesn't have anything to do now that there's no requirements/*.in left to compile -- is this staying around intentionally (e.g. pending an org-wide uv equivalent) or should it go too?
changelog: "false" in release.yml disables changelog generation same as flagged on sibling PRs in this effort. Unlike some of those, this repo already has real CHANGELOG.rst history and no insertion marker, so nothing's at risk of being deleted -- but it does mean no changelog entries get generated going forward with no stated reason.
Unlike the #511 XBlocks track's adoption of ruff elsewhere (e.g. xblocks-core), this repo has no ruff config at all -- just noting the inconsistency, not asking for it here.

irfanuddinahmad · 2026-08-10T09:46:40Z

+]
+
+[tool.uv]
+package = true


A dev-named dependency-group exists (line 129), and every uv sync call actually used in this repo already names an explicit group (--group dev, --group ci) -- exactly the condition where default-groups = [] is needed. Without it, uv sync --group ci in ci.yml also implicitly syncs the full dev superset (which includes quality+doc+ci) alongside it, installing sphinx/pycodestyle/pydocstyle/edx-lint/isort into what's meant to be a minimal CI job.

Suggested change

package = true

package = true

default-groups = []

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

- Keep setup-uv over setup-python (our modernization) - Keep pypi-publish.yml deleted (replaced by release.yml) - Keep requirements/ deleted (replaced by uv dependency groups) - Take actions/checkout v7.0.1 bump from upstream Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

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

feanil

Looks good, seems like it will be good to merge once the merge conlficts are resolved.

salman2013 and others added 3 commits July 15, 2026 00:17

chore: migrate package metadata to pyproject.toml (PEP 621)

f0ad6da

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

chore: switch dependency management from pip-tools to uv

eb6a9c7

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

chore: add python-semantic-release and commitlint workflows

4dd760c

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

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 14, 2026

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

openedx-webhooks added this to Contributions Jul 14, 2026

salman2013 mentioned this pull request Jul 14, 2026

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

Open

16 tasks

salman2013 and others added 2 commits July 15, 2026 00:27

fix: add blank line in workbench/__init__.py to satisfy isort

a6f42e9

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

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

chore: switch PyPI publishing to OIDC trusted publisher

2ed3161

salman2013 and others added 2 commits July 17, 2026 21:58

refactor: migrate to src layout

600a60a

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

chore: standardize release.yml workflow

6bad88d

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

salman2013 force-pushed the salman/modernize-python-tooling branch from 24e1715 to 6bad88d Compare July 17, 2026 17:34

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

farhan requested changes Jul 21, 2026

View reviewed changes

farhan requested changes Jul 28, 2026

View reviewed changes

irfanuddinahmad reviewed Aug 10, 2026

View reviewed changes

salman2013 and others added 3 commits August 11, 2026 16:33

fix: address PR review feedback on modernize-python-tooling migration

c2b1f42

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

fix: use lowercase LICENSE.txt in MANIFEST.in

b75cd9b

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

salman2013 requested a review from farhan August 12, 2026 10:54

feanil approved these changes Aug 13, 2026

View reviewed changes

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

	"" = ["tests", ".tests", "spec", ".spec*"]
	"" = ["tests", ".tests", "spec", ".spec", "test/", "/test/"]

	uses: actions/upload-artifact@v4
	uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4

	uses: actions/download-artifact@v4
	uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4

Uh oh!

Conversation

salman2013 commented Jul 14, 2026

Uh oh!

openedx-webhooks commented Jul 14, 2026

Uh oh!

mphilbrick211 commented Jul 15, 2026

Uh oh!

irfanuddinahmad commented Jul 16, 2026

Uh oh!

farhan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farhan Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

irfanuddinahmad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feanil left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

farhan left a comment •

edited

Loading

farhan Jul 21, 2026 •

edited

Loading