[CI] Refactor CI workflows slightly for quicker response

[murphe67]

No description provided.

[murphe67]

Slightly undoing #805 but this is just a proposal not sure exactly how to handle:

The consolidation of check-dynamatic in #805 doesn't fully work as the unit tests are gated on what is built: local users who built without --enable-abc do not run the blif equivalence tests when they run check-dynamatic.

The --enable-abc build command is currently only tested in unittests, if the build fails with this flag all build CI tests will pass and the unit tests will fail, which gives the wrong impression.

Additionally, the --enable-abc flag makes build ~1.5x slower, and so the unit test build is much slower than the normal builds (which is what made me notice all of this).

I have therefore added a check-dynamatic-all target for local users who have --enable-abc built and want to run the full suite. check-dynamatic still runs the hls-fuzzer tests when ran locally.

The CI runs a job of fast-unit-tests by building without abc and running check-dynamatic for people like me who want to make sure their new unit tests run on the server relatively quickly. Then a job of blif-equivalent builds with abc and checks the blif equivalence checks.

I also added a 4th job to the build section of build with abc, so if a change break this a failure also shows up in the build CI jobs.

Very open to thoughts on this, this is a first draft solution.

[murphe67]

Should check-constraints-programming-library also be in check-dynamatic-all? does building with cbc slow down the build and should be pulled out into its own target?

[murphe67]

oh it gets messier, the constraint programming unit tests need gurobi and so need to be ran on the self hosted runner and so can't be pulled out of the generic CI job as is

[zero9178]

Do I understand correctly that the main goal you're trying to achieve is to make unit-tests fail faster by making the build faster by having a build without ABC?

That seems fine to me but if build speed is a concern my expectation would have first been to introduce fixes such as using ccache with caching on GitHub Actions. That ought to take the build time down to basically zero. The CI Runner e.g. uses ccache and it even builds LLVM and does so in 2 minutes (!): https://github.com/EPFL-LAP/dynamatic/actions/runs/25996306320/job/76411285896
Not because the machine is so fast, but because all of LLVM is in its ccache. We can use something like https://github.com/Chocobo1/setup-ccache-action for unit-tests.

In pursuit of the above goal the cmake changes seem a bit redundant and unintuative to me. The testing time of the blif equivalence tests seem to be neglibigle. It'd be weird to explain to users "this command runs all unit tests except the blif equivalence tests, for those you need to run check-dynamatic-all". There doesn't seem to be a justification the way there is for integratation tests to me.

[murphe67]

I actually don't have a super clear end goal- I started investigating this when I saw the difference in build time but my conclusion at this point is that the current layout has no consistent logic and we should fix that.

The caching definitely seems like a useful idea to add for sure, thank you for those links 😁.

I think I totally disagree on "this command runs all unit tests except the blif equivalence tests, for those you need to run check-dynamatic-all"- my driving line would be "check-dynamatic should not run different checks depending on what is built"./"check-dynamatic should not pass on one machine and fail on another (as much as possible)"

So I think in an ideal world the checks would not be gated? so if you run check-dynamatic-all and you don't have Dynamatic built with ABC, it fails and tells you it failed because you don't have Dynamatic built with ABC.

So that the check only passes if actually everything registered to the check passes.

maybe in this case we still only have check-dynamatic, we just enforce that? depends how annoying we think it is to make people add abc to their build.

[zero9178]

I think I totally disagree on "this command runs all unit tests except the blif equivalence tests, for those you need to run check-dynamatic-all"- my driving line would be "check-dynamatic should not run different checks depending on what is built"./"check-dynamatic should not pass on one machine and fail on another (as much as possible)"

This makes sense to me. I just fear that this is an inherent problem when one has multiple build configurations, some that aren't even experimental but that we care to maintain.

My line of thinking is that if we care about a build config (like ABC) then we should have CI for it that we care about and may block a PR from being merged. If we have CI for a build config then developers too should care about not breaking it and be able to test it locally. We want developers to care about not breaking that configuration after all. Having it in check-dynamatic makes sense therefore.
In an ideal world I think enable-abc should just be the default and shouldn't be able to be disabled.

Even in the current setup, I don't think check-dynamatic should fail if ABC is not installed. This would just lead to people not even testing locally and fully relying on CI to do so. One should still be able to test as much as possible given the build configuration, even if not perfect and ideal.

[murphe67]

A brief google says we should be using the UNSUPPORTED feature so that the tests show up as not running when testing locally? rather than being silently skipped

[murphe67]

You can see in my CI runs on this PR that the unit test job fails cause the constraint programming target doesn't exist since its unsupported on the GitHub hosted machine, here the target should clearly exist and return unsupported as the test result, right?

[zero9178]

You can see in my CI runs on this PR that the unit test job fails cause the constraint programming target doesn't exist since its unsupported on the GitHub hosted machine, here the target should clearly exist and return unsupported as the test result, right?

Yes thats a good idea. I am not sure how to do this for arbitrary unit tests but one can do this in lit at the very least

[murphe67]

if it detects abc not installed, it registers a test which always returns unsupported? bit hacky...