ci: enable merge queue and run Miri only in the queue

[blaginin]

• Closes MIRI jobs consume a lot of GH runner action time #11045
• closes Use a merge queue #8422

Add merge queue to arrow-rs and move miri to run there (and not on every push)

[alamb]

Merge up to get the fix for audit failure

[alamb]

Thank you @blaginin -- I will be honest that this looks much more complicated than I was expecting

Is there some reason not to just change MIRI to run on pushes to main and merge_queue? That way we would save them running on PRs. It would still run about 2x on each PR (though sometimes the merge_queue one could be shared)

[blaginin]

Thanks for the review. I was trying to copy the approach from comet:

• ci: move the job routing policy out of ci.yml expressions and into compute-changes.py datafusion-comet#5850
• ci: add a Required Checks aggregator job so main can have a required status check datafusion-comet#5842
• ci: move the Spark 3.4/3.5/4.0, Iceberg, macOS and benchmark suites behind a merge queue datafusion-comet#5843

specifically, stuff like this is a bit tricky:

[blaginin]

That's like check-ci-config.py in Comet.

[blaginin]

That's like compute-changes.py in Comet.

[Jefffrey]

i wonder if we should try simplifying our CI routing first; for example, group parquet-variant checks into just parquet, and fix some triggers for parquet (i see theres a trigger for any arrow-avro changes but i dont think theres actually a dependency in code for this?)

i do agree this routing logic feels a bit heavy, especially given (as far as i understand) in comet its because they have a lot of heavy CI tasks so they need this granular dispatch, whilst for us here its mainly miri (and maybe the arrow integration) that are heavy

• feel free to correct me if im wrong here, not as familiar with comets setup

with datafusion, did it not have this routing logic in the first place (before merge queue was implemented there)?

[blaginin]

i wonder if we should try simplifying our CI

I did this, #11153 - but i think this could go separately? I get that python script is complicated but i think it would be good to stay consistent in arrow / datafusion CI? I made it a bit simplier in the commit on top

[Jefffrey]

it seems like it is? or am i looking at the wrong place here:

https://github.com/apache/infrastructure-actions/blob/99f9b4780c9d99446a1663dd6a4cfe526ecbc10d/actions.yml#L506-L514

[blaginin]

bad comment, i think it's more due to apache/infrastructure-actions#312 (comment) (we really tried, haha!)

[Jefffrey]

it looks like the mentioned PR is merged and available in latest release of path-filters now 👀

[blaginin]

you mean dorny/paths-filter#279? it is merged indeed but i think there will complicated / hard to make it work (apache/datafusion#21941 (comment))

also, it feels to me that it's better to stay consistent across the df / arrow projects? 😄

[Jefffrey]

i suppose we will go with this approach if the paths-filter isnt feasible; the amount of python code is manageable. my main concern is im a bit confused by how the routing logic is separated

as noted in the readme, based on the event we run:

• PR -> dev/rust/rustdoc + suite based on changed paths
• merge_group -> all suites (incl miri)
• main push -> all suites (excl miri)

but the logic around when to trigger miri is present in check-ci-config.py (its manually added in validate_config() for example), in compute-changes.py under the merge_group event check branch, in ci.yml since it follows the pattern of being triggered based on output of the changes job (even though in the python script its hardcoded to always be a suite returned), but then critically miri.yaml has its own check to run only in merge group.

its hard to keep track of, and i wonder if we should simplify it to only check in ci.yml if thats possible? for example

  miri:
    needs: changes
-   if: contains(fromJSON(needs.changes.outputs.suites), 'miri')
+   if: github.event_name == 'merge_group'
    uses: ./.github/workflows/miri.yaml

then similar for the suites which always run (dev/rust/rustdoc), we remove them from being hardcoded constants in the python scripts, and just in ci.yml always require them.

this way we can keep compute-changes.py to solely be about filtering suites for PR events

or was this current approach needed due to limitations in github actions?

[alamb]

i suppose we will go with this approach if the paths-filter isnt feasible; the amount of python code is manageable. my main concern is im a bit confused by how the routing logic is separated

I still don't understand the need for python code at all. I am probably missing something basic

It seems to me ideally what we would do is:

1. Run some subset of the CI tests on PRs
2. Run all the CI tests in the merge queue
3. Don't run CI tests explicitly on push to master (as they already run on the same commit in the merge queue)

[blaginin]

right, your understanding is correct!

the reason is that github uses the same set of "required" checks both before a pr enters the merge queue and while it's in the queue. You can't configure separate sets of required checks for these two stages in branch protection settings. Hence we have this python proxy to decide which checks need to run or not 🙂

[blaginin]

probably this one by mistake? 😄

[blaginin]

its hard to keep track of, and i wonder if we should simplify it to only check in ci.yml if thats possible? for example

OOo thanks @Jefffrey! I pushed a change - i think it's more closely aligned with your idea?

[Jefffrey]

(sorry for late review)

i do still have some reservations, particularly around how the python code seems to still encode configuration which can be a little confusing for discovery, and the addition of the cache refresh logic (which i understand why we have it but its a little messy)

but i dont want to keep blocking this PR, since it is a good idea to get merge queue in especially to start cutting down on our miri compute

do we have a sandbox to play with for future changes, similar to what datafusion had? id be interested in seeing if there are ways to try simplify this if possible, in followup PRs

[alamb]

I also worry about the complexity of this code / adding a custom python script into our build / CI triggering process.

[blaginin]

sadly i think this will have to be expressed in either python or bash if we want to run some things only in CI... i can suggest infra to move this into asf-yaml repo (given that we use this pattern in other repos as well), although not sure if we want to block on that?

[Jefffrey]

i can suggest infra to move this into asf-yaml repo (given that we use this pattern in other repos as well)

this would be pretty great if possible, but yes i dont think we should block this PR on that

if @alamb is fine with it im good with merging this PR as is

[Jefffrey]

thanks @blaginin & @alamb

lets see how this goes

[Jefffrey]

ive created a followup issue based on the discussion here, to see if its possible to simplify some of the code if possible

• Simplify CI merge queue logic #11331

[Jefffrey]

successful merge with the merge queue 🎉

• fix(arrow-cast): allow whitespace around temporal values (#11225) #11299

[alamb]

Thanks @Jefffrey and @blaginin