ENH: Use pixi to run action as composite #91

matthewfeickert · 2024-09-19T20:40:54Z

Resolves #71

Description

Use pixi to manage the environment fully, which removes the needs for micromamba and a Linux container, allowing the action to be run as a 'composite' action.
- This allows the action to be used on macOS runners in addition to Linux.
- Add pixi project configuration (pixi.toml) and pixi lock file (pixi.lock).
- Add 'osx-64' and 'osx-arm64' platforms to pixi environment solves to allow for use on Linux and macOS runners.
- Use prefix-dev/setup-pixi GitHub action to setup pixi for the action.
- Explicitly enable the 'locked' option for prefix-dev/setup-pixi.
- Don't allow for post job cleanup to avoid errors if the action is run multiple times in a workflow (this is a downside of no longer being in a Linux container).
- Make cmd.sh executable (chmod +x).
Remove use of micromamba in the upload script.
Remove the conda-lock files and the Docker based locking workflow.
Remove Dockerfile.
Note in the README that the runner used can be either Linux or macOS.
Update version numbers of action to v0.5.0.
Rename script from cmd.sh to upload_wheels.sh.
- As the script is no longer the CMD of a Linux container but is now a standalone Bash script, having a more descriptive name is preferable.

Note

GitHub Actions takes care of cleaning up the environment after each run, so you are not responsible for manually unsetting all environment variables between steps to avoid secret leakage.

stefanv

😎

Dockerfile

default_linux-64_conda_spec.txt

stefanv · 2024-09-19T20:55:29Z

pixi.toml

@@ -0,0 +1,14 @@
+[project]
+authors = ["Matthew Feickert <matthew.feickert@cern.ch>"]
+channels = ["conda-forge"]


Dockerfile

bsipocz · 2024-09-19T20:59:25Z

Nitpicking: @matthewfeickert could we follow a fork-based workflow here in the future, so the main repo won't be peppered with feature branches?

Dockerfile

default_linux-64_conda_spec.txt

pixi.toml

matthewfeickert · 2024-09-19T21:11:56Z

could we follow a fork-based workflow here in the future, so the main repo won't be peppered with feature branches?

@bsipocz I would, but c.f. #81 (comment) for why if we want CI to run we can't. (I also don't find a transient feature branch terrible, but I fully agree that a fork based workflow is the way to go.)

bsipocz · 2024-09-19T21:19:40Z

I also don't find a transient feature branch terrible

The issue is that some of the branches are not exactly transient, but are around for a while when part of some experimenting or draft PR.

pixi.toml

bsipocz · 2024-09-19T21:23:27Z

Re CI: I think it's OK to separate the current workflow into two, one that runs all the anaconda upload/cleanup checks, and one that does everything else. Then we can run most of the tests from forks, too, and in practice those would cover most of the PR cases anyway.

bsipocz · 2024-09-19T21:25:31Z

(however, that said, dependabot PRs, from local feature branches are also failing with TOKEN issues: #86)

matthewfeickert · 2024-09-19T21:27:24Z

Re CI: I think it's OK to separate the current workflow into two, one that runs all the anaconda upload/cleanup checks, and one that does everything else. Then we can run most of the tests from forks, too, and in practice those would cover most of the PR cases anyway.

@bsipocz I don't think that will work for the reasons I describe in the two previous responses I've linked, as you'll need secrets to run the upload and a secret to delete the files and neither can be run on a fork safely. But can you make an Issue to describe more what you have in mind here?

Full disclosure, there may be some clever (or obvious :P) way to get around this I've overlooked so far.

tupui · 2024-09-19T21:32:39Z

Cool 👍 Just a fly by comment. What do you think about uv otherwise? Looks like this is going to be the thing sooner than later.

bsipocz · 2024-09-19T22:00:48Z

I don't think that will work for the reasons I describe in the two previous responses I've linked, as you'll need secrets to run the upload and a secret to delete the files and neither can be run on a fork safely.

Yes, my point is to separate those that need the secret in a separate workflow/job, so 1) tests can pass on forks with the acknowledgement that the secret-requiring ones are skipped. (most of the times that approach should be good enough)

But also, something is broken with the current setup as e.g. #86 failed even though it run from a feature branch here.

matthewfeickert

Note

Though this is passing CI, I'm going to leave this in draft for a bit, to think if there's cleaner and safer ways to do everything, before I move it to 'ready for review' and ask people to spend time on it. That realistically won't be until next week though given that I have work (travel, talks, reviews) and personal deadline-driven tasks looming.

(I'll also want to rebase things and try to write more clear commit messages for reviewers.)

matthewfeickert · 2024-09-20T05:43:20Z

action.yml

+  using: "composite"
+  steps:
+    - name: Set up pixi
+      uses: prefix-dev/setup-pixi@ba3bb36eb2066252b2363392b7739741bb777659  # v0.8.1


I need to look at what the prefix-dev/setup-pixi action is actually doing to see if pinning at the hash level actually provides any safeguards. I'll also ask the pixi team to review this PR once it is ready to move from draft to review.

the setup-pixi action is designed to be backwards compatible (like pixi as well) but there are no guarantees yet.

with a proper dependabot configuration and automatic updates, i would just keep it pinned to the SHA level, though for additional reproducibility

Okay, thanks @pavelzw. That's useful info.

matthewfeickert · 2024-09-20T05:44:51Z

action.yml

+      uses: prefix-dev/setup-pixi@ba3bb36eb2066252b2363392b7739741bb777659  # v0.8.1
+      with:
+        cache: true
+        cache-write: ${{ github.event_name == 'push' && github.ref_name == 'main' }}


I'm not fully clear on how github.ref_name works when the action is being used by other workflows, so this might all be wrong. (Here I'm just copying from the https://github.com/prefix-dev/setup-pixi README).

This still holds true, but worst case scenario we cache for some and not others and I think that's okay.

matthewfeickert · 2024-09-20T05:48:08Z

action.yml

+        # Avoid post cleanup errors if action run multiple times
+        post-cleanup: false


If the action is run multiple times in the same workflow (like we do in the CI to test) then at the end of the workflow each of the job steps gets cleaned up sequentially. As

post-cleanup: description: | If the action should clean up after itself. Defaults to `true`. If `true`, the pixi environment, the pixi binary and the rattler files in ~/.rattler and ~/.cache/rattler are removed.

this will try to delete the same directory path multiple times and will error when those paths don't exist after the first clean up.

I'd like to find a smarter way around this, as I'm not sure if leaving things around is ideal, but I also don't think it is bad.

currently this is the only workaround. there are some nieche use cases where it actually makes sense to run setup-pixi twice. we might want to open an issue for this

matthewfeickert · 2024-09-20T05:48:55Z

action.yml

+        INPUT_ANACONDA_NIGHTLY_UPLOAD_TOKEN: ${{ inputs.anaconda_nightly_upload_token }}
+        INPUT_ANACONDA_NIGHTLY_UPLOAD_LABELS: ${{ inputs.anaconda_nightly_upload_labels }}
+      run: |
+        pixi run cmd.sh


cmd.sh is executable now (post chmod +x cmd.sh). We can also rename it to something more descriptive.

cmd.sh

matthewfeickert · 2024-09-20T05:51:59Z

pixi.toml

+channels = ["conda-forge"]
+description = "Environment for the scientific-python/upload-nightly-action GitHub action"
+name = "upload-nightly-action"
+platforms = ["linux-64", "osx-64", "osx-arm64"]


Adding the macOS platforms as though I assume that people will be using this following a cibuildwheel workflow on Linux, providing the extra platforms in the lock file allows for this action to be run on macOS runners as well.

matthewfeickert · 2024-09-20T06:12:54Z

What do you think about uv otherwise? Looks like this is going to be the thing sooner than later.

@tupui I like uv a lot and I use it on many projects. uv pip compile is excellent and I use it in container runtimes where I don't have control over the Python runtime given the base image environment (this is actually one area/thing that pixi doesn't really allow you to do as it needs to control the entire environment — some caveats there if you export the shell activation though and manually use it.)

That being said, I like pixi more in general, and I think for something like this where you want to have hash level reproducibility of the entire stack and not just of the Python packages then pixi is a clear winner (well really more just "locked conda installs in an isolated/containerized environment" is the winner).

tupui · 2024-09-20T08:24:46Z

Interesting thanks for the write up!

matthewfeickert

These are just some final comments in addition to the comments I left for others in my previous self-reviews.

matthewfeickert · 2024-09-25T15:54:20Z

README.md

@@ -4,7 +4,8 @@ This is a GitHub Action that uploads nightly builds to the [scientific-python ni
 as recommended in [SPEC4 — Using and Creating Nightly Wheels][].

 In a GitHub Actions workflow (`.github/workflows/*.yaml`), use the
-following snippet to upload built wheels to the channel:
+following snippet on a Linux or macOS runner to upload built wheels to the


Adding this as we've gotten questions in the past about what runner OS are supported and this PR extends support beyond Linux.

matthewfeickert · 2024-09-25T15:55:04Z

action.yml

+      uses: prefix-dev/setup-pixi@ba3bb36eb2066252b2363392b7739741bb777659  # v0.8.1
+      with:
+        cache: true
+        cache-write: ${{ github.event_name == 'push' && github.ref_name == 'main' }}


This still holds true, but worst case scenario we cache for some and not others and I think that's okay.

matthewfeickert · 2024-09-25T16:02:50Z

@scientific-python/nightly-wheels-developers this is now ready for review. As I had this in draft before to figure things out I've intentionally squashed and rebased this so that it will be easier to follow my actual intended changes from both the GitHub UI and from a local checkout.

@pavelzw @ruben-arts as you've contributed the most to the setup-pixi action if you're able to review this as well that would be very much appreciated.

edit: Also pinging @stefanv too as he's not on the @scientific-python/nightly-wheels-developers list but was kind enough to give an initial review. :)

action.yml

matthewfeickert · 2024-09-27T14:54:58Z

Thanks for reviewing for @scientific-python/nightly-wheels-developers, @MridulS! 🙏 If we're able to get one more review from another @scientific-python/nightly-wheels-developers person that would be great, but if no one else reviews by Tuesday (2024-10-01) I'll go ahead and squash and merge.

bsipocz

Looks good to me, thank you @matthewfeickert for the refactor!

matthewfeickert · 2024-09-27T19:35:58Z

Thanks @bsipocz!

I'm going to do a relock and it there's any changes to a rebase and push and then squash and merge this. I'll then update the v0.5.0 info in the repo to v0.6.0 and make a new release on GitHub.

* Use pixi to manage the environment fully, which removes the needs for micromamba and a Linux container, allowing the action to be run as a 'composite' action. - This allows the action to be used on macOS runners in addition to Linux. - Add pixi project configuration and pixi lock file. - Add 'osx-64' and 'osx-arm64' platforms to pixi environment solves to allow for use on Linux and macOS runners. - Use prefix-dev/setup-pixi GitHub action to setup pixi for the action. - Explicitly enable the 'locked' option for prefix-dev/setup-pixi. - Don't allow for post job cleanup to avoid errors if the action is run multiple times in a workflow (this is a downside of no longer being in a Linux container). - Note: GitHub Actions takes care of cleaning up the environment after each run, so you are not responsible for manually unsetting all environment variables between steps to avoid secret leakage. - Make cmd.sh executable (chmod +x). * Remove use of micromamba in the upload script. * Remove the conda-lock files and the Docker based locking workflow. * Remove Dockerfile. * Note in the README that the runner used can be either Linux or macOS. * Update version numbers of action to v0.5.0.

* As the script is no longer the CMD of a Linux container but is now a standalone Bash script, having a more descriptive name is preferable.

matthewfeickert self-assigned this Sep 19, 2024

stefanv reviewed Sep 19, 2024

View reviewed changes

matthewfeickert mentioned this pull request Sep 19, 2024

feat: Export conda explicit specification file from project prefix-dev/pixi#1873

Merged

matthewfeickert commented Sep 19, 2024

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

default_linux-64_conda_spec.txt Outdated Show resolved Hide resolved

pixi.toml Outdated Show resolved Hide resolved

matthewfeickert commented Sep 19, 2024

View reviewed changes

pixi.toml Outdated Show resolved Hide resolved

matthewfeickert temporarily deployed to remove-old-wheels September 20, 2024 05:11 — with GitHub Actions Inactive

matthewfeickert commented Sep 20, 2024

View reviewed changes

matthewfeickert force-pushed the feat/use-pixi-for-locking branch from 11b88f2 to ec7c415 Compare September 25, 2024 15:49

matthewfeickert changed the title ~~ENH: Use pixi to maintain lock file~~ ENH: Use pixi to run action as composite Sep 25, 2024

matthewfeickert temporarily deployed to remove-old-wheels September 25, 2024 15:50 — with GitHub Actions Inactive

matthewfeickert commented Sep 25, 2024

View reviewed changes

matthewfeickert marked this pull request as ready for review September 25, 2024 15:58

matthewfeickert requested a review from a team September 25, 2024 15:58

matthewfeickert requested a review from stefanv September 25, 2024 16:03

MridulS reviewed Sep 26, 2024

View reviewed changes

action.yml Show resolved Hide resolved

matthewfeickert commented Sep 26, 2024

View reviewed changes

action.yml Show resolved Hide resolved

MridulS approved these changes Sep 27, 2024

View reviewed changes

matthewfeickert temporarily deployed to remove-old-wheels September 27, 2024 14:53 — with GitHub Actions Inactive

bsipocz approved these changes Sep 27, 2024

View reviewed changes

matthewfeickert added 2 commits September 27, 2024 14:38

MNT: Rename script to 'upload_wheels.sh'

6f223fe

* As the script is no longer the CMD of a Linux container but is now a standalone Bash script, having a more descriptive name is preferable.

matthewfeickert force-pushed the feat/use-pixi-for-locking branch from 1f08c30 to 6f223fe Compare September 27, 2024 19:39

matthewfeickert temporarily deployed to remove-old-wheels September 27, 2024 19:40 — with GitHub Actions Inactive

matthewfeickert merged commit cb54b9a into main Sep 27, 2024
2 checks passed

matthewfeickert deleted the feat/use-pixi-for-locking branch September 27, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Use pixi to run action as composite #91

ENH: Use pixi to run action as composite #91

matthewfeickert commented Sep 19, 2024 •

edited

Loading

stefanv left a comment

stefanv Sep 19, 2024

bsipocz commented Sep 19, 2024

matthewfeickert commented Sep 19, 2024 •

edited

Loading

bsipocz commented Sep 19, 2024

bsipocz commented Sep 19, 2024

bsipocz commented Sep 19, 2024

matthewfeickert commented Sep 19, 2024 •

edited

Loading

tupui commented Sep 19, 2024

bsipocz commented Sep 19, 2024 •

edited

Loading

matthewfeickert left a comment •

edited

Loading

matthewfeickert Sep 20, 2024

pavelzw Sep 26, 2024

matthewfeickert Sep 26, 2024

matthewfeickert Sep 20, 2024

matthewfeickert Sep 25, 2024

matthewfeickert Sep 20, 2024

pavelzw Sep 26, 2024

matthewfeickert Sep 20, 2024

matthewfeickert Sep 20, 2024

matthewfeickert commented Sep 20, 2024

tupui commented Sep 20, 2024

matthewfeickert left a comment

matthewfeickert Sep 25, 2024

matthewfeickert Sep 25, 2024

matthewfeickert commented Sep 25, 2024 •

edited

Loading

matthewfeickert commented Sep 27, 2024

bsipocz left a comment

matthewfeickert commented Sep 27, 2024

		# Avoid post cleanup errors if action run multiple times
		post-cleanup: false

ENH: Use pixi to run action as composite #91

ENH: Use pixi to run action as composite #91

Conversation

matthewfeickert commented Sep 19, 2024 • edited Loading

Description

stefanv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz commented Sep 19, 2024

matthewfeickert commented Sep 19, 2024 • edited Loading

bsipocz commented Sep 19, 2024

bsipocz commented Sep 19, 2024

bsipocz commented Sep 19, 2024

matthewfeickert commented Sep 19, 2024 • edited Loading

tupui commented Sep 19, 2024

bsipocz commented Sep 19, 2024 • edited Loading

matthewfeickert left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewfeickert commented Sep 20, 2024

tupui commented Sep 20, 2024

matthewfeickert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewfeickert commented Sep 25, 2024 • edited Loading

matthewfeickert commented Sep 27, 2024

bsipocz left a comment

Choose a reason for hiding this comment

matthewfeickert commented Sep 27, 2024

matthewfeickert commented Sep 19, 2024 •

edited

Loading

matthewfeickert commented Sep 19, 2024 •

edited

Loading

matthewfeickert commented Sep 19, 2024 •

edited

Loading

bsipocz commented Sep 19, 2024 •

edited

Loading

matthewfeickert left a comment •

edited

Loading

matthewfeickert commented Sep 25, 2024 •

edited

Loading