Skip to content

build: Modernize xapi-db-load: migrate to uv, pyproject.toml, PEP 735 dependency groups - #258

Open
farhan wants to merge 8 commits into
openedx:mainfrom
farhan:farhan/modernize-python-repos
Open

build: Modernize xapi-db-load: migrate to uv, pyproject.toml, PEP 735 dependency groups#258
farhan wants to merge 8 commits into
openedx:mainfrom
farhan:farhan/modernize-python-repos

Conversation

@farhan

@farhan farhan commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Important

PR implemented with the assistance of Claude Code. Refined and validated before being submitted for code review.

Modernize `xapi-db-load`
Part of openedx/public-engineering#506

Summary

  • Replace `setup.py`/`setup.cfg` with `pyproject.toml` (PEP 621 static metadata)
  • Switch from pip-compile to `uv` with PEP 735 dependency groups; commit `uv.lock`
  • Retain pylint/isort/pycodestyle/pydocstyle/mypy as on master.
  • Coverage config moved into `pyproject.toml`
  • Update CI to use `astral-sh/setup-uv`; SHA-pin all actions
  • Fix `MANIFEST.in`: corrected `recursive-include xapi-db-load` (hyphen — never matched the package dir) to `xapi_db_load` (underscore); source distributions were previously missing all package data files
  • Add `quality` and `docs` envs to CI matrix (were not run on master)
  • Fix `tox -e docs` failures: mock `chdb` import, force-regenerate stale RST files, remove unsupported `logo_only` theme option
  • Fix `tox -e quality` failures: fix pre-existing pylint violations at source; suppress mypy errors for pre-existing code; add `build` to quality deps; enforce isort multi-line import style
  • Regenerate `pylintrc` with edx-lint 6.1.0 (was already pinned at 6.1.0 but `pylintrc` had been generated with 5.3.0)

Removed/Updated

Deleted files: `setup.py`, `setup.cfg`, `requirements/`, `.coveragerc`, `codecov.yml`

Updated Makefile targets:

Target Change
`upgrade` Replaced pip-compile workflow with `uv run edx_lint write_uv_constraints` + `uv lock --upgrade`
`requirements` Replaced pip-sync with `uv sync --group dev` + `uv tool install tox --with tox-uv`
`quality` Removed `*.py` glob from linter commands (`setup.py` was the only root-level `.py` file; glob would expand to nothing after deletion); replaced `python setup.py bdist_wheel` with `uv run python -m build`

Versioning

`version = "3.1.0"` declared directly in `pyproject.toml` — master had no PyPI publish workflow, so `setuptools-scm` is not used and the version is bumped manually on each release tag.

Quality / Docs CI

These envs were added to the CI matrix in this PR. All failures fixed are pre-existing (master never ran these checks in CI):

  • pylint: fixed violations at source where possible (sys.exit, super(), enumerate, generators, implicit booleanness, renamed built-in shadowing). For violations that require structural refactoring (abstract method hierarchies, intentional signature narrowing in xapi subclasses, pytest fixture patterns, circular import avoidance), added inline `# pylint: disable` comments at the specific location with a justification comment. No global suppression in `pylintrc_tweaks`.
  • mypy: added `ignore_errors = true` override for all `xapi_db_load.*` modules — master never ran mypy in CI and there are ~94 pre-existing type errors across the codebase. This allows mypy to remain configured and runnable without failing on pre-existing issues.
  • pycodestyle: added `W503` (line break before binary operator — deprecated by pycodestyle, conflicts with W504) and `W505` (doc line too long — already enforced by pylint's `max-doc-length`) to ignore list
  • isort: added `force_grid_wrap = 2` to preserve the existing multi-line-per-name import style; isort was previously collapsing multi-line imports onto one line when they fit within the 120-char limit, which conflicts with the style used throughout the codebase
  • docs: mocked `chdb` (broken on macOS but works in Linux CI), force-regenerated stale RST files with `sphinx-apidoc -f`

Important Notes

  • This repo does not publish to PyPI, so `src/` layout was not adopted.
  • This repo has no PyPI publish workflow on master, so `python-semantic-release` and `release.yml` were not added.
  • `codecov.yml` was deleted because it contained a hardcoded 74 % patch-coverage target that is not enforced by any current CI gate; Codecov config can be re-added with deliberate thresholds if desired.
  • `tox -e py312` fails locally on macOS because `chdb` crashes at import time on this platform (broken native extension). The same failure exists on master. CI runs on Linux where `chdb` works correctly.

Testing Notes

This PR has not been manually tested against the repo's own features. Testing relied on CI checks and local agent tooling (`make requirements`, `make lint`, `make test`, `python -m build`). Repo-owner is encouraged to run the repo's feature tests before merging.


🤖 Generated with Claude Code

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@farhan
farhan force-pushed the farhan/modernize-python-repos branch from 1cd60ea to fe06549 Compare July 23, 2026 17:11
@farhan farhan self-assigned this Jul 23, 2026
@farhan farhan moved this to 👀 In review in Aximprovements Team Jul 23, 2026
@farhan
farhan marked this pull request as ready for review July 23, 2026 17:21
@farhan farhan moved this to 👀 In review in Aximprovements Team Jul 24, 2026
@farhan
farhan force-pushed the farhan/modernize-python-repos branch from fe06549 to 48832c3 Compare July 24, 2026 11:39
@farhan farhan changed the title feat: Modernize xapi-db-load: migrate to uv, pyproject.toml, PEP 735 dependency groups build: Modernize xapi-db-load: migrate to uv, pyproject.toml, PEP 735 dependency groups Jul 24, 2026

@irfanuddinahmad irfanuddinahmad left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid migration overall — dependency list matches 1:1 with the old requirements/base.in, version handling is consistent, and I also want to flag a nice incidental fix: MANIFEST.in's recursive-include xapi-db-load ... (hyphen — never matched the real xapi_db_load package dir) is corrected to the underscore form. Worth a line in the PR description since it's a real behavior change, not just tooling.

One gap worth closing before merge:

tox.ini's envlist gains docs and quality (envlist = py312, docs, quality), and both env definitions are correctly wired to dependency_groups. But .github/workflows/ci.yml's matrix still only runs toxenv: [py]. So make quality (pylint/pycodestyle/pydocstyle/mypy/isort) and the docs build still never run in CI — same gap as before this PR, but now that the scaffolding exists, it'd be low-cost to close it. forum#283 (same author, same day) does add quality/docs to its CI matrix — worth matching that here for consistency.

@farhan

farhan commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review, @irfanuddinahmad!

Both points addressed in the latest commit:

  • CI matrix — added quality and docs to .github/workflows/ci.yml's toxenv list, matching the pattern from forum#283.
  • MANIFEST.in fix — added a bullet in the PR description calling it out as a real behavior change (source distributions were silently missing all package data files before).

- Replace setup.py/setup.cfg with pyproject.toml (PEP 621 static metadata)
- Switch from pip-compile to uv with PEP 735 dependency groups; commit uv.lock
- Update tox.ini to use tox-uv>=1 and uv-venv-lock-runner
- Update CI to use astral-sh/setup-uv; SHA-pin all actions
- Migrate .coveragerc config into pyproject.toml
- Update Makefile: upgrade → uv lock --upgrade; requirements → uv sync
- Remove requirements/ directory and stale config files
- Retain pylint/isort/pycodestyle/pydocstyle/mypy as quality linters
- Add static version = "3.1.0", replace hardcoded __version__ with importlib.metadata
- Remove setuptools-scm, dynamic version, and redundant base dependency group
- Fix sdist: include xapi_db_load/tests, prune example_configs/logs/private_configs
- Remove deprecated license classifier superseded by PEP 639
- Remove codecov.yml with invented patch coverage threshold
- Add doc8 to doc dependency group, remove twine duplicate

Part of openedx/public-engineering#506

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@farhan
farhan force-pushed the farhan/modernize-python-repos branch from 73bc979 to 216e624 Compare July 27, 2026 10:52
farhan and others added 2 commits July 27, 2026 16:20
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@farhan
farhan force-pushed the farhan/modernize-python-repos branch from 216e624 to a4779f4 Compare July 27, 2026 11:20
@FuaadZam FuaadZam moved this from 👀 In review to 🏗 In progress in Aximprovements Team Jul 27, 2026
- Mock chdb at autodoc time (fails to initialize on macOS)
- Add -f to sphinx-apidoc so stale RST files are overwritten on each build
- Remove unsupported logo_only theme option from sphinx_book_theme
- Fix get_config docstring RST indentation (unexpected indent warning)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@farhan
farhan force-pushed the farhan/modernize-python-repos branch from fa7dcbe to b20ffef Compare July 27, 2026 14:37
farhan and others added 3 commits July 27, 2026 20:05
edx-lint was already pinned at 6.1.0 in requirements but pylintrc was
last generated with 5.3.0. Re-running `edx_lint write pylintrc` brings
it in sync: updates the version header, adds broad-exception-raised to
the base disable list, and fixes overgeneral-exceptions to use the fully
qualified builtins.Exception name.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add force_grid_wrap = 2 to [tool.isort] so that any import with 2 or
more names is always formatted one-per-line, regardless of line length.
This preserves the existing style in the codebase and prevents isort
from collapsing multi-line imports into a single line.

Apply isort to reformat all files to be consistent with the new rule.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- tox.ini: ignore W503 (line break before binary operator) and W505
  (doc line too long) in pycodestyle, matching how E501 is handled
- pyproject.toml: add 'build' to quality dependency group (needed for
  'python -m build' in make quality); suppress mypy errors for all
  xapi_db_load.* modules since master never ran mypy in CI
- uv.lock: updated after adding 'build' to quality deps

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@farhan
farhan force-pushed the farhan/modernize-python-repos branch from d2b2c2a to ff8947e Compare July 28, 2026 07:19
Rather than globally suppressing pylint codes in pylintrc_tweaks,
fix each violation directly:

- Simple code fixes: sys.exit(), super(), consider-using-in,
  enumerate, generator expressions, implicit booleanness, renamed
  built-in 'id' -> 'tag_id', unused loop vars to '_'
- Added missing class/module docstrings throughout
- Inline pylint: disable comments with justification for violations
  that require structural refactoring to fix properly:
  abstract-method (intentional partial implementations),
  arguments-differ (intentional signature narrowing in xapi subclasses),
  too-many-positional-arguments (existing API surface),
  attribute-defined-outside-init (existing class structure),
  broad-exception-caught (intentional catch-alls),
  import-outside-toplevel (circular import avoidance),
  redefined-outer-name (standard pytest fixture pattern),
  protected-access (intentional test inspection)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@farhan
farhan force-pushed the farhan/modernize-python-repos branch from ff8947e to ab21d6d Compare July 28, 2026 07:40
@farhan

farhan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@irfanuddinahmad PR is available for the next pass

@farhan
farhan requested a review from irfanuddinahmad July 28, 2026 08:33
@farhan farhan moved this from 🏗 In progress to 👀 In review in Aximprovements Team Jul 28, 2026

@irfanuddinahmad irfanuddinahmad left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is green (7/7). The MANIFEST.in hyphen/underscore fix is a nice incidental catch (source dists were silently missing package data before). The previously flagged gap — quality/docs tox envs not wired into the CI matrix — is fixed; verified the matrix now includes toxenv: [py, quality, docs] at the PR head commit. LGTM.

@farhan farhan moved this from 👀 In review to 🛑 Blocked in Aximprovements Team Jul 31, 2026
@feanil
feanil self-requested a review August 5, 2026 12:58
Comment thread Makefile
pip install -r requirements/pip-tools.txt
pip-sync requirements/dev.txt requirements/private.*
uv sync --group dev
uv tool install tox --with tox-uv

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tool installs happen globally. We should just be including tox in the dev dependency group.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we do include tox in the dev group. So this should be fully unnecessary.

Comment thread pyproject.toml

# From requirements/doc.in
doc = [
{include-group = "test"},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need test requirements for the docs build?

Comment thread pyproject.toml

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you're missing setuptools-scm and semantic release here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🛑 Blocked

Development

Successfully merging this pull request may close these issues.

4 participants