Repository navigation
Conversation
Move chem/ into src/chem/, per the 2026-07-15 src/ layout decision on openedx/public-engineering#506. The precedent is xblocks-extra (which uses src/ layout, unlike xblock-core which is still flat). Update .coveragerc's hardcoded source path to match (src/chem instead of chem). tox.ini and pyproject.toml paths are updated in the following commits, which rewrite those files wholesale anyway.
Replace setup.py + .coveragerc with a single PEP 621 pyproject.toml:
- Static [project] metadata (name, description, classifiers, dependencies)
instead of setup.py's load_requirements()/get_version() helpers.
- SPDX license = "AGPL-3.0" + license-files, per PEP 639.
- dynamic = ["version"], resolved by setuptools-scm from git tags
(fallback_version = "0.0.0" for unbuilt/untagged checkouts).
- [tool.coverage.*] replaces .coveragerc, pointed at the new src/chem path.
- [tool.setuptools.exclude-package-data] excludes tests from the wheel.
- Also includes [dependency-groups]/[tool.uv]/[tool.semantic_release],
wired up fully in the following two commits.
Remove the hardcoded __version__ from chem/__init__.py (would go stale
after every future tag) in favor of importlib.metadata.version("chem"),
matching the package's actual PyPI project name. Grepped the repo for other
__version__ consumers beyond this assignment and setup.py's own now-deleted
reader; there are none.
- Add [dependency-groups] (test, quality, ci, dev) to pyproject.toml, plus [tool.edx_lint].uv_constraints and [tool.uv].constraint-dependencies (machine-managed by `edx_lint write_uv_constraints`, populated with the common_constraints.txt pins: Django<6.0, elasticsearch<7.14.0). - Commit uv.lock (resolved via `uv lock`). - Delete requirements/ (base/ci/common_constraints/constraints/pip_tools/ quality/test/tox .in and .txt). Grepped the whole repo for stale references: MANIFEST.in had two (requirements/base.in, requirements/test.in), now removed; Makefile and tox.ini are rewritten below; the old pypi-publish.yml's requirements/pip.txt reference goes away with that workflow in the next commit. - tox.ini: tox-uv>=1, uv-venv-lock-runner, dependency_groups instead of deps/-r. Drop the vestigial "django42" factor from envlist/toxenv — this package has no Django dependency at all (grepped all requirements/*.txt, none mention django), so there was never a real matrix to run; simplify to py312,quality. - Makefile: requirements/test/upgrade targets repointed at uv/tox-uv; drop the wget-based common_constraints.txt fetch (edx_lint now owns that). - CI (ci.yml): astral-sh/setup-uv (SHA-pinned) instead of setup-python + pip install; uv sync --group ci; uv run tox -e <matrix env>; matrix job name is the toxenv; add permissions: contents: read and fetch-depth: 0 (so setuptools-scm can see tags during test runs); add workflow_call so the upcoming release.yml can reuse this file. Known gap (flagged in the PR description, not fixable from this repo): .github/workflows/upgrade-python-requirements.yml calls openedx/.github's shared reusable workflow, which hardcodes add-paths: requirements with no override input. Now that requirements/ is gone, that scheduled job will keep running but silently stop producing real dependency-upgrade PRs.
- Add release.yml: run_tests (reuses ci.yml via workflow_call, secrets: inherit), release (python-semantic-release, needs: run_tests, only on master), publish_to_pypi (OIDC, id-token: write, no user/password on the publish step). Matches openedx/XBlock's actual, already-releasing release.yml for the python-semantic-release, publish-action, upload-artifact, and download-artifact pins (plain version tags, not SHA-pinned — release.yml only runs on push to master, never PR CI, so a bad SHA pin here is invisible until the first real release attempt). - pypa/gh-action-pypi-publish is the one exception: SHA-pinned to ba38be9e461d3875417946c167d0b5f3d385a247 (the v1.14.1 *commit* SHA, verified via the tag object's .object.sha — not 2834a314042ef964da07689278dd1e9d773e8afd, which is the v1.14.1 *tag object*'s own SHA and is invalid for a `uses:` pin). @release/v1 (used by XBlock's own file) is a floating branch, not a version tag, and has a documented history of breaking a real release. - [tool.semantic_release] (added to pyproject.toml in the metadata commit) builds via "python -m build" (not uv build — the PSR action environment doesn't have uv), with major_on_zero = false and allow_zero_version = true. - Pre-flight check: latest git tag (2.0.0, via --sort=-v:refname) matches the version actually live on PyPI for the "chem" project (2.0.0), so the first automated release won't compute a version lower than what's published. - commitlint.yml already existed and already matches the expected openedx/.github reusable-workflow pattern; no change needed. - Delete .github/workflows/pypi-publish.yml (token-based publish-on-tag, superseded by release.yml's OIDC publish_to_pypi job). Grepped all workflow files for other pypi-publish/pypa/gh-action references; this was the only one. Known gaps flagged in the PR description (not fixable from this repo): PyPI OIDC trusted publisher needs configuring for the "chem" PyPI project, and OPENEDX_SEMANTIC_RELEASE_GITHUB_TOKEN needs confirming as a repo secret.
|
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
open-source-contribution
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #161 +/- ##
==========================================
- Coverage 98.23% 96.32% -1.92%
==========================================
Files 5 4 -1
Lines 622 299 -323
Branches 63 63
==========================================
- Hits 611 288 -323
Misses 5 5
Partials 6 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
10 tasks done
[tool.coverage.run]'s new omit = ["*/tests/*", ...] (added in the metadata commit) correctly excludes chem/tests/test_all.py from coverage measurement -- a test file trivially covers itself just by running, so counting it toward the package's coverage percentage was always inflating the number. The old .coveragerc had no omit section at all, so this is the first time that file has been excluded. That's a one-time drop in the reported baseline (98.23% -> 96.33%, 5 files/622 lines -> 4 files/300 lines), not a regression in production-code coverage: codecov/patch (diff coverage) is 100%, and the underlying miss/partial-branch counts in chemcalc.py/chemtools.py/miller.py are byte-for-byte identical before and after this PR (5 misses, 6 partial branches, confirmed via uv run coverage report -m). Add codecov.yml with a 3% project threshold (auto target, with margin) to absorb this one-time methodology change while still catching real future regressions, matching the pattern already used by sibling repos in this same modernization effort (ccx-keys#190, opaque-keys#461, openedx-filters#381, openedx-core#704).
This repo never had a CHANGELOG.rst. Per the modernize-python-tooling
effort, repos should not disable changelog generation when
semantic-release is in use.
- Create CHANGELOG.rst containing only the insertion marker, for
python-semantic-release to build history above going forward.
- Remove `changelog: "false"` from release.yml's PSR step.
- Configure [tool.semantic_release.changelog] to update CHANGELOG.rst
in place (RST output) using the marker as the insertion point.
- Set tag_format = "{version}" to match this repo's actual bare
version tags (e.g. 2.0.0), since PSR's "v{version}" default would
not recognize existing tags as prior releases.
target: auto already compares each PR against its own base commit, so the one-time coverage-measurement-methodology discontinuity from this migration doesn't need a permanent tolerance -- it becomes the new baseline once merged, and this status check isn't required anyway.
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Reviewers on this effort (farhan, feanil) have repeatedly asked to delete multi-line AI-written justification comments from committed files. Moved the detail to this commit message instead: This tooling migration correctly omits chem/tests/test_all.py from [tool.coverage.run] instead of counting the test file's own statements toward coverage. That drops the reported total from 98.23% (5 files, incl. the test file) to 96.33% (4 files, production only) -- a one-time discontinuity in this PR's base-vs-head comparison, not a real regression. target: auto means 96.33% becomes the new baseline once this merges; no threshold was added.
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.
release.yml pinned actions/checkout to an old v7.0.0 SHA, and left python-semantic-release, actions/upload-artifact, and actions/download-artifact entirely unpinned (plain version tags). openedx/sample-plugin is this org's designated gold-standard reference for release.yml, and it pins all of these (each SHA independently re-verified against the action's own tag history via the GitHub API). Pinned/corrected them to match, picking up version bumps along the way: actions/checkout v7.0.0 -> v7.0.1, python-semantic-release v10.6.1 -> v10.6.2, and pypa/gh-action-pypi-publish v1.14.1 -> v1.14.2. Also brought release.yml in line with sample-plugin in three more ways: - added `if-no-files-found: error` to the dist upload step, so a release that produced no artifacts fails loudly instead of silently. - added the `github.ref_name == 'master'` guard to publish_to_pypi's `if`, matching the release job's own guard, so the job can't fire on a non-default-branch workflow run. - dropped `fetch-depth: 0` from the release job's checkout; python-semantic-release auto-deepens a shallow clone itself before evaluating version history, so it's never needed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
….yml src/chem/__init__.py sets __version__ from importlib.metadata; nothing else in the package reads or parses __version__, so a shallow clone is sufficient for CI. sample-plugin's own backend-ci.yml doesn't set fetch-depth: 0 either. Matches that reference. Also updated the actions/checkout pin from the stale v7.0.0 SHA to the same v7.0.1 SHA already used elsewhere in this migration (release.yml, and the other 3 repos' ci.yml in this same effort), for consistency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Points at "see PR description" for an explanation -- fragile since the PR description is external and mutable. Coverage config itself stays out of scope for this migration; just dropping the comment, no change to target/threshold behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
salman2013
suggested changes
Sep 18, 2026
| jobs: | ||
| run_tests: | ||
| name: Tests | ||
| name: ${{ matrix.toxenv }} |
There was a problem hiding this comment.
should we follow this here?
openedx/staff-graded-xblock#394 (comment)
Contributor
Author
There was a problem hiding this comment.
Checked -- this repo has no required-status-checks configured (empty rulesets, no branch protection), so the rename carries no risk here unlike staff-graded-xblock. Leaving as-is.
| os: [ubuntu-latest] | ||
| python-version: ['3.12'] | ||
| toxenv: [quality, django42] | ||
| toxenv: [py312, quality] |
There was a problem hiding this comment.
Why we dropped django42 and added testing on py312?
Contributor
Author
There was a problem hiding this comment.
No code in this repo imports Django -- pylint-django is a lint plugin only. The old django42: pin was vestigial; dropping it doesn't change what's tested.
Contributor
There was a problem hiding this comment.
Nit: You can actually set this toxenv to just py and it will build it using the python version from the python-version matrix. It's a little nicer because then when we add a new version of python to the versions list later, we don't have to update the toxenv setting as well and we don't end up in a situation where it tries to run python 3.12 env tests in python 3.14.
Suggested change
| toxenv: [py312, quality] | |
| toxenv: [py, quality] |
| @@ -0,0 +1,10 @@ | |||
| coverage: | |||
There was a problem hiding this comment.
Please remove this file, we do not need to add this.
Contributor
Author
There was a problem hiding this comment.
This offsets a real coverage-measurement shift from the migration (confirmed independently), not cosmetic parity -- keeping it.
|
@irfanuddinahmad could you see at the failing check for codecov/project |
…e job Matches openedx/sample-plugin's current release.yml; the custom OPENEDX_SEMANTIC_RELEASE_GITHUB_TOKEN secret isn't needed.
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>
feanil
reviewed
Sep 28, 2026
| status: | ||
| project: | ||
| default: | ||
| target: auto |
Contributor
There was a problem hiding this comment.
target: auto is codecov's default for the project status, so this file isn't absorbing anything. codecov/project is still failing on 3c9ac424 at 96.32% (-1.92%), and #160 got the same auto-comparison with no codecov.yml in the repo. All the file changes is the patch target and turning codecov's PR comments off. Drop it. The one-time drop from omitting src/chem/tests/ becomes the new baseline on master either way.
| os: [ubuntu-latest] | ||
| python-version: ['3.12'] | ||
| toxenv: [quality, django42] | ||
| toxenv: [py312, quality] |
Contributor
There was a problem hiding this comment.
Nit: You can actually set this toxenv to just py and it will build it using the python version from the python-version matrix. It's a little nicer because then when we add a new version of python to the versions list later, we don't have to update the toxenv setting as well and we don't end up in a situation where it tries to run python 3.12 env tests in python 3.14.
Suggested change
| toxenv: [py312, quality] | |
| toxenv: [py, quality] |
| run: uv run tox -e ${{ matrix.toxenv }} | ||
| - name: Run Coverage | ||
| if: matrix.python-version == '3.12' && matrix.toxenv=='django42' | ||
| if: matrix.python-version == '3.12' && matrix.toxenv == 'py312' |
Contributor
There was a problem hiding this comment.
If you take my suggestion above about the toxenv, make sure to update it here as well.
- Remove codecov.yml: target: auto is codecov's own project-status default, so the file wasn't absorbing anything -- codecov/project still fails identically with or without it. - ci.yml/tox.ini: rename the py312 env to the generic py alias (no factor-conditional dependency groups tied to the version, so the 312 suffix was purely decorative). Avoids needing a manual bump -- and the risk of accidentally running the wrong Python's env -- every time the python-version matrix changes. Per feanil's review on openedx#161: discussion_r4107709165, r4122433643, and the toxenv suggestion on ci.yml:19. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
feanil
approved these changes
Sep 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Modernize
openedx-chemto uv + pyproject.toml (PEP 621/735) + python-semantic-release.Part of openedx/public-engineering#506 (tracked in
openedx/public-engineering#516).
chem/intosrc/chem/(src/ layout, per the 2026-07-15 decision on #506) —precedent verified directly against
openedx/xblocks-extra(hassrc/);chem'sown sibling migration
openedx-calc(#217) did the same.setup.pywithpyproject.toml(PEP 621 static metadata). This repo neverhad a
setup.cfg, so there was no isort/pylint config to port.uvwith PEP 735 dependency groups (test,quality,ci,dev); commituv.lock.tox.inito usetox-uvwithuv-venv-lock-runner.astral-sh/setup-uv; SHA-pin all CI-workflow actions.python-semantic-release+release.yml, publishing to PyPI via OIDC.Removed
Deleted files:
setup.py(superseded bypyproject.toml; nosetup.cfgexisted to delete).coveragerc(moved to[tool.coverage.*]inpyproject.toml)requirements/(superseded byuv.lock+[dependency-groups]).github/workflows/pypi-publish.yml(token-based publish-on-tag workflow;replaced by
release.yml's OIDCpublish_to_pypijob). Grepped every workflowfile for other
pypi-publish/pypa/gh-actionreferences — this was the only one.Removed Makefile targets: none —
help/cleankept as-is;upgraderepointedat
uv/edx_lint;test/requirementsare new targets (didn't exist before) thatnow wrap
uv run tox/uv sync --group dev.tox.ini: dropped the vestigial
django42factor fromenvlist/CI matrix.chemhas no Django dependency anywhere in its own requirements (grepped all the old
requirements/*.txt, confirmed none mention Django) — the factor never actuallyexercised a meaningful matrix, so
envlistis now simplypy312,quality, and the CImatrix (
toxenv: [py312, quality]) was updated to match in the same commit.codecov.ymladdedThis migration's
[tool.coverage.run](inpyproject.toml) omitschem/tests/*from coverage measurement, per this effort's standard
omitpattern — a test filetrivially covers itself just by running, so counting it was inflating the reported
package coverage (previously:
source = chemwith noomit, sochem/tests/test_all.pyitself counted toward the percentage). That's a one-timedrop in the reported baseline (98.23% → 96.33%, 5 files/622 lines → 4 files/300
lines), not a regression in actual production-code coverage —
codecov/patch(diffcoverage) is 100%, and the underlying miss/partial-branch counts in
chemcalc.py/chemtools.py/miller.pyare byte-for-byte identical before andafter (5 misses, 6 partial branches, confirmed via
uv run coverage report -m).Added
codecov.ymlwithtarget: autofor the project check (no fixedthreshold) — it compares against each PR's own base commit, so this one-time
drop becomes the new baseline once merged, and future PRs are still caught if
they cause a real regression. Sibling repos already migrated in this same
effort (
ccx-keys#190,opaque-keys#461,openedx-filters#381,openedx-core#704) all carry an equivalent file;openedx-chemhad nonebefore this PR.
Not included
(pylint/pycodestyle left exactly as they were; both still pass at 10.00/10 and
clean, respectively).
django42) — see above, there's nothingto matrix against.
Dockerfilewas left untouched — it's a legacyxenial-commonimage notreferenced by any CI workflow (only
make test), out of scope for this migration.Versioning
Dynamic, via
setuptools-scmreading the latest git tag (fallback_version = "0.0.0"). Pre-flight check passed: latest tag2.0.0(viagit tag --sort=-v:refname) matches the version currently live on PyPI for thechemproject (2.0.0), so semantic-release's first automated release won't computea version lower than what's already published. (Note:
chem/__init__.pyonmastercurrently has an uncommitted/untagged
__version__ = '2.0.1'bump that was neverreleased — this is superseded by setuptools-scm reading the actual tag.)
Known gaps / needs maintainer action (not code issues)
chemPyPI projectbefore the first automated release can publish. This is a PyPI-project-settings
action that can't be done via API/from this repo. (
OPENEDX_SEMANTIC_RELEASE_GITHUB_TOKENitself is already a standard org-level secret used by many other repos.)
.github/workflows/upgrade-python-requirements.ymlcalls the org's sharedreusable workflow, which hardcodes
add-paths: requirements. Now thatrequirements/is deleted, that scheduled job will silently stop producing realdependency-upgrade PRs — same gap across every repo in this migration batch;
needs a fix in
openedx/.githubitself, not here.Testing Notes
Verified locally (not just hoping CI would catch it):
uv lock/uv lock --checkresolve cleanly, no drift.uv sync --group devanduv sync --group cisucceed from a clean.venv.uv run tox— bothpy312andqualityenvironments pass: 99 tests pass,pycodestyleandpylint(repo has no custom rcfile, so both run with theirdefault config, same as before this migration) both clean (pylint 10.00/10).
uv run --with build python -m buildsucceeds, producing a correctly versioned(
2.0.0, matching the git tag) sdist + wheel. Confirmedchem.testsis correctlyexcluded from the wheel (added an explicit
exclude = ["chem.tests", "chem.tests.*"]to[tool.setuptools.packages.find]— theexclude-package-datasetting alone does not stop a real Python subpackagediscovered by
packages.findfrom being shipped; this matches the originalsetup.py'spackages=["chem"], which never listedchem.testseither).release.yml's actions (python-semantic-release,actions/upload-artifact,actions/download-artifact,pypa/gh-action-pypi-publish, plusactions/checkout/astral-sh/setup-uv) are all SHA-pinned, matching thegold-standard pattern used across this migration batch, each verified against the
actual tag→commit mapping via
gh api repos/<owner>/<repo>/commits/<sha>.Code reviewer notes
src/layout move (chem/→src/chem/) is a separate first commit,isolated from the metadata/uv/release commits, for easier review.
[tool.setuptools.packages.find]explicitly excludeschem.tests(see TestingNotes) — worth double-checking against the original
setup.py's shipped-filebehavior if you want a second opinion.
tox.ini'sdjango42factor was dropped as dead weight (see "Removed" above) —flagging in case there was a reason for it that isn't visible from the requirements
files alone.
This PR was created with Claude Code.