Skip to content

Cleanup from UV migration + standardization of workflows and package config#336

Merged
bjester merged 8 commits into
learningequality:release-v0.9.xfrom
bjester:uv-wrapup
Jun 12, 2026
Merged

Cleanup from UV migration + standardization of workflows and package config#336
bjester merged 8 commits into
learningequality:release-v0.9.xfrom
bjester:uv-wrapup

Conversation

@bjester

@bjester bjester commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

  • Restructures dependencies in pyproject.toml, making dev and test dependency groups instead of optional, leaving postgres and accelerated as optional
  • Updates instructions and workflows to install groups as necessary
  • For python < 3.9 where we use tox < 4, it overcomes lack of dependency group support by using uv to install the group deps into the system packages, then allows tox to use those
  • Reorganizes skip checks to the job level instead of step level
  • Adds concurrency control to cancel in-progress actions when PR is updated
  • Adds action linting
  • Fixes issues flagged by action linting, except one that is ignored
  • Updates dependabot cadence to check monthly instead of weekly
  • Updates import lint/formatting rule to enforce single-line imports
  • Reformats python files due to above change
  • Adds fan-in summary job for a single required PR check
  • Ensures all actions use full explicit versions, except duplicate check (TODO: Replace fkirc/skip-duplicate-actions with a reusable skip-on-paths workflow .github#83)

Reviewer guidance

  • Code reformatting looks okay?
  • CI passes?

AI Usage

Had AI start the process, then wrapped it up myself, as there were challenges with python+tox+pip versioning and other stuff was just copy & paste.

@bjester bjester force-pushed the uv-wrapup branch 3 times, most recently from 4d3b79a to 6d628c3 Compare June 12, 2026 16:57
@bjester bjester marked this pull request as ready for review June 12, 2026 18:03
@bjester bjester requested a review from rtibblesbot June 12, 2026 18:07
@bjester bjester changed the title Uv wrapup Cleanup from UV migration + standardization of workflows and package config Jun 12, 2026

@rtibblesbot rtibblesbot left a comment

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.

Clean tooling modernisation — migrating test/dev/docs to PEP 735 [dependency-groups], upgrading pre-commit hooks, and improving CI hygiene. All 28 CI checks pass.

No UI files changed; Phase 3 skipped.

Two nitpicks: a pair of redundant step-level skip conditions that the refactor missed cleaning up in check_migrations_sqlite.yml, and inconsistent action-version pin style in the new pre-commit.yml. Everything else looks solid.

backend review: This PR is a clean, mechanical refactor: the vast majority of changes are import statement reformatting enforced by ruff's new force-single-line = true isort setting, applied consistently to all ~80 Python files. No Python backend logic was changed — every diff in morango/ and tests/ is either a grouped-import split or a trivial formatting tweak (e.g. _self_ref_order field definition line-length). The .git-blame-ignore-revs entry correctly marks the reformatting commit.

The more substantive changes are in the build and CI layer: test/dev/docs groups migrate from [project.optional-dependencies] to PEP 735 [dependency-groups], which is the right separation (they're internal tooling deps, not published package extras). postgres and accelerated correctly remain as [project.optional-dependencies]. The dev group gains {include-group = "test"}, making it a proper superset. CI workflows gain concurrency-cancel blocks and move per-step if: skip_check guards to the job level, which is cleaner. EOL Python matrix entries gain --sitepackages + uv pip install --system --group test to work around uv's limitations with Python < 3.8.

Two minor issues found: an unpinned third-party action in the new pre-commit workflow, and two leftover step-level if conditions in check_migrations_sqlite.yml that are now redundant.

[nitpick] Redundant step-level skip conditions in migration check workflow

The migration_test job in check_migrations_sqlite.yml now has a job-level if: ${{ needs.pre_job.outputs.should_skip != 'true' }} (added by this PR), which means the job is skipped entirely when should_skip is true. Two step-level conditions that pre-date this PR were not cleaned up:

  • "Install build dependencies" step (line 55) still has if: ${{ needs.pre_job.outputs.should_skip != 'true' }}
  • "Run migrations" step (line 99) still has the same condition

These are now no-ops — the job never reaches them when skipping. All other workflows had their step-level conditions promoted to the job level correctly; these two were missed. Safe to remove both.

[praise] EOL Python workaround is well-crafted

The approach for Python < 3.9 (tox < 4, which doesn't support dependency_groups) — pre-installing test deps into the system Python via uv pip install --system --group test and then passing --sitepackages to tox — is a clean, minimal workaround with no footguns. It avoids duplicating the dependency list and stays consistent with how modern Python versions get their deps. The same pattern is applied uniformly across EOL unit tests, EOL cryptography tests, Windows, and migration checks, and CI confirms it works across Python 3.6–3.8.

[praise] Skip-condition refactor improves CI clarity

Moving if: ${{ needs.pre_job.outputs.should_skip != 'true' }} from every individual step up to the job level is the correct pattern: it prevents GitHub from even starting the job (saving runner minutes and reducing log noise) rather than running the job and skipping each step individually. The change is applied consistently across tox.yml, check_migrations_sqlite.yml, and check-docs.yml.

[praise] git-blame-ignore-revs housekeeping

Adding the import-reformatting commit to .git-blame-ignore-revs is good housekeeping — git blame --ignore-revs-file .git-blame-ignore-revs (and GitHub's blame UI, which respects this file) will skip over the mechanical reformatting and show the original authors.

[praise] PEP 735 dependency groups migration is correct

test, dev, and docs moving from [project.optional-dependencies] to [dependency-groups] is the right call: these are internal tooling deps that shouldn't be exposed as published package extras, and uv's native group support is the proper mechanism. postgres and accelerated correctly remain as optional-dependencies since they change runtime behaviour for consumers. The {include-group = "test"} entry in dev is a clean DRY improvement.

[praise] Import reformatting handled cleanly end-to-end

The force-single-line = true isort rule is introduced in pyproject.toml, applied mechanically by ruff, and the reformatting commit is registered in .git-blame-ignore-revs. Each import is now independently navigable in IDEs and diffs are more granular. The scope is complete — all Python files in the diff follow the new style.

Comments on lines not in diff:

.github/workflows/check_migrations_sqlite.yml:55[nitpick] Redundant step-level if conditions in check_migrations_sqlite.yml

The migration_test job gained a job-level if: ${{ needs.pre_job.outputs.should_skip != 'true' }} in this PR (line 49), which correctly prevents the entire job from running when the skip check fires. However, two step-level copies of the same condition were not removed:

  • Line 55: Install build dependencies step
  • Line 99: Run migrations step

All other equivalent step-level conditions were removed in this PR. These two are now dead code — the job won't reach them when should_skip == 'true'. Low impact, but inconsistent with the rest of the cleanup.


@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran a phased review pipeline over the pull request diff:

  • Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
  • Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
  • Specialized frontend/backend review passes applied framework-specific lenses where those files changed
  • For UI changes: manual QA and an accessibility audit against a live dev server, when available
  • Discarded findings citing files outside the diff; checked CI status and linked issue acceptance criteria
  • Calibrated the final verdict deterministically from finding severities, CI status, and QA evidence

Comment thread .github/workflows/pre-commit.yml
Comment thread .github/workflows/pre-commit.yml

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just one question - I didn't read the reformatting commit to save my brain, just assuming that ruff works as expected :)

Comment thread .github/workflows/tox.yml
@bjester bjester merged commit 638435f into learningequality:release-v0.9.x Jun 12, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants