[CI] Add check-mlir-python to MLIR pre-merge checks #72847
Conversation
MacDue
force-pushed
the
add_mlir_python_checks
branch
from
8a90fef
to
64bc76f
Compare
This reverts commit 64bc76f.
|
It's part of the image installed on the builders, and it's not something you control directly. Right now @metaflow is managing those images. I think this is great though -- it shows that MLIR needs something slightly more specific than the monolithic CI setup we currently provide for most projects. This is what I suspected (i.e. that subprojects would have specific needs). Once we have runners able to take the load, we should move this to Github actions and then MLIR could define its own jobs that run appropriate checks and use appropriate dependencies. |
|
Some sort of public documentation on what exactly are the images and how to update those (even if it involves filing an issue an assigning that to specific people) is needed IMO. Specifically, I'm thinking of the case where MLIR bumps python/pybind11/numpy version and the image need to be updated in order for that change to land. Otherwise I'm happy to have this! (cc @joker-eph ) |
Better update the test script to run |
Are you suggesting that we should split the monolithic config? |
|
@llvm/pr-subscribers-mlir Author: Benjamin Maxwell (MacDue) ChangesPR for: https://discourse.llvm.org/t/add-check-mlir-python-to-the-mlir-pre-commit-tests/74041 It’s easy to forget about the Python bindings and not build/test them locally. It’s also easy to change something that’ll break the python binding tests and not find out till after you’ve committed your change. These tests seem to run quickly and don’t require much extra setup, so let's add them to the general MLIR pre-merge tests. Full diff: https://github.com/llvm/llvm-project/pull/72847.diff 3 Files Affected:
diff --git a/.ci/monolithic-linux.sh b/.ci/monolithic-linux.sh
index 311488b2c2878b1..f0577d1069d5dec 100755
--- a/.ci/monolithic-linux.sh
+++ b/.ci/monolithic-linux.sh
@@ -49,7 +49,8 @@ cmake -S ${MONOREPO_ROOT}/llvm -B ${BUILD_DIR} \
-D LLVM_ENABLE_LLD=ON \
-D CMAKE_CXX_FLAGS=-gmlt \
-D BOLT_CLANG_EXE=/usr/bin/clang \
- -D LLVM_CCACHE_BUILD=ON
+ -D LLVM_CCACHE_BUILD=ON \
+ -D MLIR_ENABLE_BINDINGS_PYTHON=ON
echo "--- ninja"
# Targets are not escaped as they are passed as separate arguments.
diff --git a/.ci/monolithic-windows.sh b/.ci/monolithic-windows.sh
index 00c3037c4c4fd61..7ac806a0b399ad5 100755
--- a/.ci/monolithic-windows.sh
+++ b/.ci/monolithic-windows.sh
@@ -48,7 +48,8 @@ cmake -S ${MONOREPO_ROOT}/llvm -B ${BUILD_DIR} \
-D LLVM_LIT_ARGS="-v --xunit-xml-output ${BUILD_DIR}/test-results.xml" \
-D COMPILER_RT_BUILD_ORC=OFF \
-D CMAKE_C_COMPILER_LAUNCHER=sccache \
- -D CMAKE_CXX_COMPILER_LAUNCHER=sccache
+ -D CMAKE_CXX_COMPILER_LAUNCHER=sccache \
+ -D MLIR_ENABLE_BINDINGS_PYTHON=ON
echo "--- ninja"
# Targets are not escaped as they are passed as separate arguments.
diff --git a/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td b/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td
index ba33a2826e6ca4b..37b99f0738adc43 100644
--- a/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td
+++ b/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td
@@ -11,6 +11,7 @@
//
//===----------------------------------------------------------------------===//
+// Test
#ifndef ARMSME_OPS
#define ARMSME_OPS
|
|
@llvm/pr-subscribers-mlir-sme Author: Benjamin Maxwell (MacDue) ChangesPR for: https://discourse.llvm.org/t/add-check-mlir-python-to-the-mlir-pre-commit-tests/74041 It’s easy to forget about the Python bindings and not build/test them locally. It’s also easy to change something that’ll break the python binding tests and not find out till after you’ve committed your change. These tests seem to run quickly and don’t require much extra setup, so let's add them to the general MLIR pre-merge tests. Full diff: https://github.com/llvm/llvm-project/pull/72847.diff 3 Files Affected:
diff --git a/.ci/monolithic-linux.sh b/.ci/monolithic-linux.sh
index 311488b2c2878b1..f0577d1069d5dec 100755
--- a/.ci/monolithic-linux.sh
+++ b/.ci/monolithic-linux.sh
@@ -49,7 +49,8 @@ cmake -S ${MONOREPO_ROOT}/llvm -B ${BUILD_DIR} \
-D LLVM_ENABLE_LLD=ON \
-D CMAKE_CXX_FLAGS=-gmlt \
-D BOLT_CLANG_EXE=/usr/bin/clang \
- -D LLVM_CCACHE_BUILD=ON
+ -D LLVM_CCACHE_BUILD=ON \
+ -D MLIR_ENABLE_BINDINGS_PYTHON=ON
echo "--- ninja"
# Targets are not escaped as they are passed as separate arguments.
diff --git a/.ci/monolithic-windows.sh b/.ci/monolithic-windows.sh
index 00c3037c4c4fd61..7ac806a0b399ad5 100755
--- a/.ci/monolithic-windows.sh
+++ b/.ci/monolithic-windows.sh
@@ -48,7 +48,8 @@ cmake -S ${MONOREPO_ROOT}/llvm -B ${BUILD_DIR} \
-D LLVM_LIT_ARGS="-v --xunit-xml-output ${BUILD_DIR}/test-results.xml" \
-D COMPILER_RT_BUILD_ORC=OFF \
-D CMAKE_C_COMPILER_LAUNCHER=sccache \
- -D CMAKE_CXX_COMPILER_LAUNCHER=sccache
+ -D CMAKE_CXX_COMPILER_LAUNCHER=sccache \
+ -D MLIR_ENABLE_BINDINGS_PYTHON=ON
echo "--- ninja"
# Targets are not escaped as they are passed as separate arguments.
diff --git a/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td b/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td
index ba33a2826e6ca4b..37b99f0738adc43 100644
--- a/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td
+++ b/mlir/include/mlir/Dialect/ArmSME/IR/ArmSMEOps.td
@@ -11,6 +11,7 @@
//
//===----------------------------------------------------------------------===//
+// Test
#ifndef ARMSME_OPS
#define ARMSME_OPS
|
There was a problem hiding this comment.
MLIR could define its own jobs that run appropriate checks and use appropriate dependencies.
Are you suggesting that we should split the monolithic config? That seems like a pretty bad idea for the mono-repo in terms of efficiencies: LLVM is pretty "heavy" to build and if I change MLIR that means right now running both MLIR tests and Flang tests. If Flang and MLIR have disjoint testing that means that we would build LLVM+MLIR twice (once for a MLIR job and once for the Flang job).
That's what I am suggesting. However, it doesn't mean we'd necessarily have to build everything twice, we could share stuff via artifacts or we could use ccache to reduce the amount of duplicate work we're actually doing. But I do think that the project would be better off with each sub-project defining the tests that make sense for them, yes.
The monolithic config is just nicely setup to build just the set of needed projects once, and run the necessary tests.
The problem is that we don't run "the necessary tests". We run check-<whatever> and that's it -- for some projects that may be insufficient. For example this patch mentions that we need to also check check-mlir-python, but in the runtimes we have much larger needs like checking different configurations (exceptions enabled or not, etc). I suspect other projects have similar special needs which would not be met by the current monolithic config, and which wouldn't make sense to share across all LLVM projects. For instance it may not make sense to test libc with exceptions enabled and disabled, but that's definitely a requirement for e.g. libc++.
As mentioned here, there's also the need for potentially different tool versions being used by various projects, which is also not easy to accommodate in a monolithic world.
All in all, I have little stakes in this since libc++ already has its own setup that works for our needs. But I do believe that the other LLVM subprojects would see benefits in moving away from the monolithic setup and towards something where they control their CI configuration a bit more (obviously without having to go as far as to set up their own self-hosted builders, which should definitely be handled globally for all of LLVM).
As far as this change goes, I'm fine with it.
This is at least how llvm, clang, flang, bolt, lldb, lld, and mlir are tested. That's not just isolated cases...
I don't disagree, but you seem to come from the "exception" and proposing to generalize it. I don't quite understand why this is better...
This was debunked as wrong during the review and removed from the diff already.
Sure, the runtimes are "different" from the monolithic build, but I don't see how that applies to the testing of changes to "llvm, clang, flang, bolt, lldb, lld, and mlir"?
For the other projects this is more a questions of whether there are "project specific configs": for example I'm sure there are many configs for LLVM (let's with/without MLGO configured for example, or with/without Z3 solver) that won't affect all the other project. And enabling such flag deserve a dedicated testing (whether it deserves to run this in premerge is another story). But if we look at the testing matrix of the "standard" config of the mono-repo to run all the unit-tests (excluding the runtime), you have:
So without changing any config to the project settings, we already have a matrix to sweep over >100 configurations here. Having every single project setting up the full matrix does not seem like a good path forward to me: this is highly redundant and there is no "project-specific value add" in any of these.
If I'm working on MLIR, lld or Bolt, it's much better for me to benefit from the monolithic setup taking care of the two steps above instead of every sub-project having to redo everything. (On top of being more human-resource efficient, it's also much much more efficient in terms of could resources). |
This reverts commit 68a4b12.
|
I'll be landing this shortly, I'm assuming this is fine for now with regards to the python dependencies :) |
Local branch amd-gfx 1dfdb7d Merged main:9cee94b81b14 into amd-gfx:fe664d7a82a6 Remote branch main 2170252 [CI] Add check-mlir-python to MLIR pre-merge checks (llvm#72847)
PR for: https://discourse.llvm.org/t/add-check-mlir-python-to-the-mlir-pre-commit-tests/74041
It’s easy to forget about the Python bindings and not build/test them locally. It’s also easy to change something that’ll break the python binding tests and not find out till after you’ve committed your change.
These tests seem to run quickly and don’t require much extra setup, so let's add them to the general MLIR pre-merge tests.