Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mlir] Add option to disable MLIR Python dev package configuration. #117934

Conversation

stellaraccident
Copy link
Contributor

@stellaraccident stellaraccident commented Nov 27, 2024

Adds a CMake option MLIR_DISABLE_CONFIGURE_PYTHON_DEV_PACKAGES which gates doing package discovery and configuration for Python dev packages by MLIR (this was made opt-out to preserve compatibility with find_package(MLIR) based uses which do not set the standard options).

The default Python setup that MLIR does has been a problem for super-projects that include LLVM for a long time because it forces a very specific package discovery mechanism that is not uniform in all uses.

When reviewing #117922, I noted that this would effectively be a break the world event for downstreams, forcing them to adapt their nanobind dep to the exact way that MLIR does it. Adding the option to just wholesale skip the built-in configuration heuristics at least gives us a mechanism to tell downstreams to migrate to, giving them complete control and not requiring packaging workarounds. This seemed a better option than (once again) creating a situation where downstreams could not integrate the dep change without doing tricky infra upgrades, and it removes the burden from the author of that patch from needing to think about how this affects super-projects that include MLIR (i.e. they can just be told to do it themselves as needed vs being in a wedged state and unable to upgrade).

@llvmbot llvmbot added the mlir label Nov 27, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 27, 2024

@llvm/pr-subscribers-mlir

Author: Stella Laurenzo (stellaraccident)

Changes

Adds a CMake option MLIR_CONFIGURE_PYTHON_DEV_PACKAGES which gates doing package discovery and configuration for Python dev packages by MLIR.

The default Python setup that MLIR does has been a problem for super-projects that include LLVM for a long time because it forces a very specific package discovery mechanism that is not uniform in all uses.

When reviewing #117922, I noted that this would effectively be a break the world event for downstreams, forcing them to adapt their nanobind dep to the exact way that MLIR does it. Adding the option to just wholesale skip the built-in configuration heuristics at least gives us a mechanism to tell downstreams to migrate to, giving them complete control and not requiring packaging workarounds. This seemed a better option than (once again) creating a situation where downstreams could not integrate the dep change without doing tricky infra upgrades, and it removes the burden from the author of that patch from needing to think about how this affects super-projects that include MLIR (i.e. they can just be told to do it themselves as needed vs being in a wedged state and unable to upgrade).


Full diff: https://github.com/llvm/llvm-project/pull/117934.diff

2 Files Affected:

  • (modified) mlir/CMakeLists.txt (+12)
  • (modified) mlir/cmake/modules/MLIRDetectPythonEnv.cmake (+28-26)
diff --git a/mlir/CMakeLists.txt b/mlir/CMakeLists.txt
index 2880dc30bca91f..1e9e0bcfa66ff2 100644
--- a/mlir/CMakeLists.txt
+++ b/mlir/CMakeLists.txt
@@ -162,6 +162,9 @@ configure_file(
 # Requires:
 #   The pybind11 library can be found (set with -DPYBIND_DIR=...)
 #   The python executable is correct (set with -DPython3_EXECUTABLE=...)
+# By default, find_package and probing for installed pybind11 is performed.
+# Super projects can set MLIR_CONFIGURE_PYTHON_DEV_PACKAGES to disable all
+# package setup and control it themselves.
 #-------------------------------------------------------------------------------
 
 set(MLIR_ENABLE_BINDINGS_PYTHON 0 CACHE BOOL
@@ -170,8 +173,17 @@ set(MLIR_DETECT_PYTHON_ENV_PRIME_SEARCH 1 CACHE BOOL
        "Prime the python detection by searching for a full 'Development' \
        component first (temporary while diagnosing environment specific Python \
        detection issues)")
+set(MLIR_CONFIGURE_PYTHON_DEV_PACKAGES 1 CACHE BOOL
+       "Performs python dev package configuration sufficient to use all MLIR \
+       python features. Super-projects that wish to control their own setup \
+       must perform an appropriate find_package of Python3 with \
+       'Development.Module' and ensure that find_package(pybind11) is \
+       satisfied (and keep up to date as requirements evolve).")
+
 if(MLIR_ENABLE_BINDINGS_PYTHON)
   include(MLIRDetectPythonEnv)
+  # Note that both upstream and downstreams often call this macro. It gates
+  # internally on the MLIR_CONFIGURE_PYTHON_DEV_PACKAGES option.
   mlir_configure_python_dev_packages()
 endif()
 
diff --git a/mlir/cmake/modules/MLIRDetectPythonEnv.cmake b/mlir/cmake/modules/MLIRDetectPythonEnv.cmake
index d3a98aaf6ffd17..826b805d4564d9 100644
--- a/mlir/cmake/modules/MLIRDetectPythonEnv.cmake
+++ b/mlir/cmake/modules/MLIRDetectPythonEnv.cmake
@@ -2,34 +2,36 @@
 
 # Finds and configures python packages needed to build MLIR Python bindings.
 macro(mlir_configure_python_dev_packages)
-  if(MLIR_DETECT_PYTHON_ENV_PRIME_SEARCH)
-    # Prime the search for python to see if there is a full development
-    # package. This seems to work around cmake bugs searching only for
-    # Development.Module in some environments. However, in other environments
-    # it may interfere with the subsequent search for Development.Module.
-    find_package(Python3 ${LLVM_MINIMUM_PYTHON_VERSION}
-      COMPONENTS Interpreter Development)
-  endif()
+  if(MLIR_CONFIGURE_PYTHON_DEV_PACKAGES)
+    if(MLIR_DETECT_PYTHON_ENV_PRIME_SEARCH)
+      # Prime the search for python to see if there is a full development
+      # package. This seems to work around cmake bugs searching only for
+      # Development.Module in some environments. However, in other environments
+      # it may interfere with the subsequent search for Development.Module.
+      find_package(Python3 ${LLVM_MINIMUM_PYTHON_VERSION}
+        COMPONENTS Interpreter Development)
+    endif()
 
-  # After CMake 3.18, we are able to limit the scope of the search to just
-  # Development.Module. Searching for Development will fail in situations where
-  # the Python libraries are not available. When possible, limit to just
-  # Development.Module.
-  # See https://pybind11.readthedocs.io/en/stable/compiling.html#findpython-mode
-  set(_python_development_component Development.Module)
+    # After CMake 3.18, we are able to limit the scope of the search to just
+    # Development.Module. Searching for Development will fail in situations where
+    # the Python libraries are not available. When possible, limit to just
+    # Development.Module.
+    # See https://pybind11.readthedocs.io/en/stable/compiling.html#findpython-mode
+    set(_python_development_component Development.Module)
 
-  find_package(Python3 ${LLVM_MINIMUM_PYTHON_VERSION}
-    COMPONENTS Interpreter ${_python_development_component} REQUIRED)
-  unset(_python_development_component)
-  message(STATUS "Found python include dirs: ${Python3_INCLUDE_DIRS}")
-  message(STATUS "Found python libraries: ${Python3_LIBRARIES}")
-  message(STATUS "Found numpy v${Python3_NumPy_VERSION}: ${Python3_NumPy_INCLUDE_DIRS}")
-  mlir_detect_pybind11_install()
-  find_package(pybind11 2.10 CONFIG REQUIRED)
-  message(STATUS "Found pybind11 v${pybind11_VERSION}: ${pybind11_INCLUDE_DIR}")
-  message(STATUS "Python prefix = '${PYTHON_MODULE_PREFIX}', "
-                 "suffix = '${PYTHON_MODULE_SUFFIX}', "
-                 "extension = '${PYTHON_MODULE_EXTENSION}")
+    find_package(Python3 ${LLVM_MINIMUM_PYTHON_VERSION}
+      COMPONENTS Interpreter ${_python_development_component} REQUIRED)
+    unset(_python_development_component)
+    message(STATUS "Found python include dirs: ${Python3_INCLUDE_DIRS}")
+    message(STATUS "Found python libraries: ${Python3_LIBRARIES}")
+    message(STATUS "Found numpy v${Python3_NumPy_VERSION}: ${Python3_NumPy_INCLUDE_DIRS}")
+    mlir_detect_pybind11_install()
+    find_package(pybind11 2.10 CONFIG REQUIRED)
+    message(STATUS "Found pybind11 v${pybind11_VERSION}: ${pybind11_INCLUDE_DIR}")
+    message(STATUS "Python prefix = '${PYTHON_MODULE_PREFIX}', "
+                  "suffix = '${PYTHON_MODULE_SUFFIX}', "
+                  "extension = '${PYTHON_MODULE_EXTENSION}")
+  endif()
 endmacro()
 
 # Detects a pybind11 package installed in the current python environment

Adds a CMake option MLIR_CONFIGURE_PYTHON_DEV_PACKAGES which gates doing package discovery and configuration for Python dev packages by MLIR.

The default Python setup that MLIR does has been a problem for super-projects that include LLVM for a long time because it forces a very specific package discovery mechanism that is not uniform in all uses.

When reviewing #117922, I noted that this would effectively be a break the world event for downstreams, forcing them to adapt their nanobind dep to the exact way that MLIR does it. Adding the option to just wholesale skip the built-in configuration heuristics at least gives us a mechanism to tell downstreams to migrate to, giving them complete control and not requiring packaging workarounds. This seemed a better option than (once again) creating a situation where downstreams could not integrate the dep change without doing tricky infra upgrades, and it removes the burden from the author of that patch from needing to think about how this affects super-projects that include MLIR (i.e. they can just be told to do it themselves as needed vs being in a wedged state and unable to upgrade).
@stellaraccident stellaraccident force-pushed the users/stellaraccident/mlir_cmake_optional_configure_python branch from 7c301e9 to 08bfede Compare November 27, 2024 23:00
stellaraccident added a commit to iree-org/iree that referenced this pull request Nov 27, 2024
… config.

This is in preparation for llvm/llvm-project#117934 and a followon patch which changes the default python setup in a way that will conflict with what we do. Landing pre-emptively to avoid disruption.

Signed-off-by: Stella Laurenzo <[email protected]>
stellaraccident added a commit to iree-org/iree that referenced this pull request Nov 27, 2024
… config. (#19328)

This is in preparation for
llvm/llvm-project#117934 and a followon patch
which changes the default python setup in a way that will conflict with
what we do. Landing pre-emptively to avoid disruption.

Signed-off-by: Stella Laurenzo <[email protected]>
Copy link
Contributor

@makslevental makslevental left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

@stellaraccident stellaraccident merged commit 65339e4 into main Nov 28, 2024
8 checks passed
@stellaraccident stellaraccident deleted the users/stellaraccident/mlir_cmake_optional_configure_python branch November 28, 2024 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants