CI: Attempt to ensure venv working after restore #318
Conversation
WalkthroughPropagates the resolved Python minor version from a cache-identify job to all downstream jobs in .github/workflows/verify.yml. Setup Python steps now consume the propagated version, cache keys include the minor version, and restore-venv calls are updated accordingly. The cache job step name reflects setup and minor-version determination. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Workflow Trigger
participant Cache as Job: cache-identify
participant Build as Job: build/test/…
participant Runner as Runner
Dev->>Cache: Start job
Cache->>Runner: actions/setup-python
Runner-->>Cache: Determine python minor (e.g., 3.x)
Cache-->>Build: Expose output python-version
Dev->>Build: Start downstream jobs (needs: cache)
Build->>Runner: actions/setup-python (version = needs.cache.outputs.python-version)
Build->>Runner: restore-venv (key includes python minor)
Runner-->>Build: Environment ready
Build-->>Dev: Execute steps with consistent Python minor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/actions/restore-venv/action.yml (4)
29-33: Harden the venv upgrade step: quote path and guard for existenceQuoting avoids path issues; guarding against missing venv prevents accidental creation or cryptic failures if the cache is inconsistent. Also echoing the Python version aids debugging.
- - name: Ensure virtual environment after restore - if: ${{ steps.cache-create.outputs.cache-hit == 'true' }} - shell: bash - run: | - python -m venv --upgrade ${{ inputs.venv-dir }} + - name: Ensure virtual environment after restore + if: ${{ steps.cache-create.outputs.cache-hit == 'true' }} + shell: bash + run: | + set -euo pipefail + echo "Using $(python --version) to upgrade venv" + if [[ -f "${{ inputs.venv-dir }}/pyvenv.cfg" ]]; then + python -m venv --upgrade "${{ inputs.venv-dir }}" + else + echo "No existing venv at '${{ inputs.venv-dir }}' (pyvenv.cfg missing); skipping upgrade." + fi
39-41: Keep venv path consistent: use inputs.venv-dir instead of hardcoded 'venv'Creation/activation paths should honor the input to avoid surprises and ensure cache and creation target the same directory.
- python -m venv venv - . venv/bin/activate + python -m venv "${{ inputs.venv-dir }}" + . "${{ inputs.venv-dir }}"/bin/activate @@ - . venv/bin/activate + . "${{ inputs.venv-dir }}"/bin/activateAlso applies to: 50-50
16-16: Typo in comment“DefauLt” → “Default”.
- default: "true" # DefauLt fail if not available + default: "true" # Default fail if not available
21-28: Ensure cache key includes runner.os and Python version — verification couldn't find workflow callersrg returned "No files were searched", so I couldn't locate any workflows that call ./.github/actions/restore-venv to confirm whether their cache keys include OS and Python version.
- File to review: .github/actions/restore-venv/action.yml (lines 21–28) — it currently uses: key: ${{ inputs.cache-key }}
- Risk: If callers don't include runner.os and the Python version in the cache key, a cached venv from a different OS/Python can be reused, causing subtle breakages.
- Recommended fixes (choose one):
- Update callers to append runner.os and Python version to the cache key, e.g.:
key: ${{ inputs.cache-key }}-${{ runner.os }}-python-${{ matrix.python-version }}- Or update the action to append runner.os and an input/python-version before calling actions/cache, so callers cannot omit it.
I couldn't confirm which workflows (if any) need changes — please re-run the search or grant access to workflow files so I can verify callers and propose a patch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/actions/restore-venv/action.yml(1 hunks)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #318 +/- ##
==========================================
- Coverage 80.58% 80.49% -0.10%
==========================================
Files 36 36
Lines 7547 7547
==========================================
- Hits 6082 6075 -7
- Misses 1465 1472 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Not sure why CI changes would change coverage ... good/feel-free to merge |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/verify.yml (1)
47-59: restore-venv action does not validate or use the python-version input — fix requiredShort summary: action.yml declares a required
python-versioninput but the action never uses it to select or validate the Python interpreter; it only echoes the input on failure. The action creates/activates a hardcodedvenvand usesinputs.venv-dironly for cache paths, so a Python-version mismatch won't trigger a rebuild.Files/locations to fix:
- .github/actions/restore-venv/action.yml
- inputs.python-version declared (lines ~4–6)
- cache paths use
${{ inputs.venv-dir }}(lines ~24–27, ~51–54)- venv creation uses
python -m venv venvand activation. venv/bin/activate(lines ~33–36, ~45–46)- failure message echoes
${{ inputs.python-version }}but does not validate it (lines ~57–60)Recommended changes:
- Use the
python-versioninput when creating or validating the venv (e.g., create venv with the requested interpreter:python3.X -m venv ${{ inputs.venv-dir }}or invoke the correctpythonbinary).- After restoring from cache, verify the venv's interpreter matches
inputs.python-version(checkvenv/bin/python --versionorpyvenv.cfg) and recreate the venv into${{ inputs.venv-dir }}if mismatched.- Ensure the action consistently uses
${{ inputs.venv-dir }}(not a hardcodedvenv) when creating/activating the environment.
🧹 Nitpick comments (2)
.github/workflows/verify.yml (2)
26-26: Good propagation of Python version across jobs; verify “minor vs patch” assumptionWiring
outputs.python-version: ${{ steps.python.outputs.python-version }}is correct. Note: setup-python’spython-versionoutput is the full version (major.minor.patch), while your comments refer to “minor”. If you intended to keep caches stable across patch updates, consider emitting a separateminor-versionoutput by trimming the patch component; otherwise, your caches will rotate on patch bumps.Also, for complete consistency with the PR objective (“Ensure all runners use THIS minor version”), the
test-publishingjob doesn’t set up Python and seeds a venv with whatever Python the runner has. If you want strict alignment, add a setup step there as well and pass it to uv (optional, but improves determinism).To add Python setup to the test-publishing job, insert a step like this before “Prepare uv” (outside the selected range, snippet for reference):
- name: Set up Python ${{ needs.cache.outputs.python-version }} uses: actions/setup-python@v5 with: python-version: ${{ needs.cache.outputs.python-version }} # And make uv use it explicitly: # uv venv --seed --python "$(python -c 'import sys; print(sys.executable)')" venv
47-52: Consistent consumption of propagated Python version across jobsUsing
needs.cache.outputs.python-versionfor setup across jobs is correct and ensures a single resolved version is used end-to-end.Minor consistency nit: for the restore-venv calls in these jobs you sometimes pass
steps.python.outputs.python-versionand in others the job output. They should be equivalent after setup, but picking one convention (prefer the job output for readability) would reduce cognitive overhead.Also applies to: 73-78, 114-119, 193-198, 243-248, 336-341
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/verify.yml(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run pytest using Python 3.13
🔇 Additional comments (2)
.github/workflows/verify.yml (2)
30-35: Setup step correctly establishes the canonical Python used by the workflowThis is the right place to canonicalize the version that downstream jobs will consume. Using
env.DEFAULT_PYTHONhere is clear and maintainable.
165-165: Matrix pytest: forcing restore to the installed interpreter is correctUsing
python-version: ${{ steps.python.outputs.python-version }}ensures the restored venv aligns with the matrix interpreter actually installed in the runner. The additional key suffix also cleanly separates caches per matrix entry.
Summary by CodeRabbit