Skip to content

Conversation

@bkeryan
Copy link
Collaborator

@bkeryan bkeryan commented May 10, 2025

What does this Pull Request accomplish?

Create a composite action wrapper for setup-python:

  • Single-source default Python version

Replace https://github.com/Gr1N/setup-poetry with a composite action, setup-poetry:

  • Single-source default Poetry version
  • Install Poetry manually. I originally implemented this using pipx, but ran into three problems:
    • GitHub-hosted runners have pipx installed in /opt/pipx and C:\Program Files (x86)\pipx, which cannot be overwritten by actions/cache. The workaround for this is to set PIPX_HOME and PIPX_BIN_DIR.
    • pipx on Windows was trying to use the py launcher, but I think this was a side effect of mixing up / and \ in my environment variables.
    • The pipx dir is a shared resource, and caching it in this action will do the wrong thing if the workflow uses pipx to install other Python commands. Installing Poetry manually allows us to keep it completely isolated, for caching purposes. The downside is that we have to deal with venv/bin vs. venv/Scripts and create our own symlinks.
  • Specify the Python version for Poetry so we don't have to do poetry env use python
  • Cache the Poetyr bin/home directories to speed up workflows

Why should this Pull Request be merged?

This week's Poetry + virtualenv problems have me thinking about Poetry install reproducibility.

What testing has been done?

Many, many, PR builds

@bkeryan bkeryan requested a review from csjall as a code owner May 10, 2025 17:52
@github-actions
Copy link
Contributor

github-actions bot commented May 10, 2025

Test Results

   10 files  ±0     10 suites  ±0   28s ⏱️ -2s
  573 tests ±0    573 ✅ ±0  0 💤 ±0  0 ❌ ±0 
5 730 runs  ±0  5 730 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 3e9a167. ± Comparison against base commit 13cbe86.

♻️ This comment has been updated with latest results.

@bkeryan bkeryan closed this May 10, 2025
@bkeryan bkeryan reopened this May 10, 2025
@bkeryan bkeryan changed the title github: Replace Gr1N/setup-poetry with pipx github: Replace Gr1N/setup-poetry with a composite action that uses pipx and supports caching May 10, 2025
@bkeryan bkeryan closed this May 10, 2025
@bkeryan bkeryan reopened this May 10, 2025
@bkeryan bkeryan requested a review from mshafer-NI May 10, 2025 19:32
@bkeryan
Copy link
Collaborator Author

bkeryan commented May 10, 2025

@mshafer-NI Do you have any feedback on this?

I would like to start using this approach in nidaqmx-python, measurement-plugin-python, ni-apis, etc.

Perhaps this composite action should be single-sourced in its own repo, or a repo for shared actions like an ni-github-actions or ni-python-actions.

I'm also considering splitting out the call to setup-python. I think we can figure this out without passing step outputs.

@bkeryan
Copy link
Collaborator Author

bkeryan commented May 10, 2025

What is the impact of caching the Poetry install? Let's compare https://github.com/ni/nitypes-python/actions/runs/14934916371 to https://github.com/ni/nitypes-python/actions/runs/14948460939/job/41994856909?pr=28.

  • The overall workflow duration went from 4 minutes to 2 minutes.
  • The Run unit tests (Windows) durations went from 1-2 minutes to 25-27 seconds.

path: |
${{ steps.set-pipx-paths.outputs.pipx-bin-dir }}
${{ steps.set-pipx-paths.outputs.pipx-home }}
key: pipx-${{ runner.os }}-py${{ steps.setup-python.outputs.python-version }}-poetry${{ inputs.poetry-version }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

setup-python sets the pythonLocation environment variable, so we don't really need to combine these actions.

@bkeryan bkeryan changed the title github: Replace Gr1N/setup-poetry with a composite action that uses pipx and supports caching github: Replace Gr1N/setup-poetry with a composite action that supports caching May 11, 2025
Copy link
Collaborator

@mshafer-NI mshafer-NI left a comment

Choose a reason for hiding this comment

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

(approve with the suggestion that we "standardize" these)

@@ -0,0 +1,58 @@
name: Set up Poetry
Copy link
Collaborator

Choose a reason for hiding this comment

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

proposal: we move this into a "ni/gh-actions" repo

@@ -0,0 +1,18 @@
name: Set up Python
Copy link
Collaborator

Choose a reason for hiding this comment

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

proposal: we move this into a "ni/gh-actions" repo

using: composite
steps:
- name: Set up Python
uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to have Renovate auto-update this??

shell: bash
- name: Cache poetry
id: cache-poetry
uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can Renovate auto-update this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bkeryan
Copy link
Collaborator Author

bkeryan commented May 12, 2025

I moved these actions to https://github.com/ni/python-actions/ and updated this repo to use them in #30

@bkeryan bkeryan closed this May 12, 2025
@bkeryan bkeryan deleted the users/bkeryan/pipx branch June 7, 2025 00:21
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