Page MenuHomePhabricator

[mlir] Limit Python dependency to Development.Module when possible.
ClosedPublic

Authored by mikeurbach on Oct 11 2021, 4:32 PM.

Details

Summary

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

Diff Detail

Event Timeline

mikeurbach created this revision.Oct 11 2021, 4:32 PM
mikeurbach requested review of this revision.Oct 11 2021, 4:32 PM

Take two. I tested this one locally and I think it should work. Do the buildbots run after commit to main or is there anywhere I can check before committing?

mlir/CMakeLists.txt
110

There are different ways to address this, but predicating this with OR MLIR_BINDINGS_PYTHON_LOCK_VERSION seems like an appropriate use of the existing option. Curious what you think @stellaraccident .

stellaraccident accepted this revision.Oct 11 2021, 5:01 PM
stellaraccident added inline comments.
mlir/CMakeLists.txt
110

I think the right thing is to just delete MLIR_BINDINGS_PYTHON_LOCK_VERSION and retrofit add_mlir_python_extension (from AddMLIRPython.cmake) with the version I have in this patch: https://reviews.llvm.org/D111513

If this gets us an easily revertible baby step towards that, then I think my next step would be to pick just the changes to add_mlir_python_extension into a standalone path and nuke MLIR_BINDINGS_PYTHON_LOCK_VERSION entirely (it was really only in that patch because I needed to adjust some unrelated things for building the Standalone project).

So if this works like this, I'm fine with it as a next step.

This revision is now accepted and ready to land.Oct 11 2021, 5:01 PM
mikeurbach added inline comments.Oct 11 2021, 6:18 PM
mlir/CMakeLists.txt
110

Let me take a deeper look at your patch, I owe a proper review there.

Get rid of MLIR_BINDINGS_PYTHON_LOCK_VERSION and take add_mlir_python_extension from https://reviews.llvm.org/D111513.

mlir/CMakeLists.txt
110

I commented on the other patch about something I found confusing when using this in CIRCT, but I think this works.

stellaraccident accepted this revision.Oct 11 2021, 11:07 PM

Thanks - if this passes presubmit, looks good to me.

mikeurbach marked an inline comment as done.Oct 12 2021, 8:03 AM
mikeurbach edited the summary of this revision. (Show Details)

Update commit message.