Skip to content

tox: stop py3-unit re-pinning to Python 3.11, let the CI matrix actually run - #712

Open
UgaTheDev wants to merge 2 commits into
instructlab:mainfrom
UgaTheDev:fix/tox-py3-unit-basepython
Open

UgaTheDev wants to merge 2 commits into
instructlab:mainfrom
UgaTheDev:fix/tox-py3-unit-basepython

Conversation

@UgaTheDev

@UgaTheDev UgaTheDev commented Aug 24, 2026 •

Copy link
Copy Markdown

unit.yaml runs tox -e py3-unit across a matrix.python of 3.11, 3.12,
and 3.13 (three separate jobs, each with actions/setup-python installing
the matching interpreter). But [testenv:py3-unit] in tox.ini sets
basepython = python3.11, so tox re-pins every leg back to 3.11 regardless
of which interpreter setup-python put on PATH. All three matrix legs
currently exercise the same interpreter — the 3.12/3.13 legs are decorative.

This removes that pin. With no basepython override, tox's uv-venv-runner
uses the interpreter already on PATH for that job — i.e. whatever
actions/setup-python set up for the current matrix leg — so the three CI
jobs actually diverge.

Scope is intentionally narrow: only py3-unit. py3-smoke (tox.ini line 53)
keeps its basepython = python3.11 pin — it isn't part of the version matrix
in smoke.yaml/smoke-py312.yaml and shouldn't silently start floating.

Verified locally: tox -l still lists py3-unit with the same commands;
only the interpreter selection changes. No other env is touched.

Originally flagged in #705 (comment from 2026-07-29): "the 3.11/3.12/3.13 matrix is decorative: [testenv:py3-unit] in tox.ini sets basepython = python3.11, so tox re-pins the interpreter to 3.11 on all three legs no matter what setup-python installed."

Fixes the tox side of the CI-matrix finding in #705.

Summary by CodeRabbit

  • Chores
    • Updated the Python 3 unit-test environment to use tox’s default interpreter selection.

unit.yaml runs tox -e py3-unit across a matrix.python of 3.11/3.12/3.13,
but [testenv:py3-unit] pinned basepython to python3.11, silently
re-pinning every leg back to 3.11 regardless of what setup-python
installed. Removing the pin lets py3-unit use whatever interpreter is
already on PATH for that job, so the three CI legs actually diverge.
py3-smoke keeps its own basepython pin unchanged.

Ref: instructlab#705

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Kush Zingade <kush.zingade@gmail.com>
@coderabbitai

coderabbitai Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c89f1491-cb73-4bcf-8465-e4dffd05da78
Commits

Reviewing files that changed from the base of the PR and between d1123df and 0e7f5f3.

Files selected for processing: 1

  • tox.ini

No actionable comments were generated in the recent review.

Walkthrough

The py3-unit tox environment no longer sets basepython to Python 3.11. Tox now selects the interpreter by default.

Changes: Tox interpreter selection

Layer / File(s) Summary
Use tox default interpreter selection
tox.ini
The py3-unit environment no longer explicitly selects Python 3.11. Tox uses its default interpreter selection.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: Minimal · up to d1123

The change allows each py3-unit CI matrix job to use its configured Python version while leaving other environments unchanged; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: removing the Python 3.11 pin so the CI matrix can use each configured Python version.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches

🧪 Generate unit tests (beta)

  • Create PR with unit tests
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 50a42213-f3b2-4d65-80ba-d7815450ad7f

📥 Commits

Reviewing files that changed from the base of the PR and between da76404 and d1123df.

📒 Files selected for processing (1)

  • tox.ini
💤 Files with no reviewable changes (1)

  • tox.ini

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

@mergify mergify Bot added CI/CD Affects CI/CD configuration testing Relates to testing labels Aug 24, 2026
@UgaTheDev

Copy link
Copy Markdown
Author

All four of these (#712, #713, #714, #715) are mergeable with zero human reviews — and CodeRabbit rate-limited on three of them, so they've had no review at all. Since the constraint looks like review bandwidth rather than the changes themselves: want me to consolidate the four into a single PR so it's one review pass instead of four? They're independent commits, so either shape works — say the word and I'll have it up the same day.

@UgaTheDev

Copy link
Copy Markdown
Author

Friendly ping @RobotSail @Maxusmusti on this and the three related small PRs from the same pass: #713 and #715 (docs corrections) and #714 (the unreachable DeepSpeed+cpu_offload xfail). All four are green and mergeable. Happy to squash them into one if that is easier to review.

@UgaTheDev

Copy link
Copy Markdown
Author

@RobotSail @Maxusmusti one last check-in on #712–#715. All four are still mergeable and small (a tox pin, two doc corrections, and an unreachable xfail). If the repo isn't taking changes right now, let me know and I'll close them.

@mergify mergify Bot added the one-approval label Nov 5, 2026
@mergify

mergify Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@Maxusmusti

Copy link
Copy Markdown
Collaborator

Hi @UgaTheDev, thanks for opening these PRs, and apologies for the delay! This repo is currently transitioning into maintenance mode, but I will take a look at the PRs you mentioned

@Maxusmusti Maxusmusti left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this — the bug is real: with basepython = python3.11 on py3-unit, all three unit.yaml matrix legs run 3.11.

A few notes from verifying it:

This change hasn't actually been exercised by CI. The unit/lint/PyPI runs on this branch failed at startup ("workflow file issue", zero jobs created), so they don't appear in the PR checks. That looks repo-wide rather than caused by this PR (last successful unit.yaml run on main was 2026-06-30; possibly related to astral-sh/setup-uv@v7 being tag- rather than SHA-pinned, but I haven't confirmed the cause).

So I ran it locally, mirroring the workflow (venv on the matrix interpreter, then tox -e py3-unit, CPU-only):

leg interpreter tox used result
3.12 3.12.13 220 passed, 3 skipped
3.13 (setup-python-style interpreter first on PATH) 3.13.11 220 passed, 3 skipped
main (pin present), same layout 3.11.18 —

So the suite is green on 3.12/3.13 and the change behaves as intended on GitHub runners.

Caveat — interpreter selection is somewhat fragile. tox-uv asks uv to prefer system interpreters. In my first attempt the 3.13 interpreter was uv-managed, and tox silently fell back to the system python3 (3.12) even though 3.13 was first on PATH — no warning. It works on GitHub because setup-python's toolcache interpreters count as system ones. It might be worth adding a cheap sanity line (e.g. {envpython} -V before pytest in commands) so each matrix leg visibly reports the interpreter it ran on.

On leaving py3-smoke pinned: the description says it "isn't part of the version matrix", but smoke-py312.yaml exists specifically to test 3.12 — it passes python-version: 3.12 to the run-smoke action, which builds a 3.12 venv, but tox -e py3-smoke then runs pytest in tox's own env, which basepython = python3.11 pins to 3.11. So that job has the same decorative-version problem. Fine to keep this PR narrow, but worth a follow-up (and adjusting the description).

Makes each CI matrix leg show which Python it actually ran on, as
suggested in review.

Signed-off-by: Kush Zingade <kush.zingade@gmail.com>
@UgaTheDev

Copy link
Copy Markdown
Author

Thanks for verifying locally, and for the note on interpreter selection. I added {envpython} -V before pytest in commands so each matrix leg prints the interpreter it ran on. Agreed on py3-smoke: the 3.12 job is still pinned to 3.11 by basepython. I can open a follow-up for that and leave this PR narrow.

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

Labels

CI/CD Affects CI/CD configuration one-approval testing Relates to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants