Page MenuHomePhabricator

[mlir] Experimental, simplified CMake auto-config for Python development.
AcceptedPublic

Authored by stellaraccident on May 7 2022, 10:20 PM.

Details

Summary

This uses an approach that, arguably, the upstream CMake project should use for >= 3.18, in which we gained the ability to eval. Instead of a convoluted CMake script to try to query the Python config settings, we just have a Python script that does so and spits out CMake set commands as needed. This has several advantages:

  • Python is good at introspecting itself. It is better to code it there than have CMake firing off a bunch of one liners and string splicing everywhere.
  • If it breaks, people just run the Python script and see exactly what was detected (I used this to get it working on Windows without even running CMake).
  • Does not introduce the longstanding bug where there are cache and hint races with Development.Module.
  • Invokes Python once, not a bunch of times. This has a significant impact on configure time, especially on Windows.

I just implemented the variables, targets and commands as documented in the CMake manual: https://cmake.org/cmake/help/latest/module/FindPython.html. I left out a lot of excessive generality related to Python 2.

If we go with this eventually, the overall CMake scripts can be substantially simplified (with just an old-version fallback to find_package the Development component.

I can add some caching and such in the future, but since that is notoriously difficult to get right, I'll do that with a downstream in view (which is where it matters: layered projects). I believe that as I have written this, it should support switching the Python interpreter between CMake invocations (without nuking the cache), which will help both dev and deployment flows (which currently need to start fresh because the CMake find_package stuff doesn't reliably let you switch Python interpreters on subsequent calls).

See: https://github.com/llvm/llvm-project/issues/54906

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 10:20 PM
stellaraccident requested review of this revision.May 7 2022, 10:20 PM

Does this print how to run the python file in case of CMake failure?

Does this print how to run the python file in case of CMake failure?

Good idea - i think that would help diagnosing a lot

This looks good to me in general. I tried taking it for a spin, but I realized the dev machine I was on didn't have the proper python environment to test this. I will check again after installing a suitable python.

Seems reasonable. If this works it would be nice to just hand it off to the cmake folks...

This revision is now accepted and ready to land.May 10 2022, 9:54 PM

Seems reasonable. If this works it would be nice to just hand it off to the cmake folks...

Probably needs a few turns on it in practice, but yeah, agreed - at least serves as a nice POC for a simplified thing on the cmake side. We should raise an issue with them on it for visibility once we have it working ok.

mikeurbach accepted this revision.May 16 2022, 3:16 PM

Alright, I got a chance to test this, and I think it's working well.

Also, I had some dumb issues on my end, and I thought the error messages were great (e.g. I forgot to activate my virtualenv, and got a nice error about not finding Python.h since the system Python install didn't have that).

Seems like a good starting point for us to put some mileage on it, and then we can talk about making it the default, sending it to the CMake folks, etc.

Actually, may have spoken too soon. I was able to run CMake successfully, but when I went to compile the Python dynamic shared object, there were some issues finding Python.h like this:

[8/30] Building CXX object tools/circt/lib/Bindings/Python/CMakeFiles/CIRCTMLIRPythonModules.extension._circt.dso.dir/CIRCTModule.cpp.o
FAILED: tools/circt/lib/Bindings/Python/CMakeFiles/CIRCTMLIRPythonModules.extension._circt.dso.dir/CIRCTModule.cpp.o 
/usr/lib64/ccache/c++ -DCIRCTMLIRPythonModules_extension__circt_dso_EXPORTS -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/circt/lib/Bindings/Python -I../lib/Bindings/Python -Iinclude -I../llvm/llvm/include -I../include -Itools/circt/include -isystem ../llvm/llvm/../mlir/include -isystem tools/mlir/include -isystem /home/mikeu/.venv/lib/python3.9/site-packages/pybind11/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -Wmisleading-indentation -fdiagnostics-color -g -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -frtti -fexceptions -std=c++17 -MD -MT tools/circt/lib/Bindings/Python/CMakeFiles/CIRCTMLIRPythonModules.extension._circt.dso.dir/CIRCTModule.cpp.o -MF tools/circt/lib/Bindings/Python/CMakeFiles/CIRCTMLIRPythonModules.extension._circt.dso.dir/CIRCTModule.cpp.o.d -o tools/circt/lib/Bindings/Python/CMakeFiles/CIRCTMLIRPythonModules.extension._circt.dso.dir/CIRCTModule.cpp.o -c /home/mikeu/circt/lib/Bindings/Python/CIRCTModule.cpp
In file included from /home/mikeu/.venv/lib/python3.9/site-packages/pybind11/include/pybind11/cast.h:13,
                 from /home/mikeu/.venv/lib/python3.9/site-packages/pybind11/include/pybind11/attr.h:13,
                 from /home/mikeu/.venv/lib/python3.9/site-packages/pybind11/include/pybind11/detail/class.h:12,
                 from /home/mikeu/.venv/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h:13,
                 from /home/mikeu/circt/lib/Bindings/Python/DialectModules.h:16,
                 from /home/mikeu/circt/lib/Bindings/Python/CIRCTModule.cpp:9:
/home/mikeu/.venv/lib/python3.9/site-packages/pybind11/include/pybind11/detail/common.h:213:10: fatal error: Python.h: No such file or directory
 #include <Python.h>
          ^~~~~~~~~~
compilation terminated.

Will try the recommended steps to debug this script...

I do see the script correctly sets this:

SET(Python3_INCLUDE_DIRS "/path/to/python/distro/include/python3.9")

Where that directory does have Python.h and friends. But I don't see anything in the actual compile command pasted above include that directory. Is there some other voodoo going on in the old CMake beyond setting Python3_INCLUDE_DIRS? It seems like the script did exactly what it intended to, but Python.h was still not found.

I confirmed I don't hit this issue in the same environment with the new simple python detection script disabled... I will try to do a little deep dive into this and see if I can figure out what's going on.

Ok, I manually made the change in the inline comment and that fixed my issue, so I think this should be good to go after that.

mlir/cmake/modules/MLIRSimplePythonModuleConfig.cmake
86

Looks like this needs to be _type, not type?