Merge wheels publish workflows

[wence-]

Resolves #78

[wence-]

cc @shwina This is where I got to.

[wence-]

One thing to note is that the matrix_filter approach is not likely to be easy for most devs to use (transforming json with jq is lower down the list than (say) transforming with python). It's also trial and error to know what the output should look like (I think I got it right...

[wence-]

Repushed rapidsai/cudf#13352 to test this branch. I guess I will fix the merge conflicts on top of 23.08...

[vyasr]

I just repointed your PRs to 23.08 instead of 23.06 since we don't want to modify the 23.06 workflows this close to the release. Unfortunately needs one more round of conflict resolution, sorry!

Also, this is a breaking change to the workflow in that it will require all RAPIDS repositories to switch to using the new workflow names. To make this kind of change without disrupting ongoing work, we typically follow the following procedure:

1. Create PRs to each of the affected repositories that point to this branch of the shared workflows and contain the necessary changes
2. Finalize this PR
3. Merge the PRs to each of the affected repositories
4. Merge this PR, without deleting the branch
5. Create new PRs to each of the affected repos repointing to the main branch of the shared-action-workflows repo
6. Once those PRs are merged, this feature branch can be safely merged.

Does that make sense?

[wence-]

In this case, since we're consolidating files, we could do it as a shorter process:

1. Don't delete the old workflows (so after merging everything still works)
2. Validate that the new workflows work on some repos...
3. Merge the new workflows
4. Separate PR to delete the old workflows once all the downstream affected repos have been modified.

WDYT?

[vyasr]

That process also works for me. If you want to go that route then I think the conflict resolution right now would be easy too.

[wence-]

OK, I've fixed up the conflicts and ported over the changes for 23.08. So I think this is OK. For reference, here is now the diff between the "manylinux" and "unified" versions:

test

diff -u /home/wence/Documents/src/rapids/first-party/shared-action-workflows/.github/workflows/wheels-manylinux-test.yml /home/wence/Documents/src/rapids/first-party/shared-action-workflows/.github/workflows/wheels-test.yml
--- /home/wence/Documents/src/rapids/first-party/shared-action-workflows/.github/workflows/wheels-manylinux-test.yml	2023-05-24 18:55:18.912442474 +0100
+++ /home/wence/Documents/src/rapids/first-party/shared-action-workflows/.github/workflows/wheels-test.yml	2023-05-24 19:01:12.252696999 +0100
@@ -1,4 +1,4 @@
-name: Test RAPIDS manylinux wheels
+name: Test RAPIDS wheels
 
 on:
   workflow_call:
@@ -21,8 +21,14 @@
         required: true
         type: string
       matrix_filter:
+        # jq expression to filter matrix entries.
         type: string
         default: "."
+      pure_wheel:
+        # If true, filters out the matrix to a smaller set.
+        required: false
+        type: string
+        default: "false"
 
       # test settings
       test-docker-options:
@@ -106,9 +112,16 @@
           TEST_MATRIX=$(yq -n 'env(MATRICES) | .[strenv(BUILD_TYPE)]')
           export TEST_MATRIX
 
+          echo ${{ inputs.pure_wheel }}
+          if [[ ${{ inputs.pure_wheel }} == "true" ]]; then
+              pure_wheel_filter='[.[] | select(.arch == "amd64" and .image == "ubuntu18.04" and ."test-type" == "unit")]'
+          else
+              pure_wheel_filter='.'
+          fi
+          echo ${pure_wheel_filter}
           echo "MATRIX=$(
             yq -n -o json 'env(TEST_MATRIX)' | \
-            jq -c '${{ inputs.matrix_filter }} | {include: .}' \
+            jq -c "${{ inputs.matrix_filter }} | ${pure_wheel_filter} | {include: .}" \
           )" | tee --append "${GITHUB_OUTPUT}"
 
   wheel-test:

Diff finished.  Wed May 24 19:03:37 2023

publish

diff -u /home/wence/Documents/src/rapids/first-party/shared-action-workflows/.github/workflows/wheels-manylinux-publish.yml /home/wence/Documents/src/rapids/first-party/shared-action-workflows/.github/workflows/wheels-publish.yml
--- /home/wence/Documents/src/rapids/first-party/shared-action-workflows/.github/workflows/wheels-manylinux-publish.yml	2023-05-24 18:55:18.912442474 +0100
+++ /home/wence/Documents/src/rapids/first-party/shared-action-workflows/.github/workflows/wheels-publish.yml	2023-05-24 19:02:29.881635219 +0100
@@ -1,4 +1,4 @@
-name: Publish RAPIDS manylinux wheels
+name: Publish RAPIDS wheels
 
 on:
   workflow_call:

Diff finished.  Wed May 24 19:03:57 2023

[vyasr]

Couple echos to remove, and would like @ajschmidt8 to verify that the jq command looks reasonable. Assuming that's all good, we can start opening PRs to other repos to use these workflows. Given our proposed strategy I'm fine with merging this PR once we have the cudf testing PR passing CI. We can work on other repos once this is merged.

[ajschmidt8]

Before we commit the pure wheel jq filter code, is there any reason that we can't run pure wheels against the entire test matrix?

[wence-]

Before we commit the pure wheel jq filter code, is there any reason that we can't run pure wheels against the entire test matrix?

There's no reason we can't I think. I assumed that this was done to minimise CI resource usage at some point in the past, so just ported this filtering over from the previous workflows.

[vyasr]

@wence- is correct. There is no reason that we can't, but there's also no benefit to doing so. It's actually kind of wasteful that we build conda packages that are Python version specific when they're identical.

[ajschmidt8]

@wence- is correct. There is no reason that we can't, but there's also no benefit to doing so. It's actually kind of wasteful that we build conda packages that are Python version specific when they're identical.

That's a good point.

I do see that dask-cudf has conda package for both 3.9 and 3.10: https://anaconda.org/rapidsai-nightly/dask-cudf/files

I'm wondering if we should just do the same for the pure-wheels (build both 3.9 and 3.10) even though it's wasteful since it seems to be the status quo for conda.

Then in the future, we can circle back and figure out a way to dedupe both the extraneous conda and wheel builds.

Thoughts on this approach?

[vyasr]

We discussed this offline. I'm fine with removing the filter now and doing some duplicate work. In one sense the current matrix isn't sufficient for pure wheels since we should still test on all supported versions and we currently aren't, so removing the constraint and using the full matrix would increase our test coverage.

I say we go with AJ's suggestion to do the extra build for now in order to get this PR finished, then come back and remove the duplicate build for both conda and pip in a future task.

[wence-]

We discussed this offline. I'm fine with removing the filter now and doing some duplicate work. In one sense the current matrix isn't sufficient for pure wheels since we should still test on all supported versions and we currently aren't, so removing the constraint and using the full matrix would increase our test coverage.

We can leave the filter in for the test phase, but I need to expand the build matrix for pure wheels include 3.9 and 3.10 (so that the matrix is aligned with what test sees). Let me try that...

[vyasr]

My point was that not filtering for the tests makes more sense than not filtering for the build, so I'd be OK not filtering either (assuming the test matrix is all valid combinations).

[vyasr]

In light of #97 and #116 the scope of this PR has been reduced to only include the publish workflow.

[vyasr]

Retargeting this to merge into my PR for repo-local testing. Repo-local building is already merged, so with this PR merged into mine we'll be able to test the entire pipeline with a single PR to each repo.