-
Couldn't load subscription status.
- Fork 15k
[MLIR][Python] reland (narrower) type stub generation #157930
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
Conversation
7d90167 to
6747bf1
Compare
|
Here's a brief retrospective of the failures involved in the previous PR:
|
|
Note, this has been tested against IREE with @Hardcode84's help iree-org/iree#21916 where it pass PkgCI / Build Packages / Linux Release (x86_64) |
|
Does MLIR have any kind of policy on bumping versions for Python dependencies? The LLVM project is generally very conservative about dependency requirements -- you can easily build LLVM against LTS distro packages. However, the MLIR nanobind requirement in particular is being bumped extremely aggressively -- the version required here is so new that it's not even part of the upcoming Fedora 43 release (a leading edge distro). |
|
Do distros prefer/expect to use system python packages? AFAIK, the general recommendation for MLIR-based projects is to use a virtual environment. |
|
Yes, distros always use system packages. And as this is a requirement of the LLVM build itself (not just for downstream dependents), I'd have to build LLVM inside a venv -- is this what people working on MLIR do? That sounds like really bad ergonomics. |
|
Echoing @kuhar - nanobind is a source dependency (it's always compiled during build of nanobind bindings) so there's no reason to depend on a distro's distribution and thus we advise users to either pip install or to use ExternalProject (or whatever source dependency mechanism they prefer).
If you build the Python bindings, you point the build (using Pyrhon_EXECUTABLE) to a venv yes. Not sure what's unergonomic about that? |
|
Speaking of ExternalProject, I wouldn't be opposed to migrating to using that instead of pip install (there are actually a whole mess of hacks in our CMake around finding nanobind and pybind in the venv). |
I was going to say that adding mlir to the project list should work without having to perform any other arcane invocations, but apparently MLIR_ENABLE_BINDINGS_PYTHON is already disabled by default. This being a non-default configuration does make the requirement less bad than I initially thought. But I feel like the fact that you have to add docs like https://mlir.llvm.org/docs/Bindings/Python/#recommended-development-practices indicates that something is going wrong here. Building LLVM should not require this kind of additional setup. I should not have to remember to activate a venv any time I work on LLVM. |
|
You don't have to use a venv, you can install the pip requirements for the current user I believe (this is what I have setup locally) |
I guess I don't disagree with the essence of this but this is just by virtue of Python and the nature of Python dev, not nanobind. To wit: nanobind is not the only package in our requirements.txt you would need to get from pip: |
This is pretty standard fare for python development. Also I believe you should only need to activate the venv when configuring (or alternatively it should be sufficient to just specify |
…8738) LLVM has also moved to generated stubfiles, so we need to generate these ourselves (we could omit them, but they're nice to have). See llvm/llvm-project#157930. Also pulls in the following followup fixes, these should be removed when bumping again. llvm/llvm-project#160183 llvm/llvm-project#160203 llvm/llvm-project#160221 This fixes some mypy lint errors, and causes a few more, I fixed a few but mostly just ignored them. MODULAR_ORIG_COMMIT_REV_ID: 524aaf2ab047e5185703c44ab3edd7754c67fa26
…ind (#161230) Inspired by this comment #157930 (comment) (and long-standing issues related to finding nanobind/pybind in the right place), this PR moves to using `FetchContent_Declare` to get the nanobind dependency. This is pretty standard (see e.g., [IREE](https://github.com/iree-org/iree/blob/cf60359b7443b0e92e15fb6ffc011525dc40e772/CMakeLists.txt#L842-L848)). This PR also removes pybind which has been deprecated for almost a year (#117922) and which isn't compatible (for whatever reason) with `FetchContent_Declare`. --------- Co-authored-by: Jacques Pienaar <[email protected]>
… remove pybind (#161230) Inspired by this comment llvm/llvm-project#157930 (comment) (and long-standing issues related to finding nanobind/pybind in the right place), this PR moves to using `FetchContent_Declare` to get the nanobind dependency. This is pretty standard (see e.g., [IREE](https://github.com/iree-org/iree/blob/cf60359b7443b0e92e15fb6ffc011525dc40e772/CMakeLists.txt#L842-L848)). This PR also removes pybind which has been deprecated for almost a year (llvm/llvm-project#117922) and which isn't compatible (for whatever reason) with `FetchContent_Declare`. --------- Co-authored-by: Jacques Pienaar <[email protected]>
…ind (#161230) Inspired by this comment llvm/llvm-project#157930 (comment) (and long-standing issues related to finding nanobind/pybind in the right place), this PR moves to using `FetchContent_Declare` to get the nanobind dependency. This is pretty standard (see e.g., [IREE](https://github.com/iree-org/iree/blob/cf60359b7443b0e92e15fb6ffc011525dc40e772/CMakeLists.txt#L842-L848)). This PR also removes pybind which has been deprecated for almost a year (llvm/llvm-project#117922) and which isn't compatible (for whatever reason) with `FetchContent_Declare`. --------- Co-authored-by: Jacques Pienaar <[email protected]>
|
I see that this PR introduces In other words, what is the feature/change from 2.9 that you depend on? |
Your "gut feeling" is incorrect - 2.9 was specifically the first release that had this fix wjakob/nanobind#1124 which is what was needed for this PR. |
…8738) LLVM has also moved to generated stubfiles, so we need to generate these ourselves (we could omit them, but they're nice to have). See llvm/llvm-project#157930. Also pulls in the following followup fixes, these should be removed when bumping again. llvm/llvm-project#160183 llvm/llvm-project#160203 llvm/llvm-project#160221 This fixes some mypy lint errors, and causes a few more, I fixed a few but mostly just ignored them. MAX_PYTHON_ORIG_REV_ID: 524aaf2ab047e5185703c44ab3edd7754c67fa26
…8738) LLVM has also moved to generated stubfiles, so we need to generate these ourselves (we could omit them, but they're nice to have). See llvm/llvm-project#157930. Also pulls in the following followup fixes, these should be removed when bumping again. llvm/llvm-project#160183 llvm/llvm-project#160203 llvm/llvm-project#160221 This fixes some mypy lint errors, and causes a few more, I fixed a few but mostly just ignored them. MAX_PYTHON_ORIG_REV_ID: 524aaf2ab047e5185703c44ab3edd7754c67fa26
…8738) LLVM has also moved to generated stubfiles, so we need to generate these ourselves (we could omit them, but they're nice to have). See llvm/llvm-project#157930. Also pulls in the following followup fixes, these should be removed when bumping again. llvm/llvm-project#160183 llvm/llvm-project#160203 llvm/llvm-project#160221 This fixes some mypy lint errors, and causes a few more, I fixed a few but mostly just ignored them. MAX_PYTHON_ORIG_REV_ID: 524aaf2ab047e5185703c44ab3edd7754c67fa26
…8738) LLVM has also moved to generated stubfiles, so we need to generate these ourselves (we could omit them, but they're nice to have). See llvm/llvm-project#157930. Also pulls in the following followup fixes, these should be removed when bumping again. llvm/llvm-project#160183 llvm/llvm-project#160203 llvm/llvm-project#160221 This fixes some mypy lint errors, and causes a few more, I fixed a few but mostly just ignored them. MAX_PYTHON_ORIG_REV_ID: 524aaf2ab047e5185703c44ab3edd7754c67fa26
…8738) LLVM has also moved to generated stubfiles, so we need to generate these ourselves (we could omit them, but they're nice to have). See llvm/llvm-project#157930. Also pulls in the following followup fixes, these should be removed when bumping again. llvm/llvm-project#160183 llvm/llvm-project#160203 llvm/llvm-project#160221 This fixes some mypy lint errors, and causes a few more, I fixed a few but mostly just ignored them. MAX_PYTHON_ORIG_REV_ID: 524aaf2ab047e5185703c44ab3edd7754c67fa26
…8738) LLVM has also moved to generated stubfiles, so we need to generate these ourselves (we could omit them, but they're nice to have). See llvm/llvm-project#157930. Also pulls in the following followup fixes, these should be removed when bumping again. llvm/llvm-project#160183 llvm/llvm-project#160203 llvm/llvm-project#160221 This fixes some mypy lint errors, and causes a few more, I fixed a few but mostly just ignored them. MAX_PYTHON_ORIG_REV_ID: 524aaf2ab047e5185703c44ab3edd7754c67fa26
…nstead of abstract PyOpView (#165053) llvm/llvm-project#157930 changed `nb::object getOwner()` to `PyOpView getOwner()` which implicitly constructs the generic OpView against from a (possibly) concrete OpView. This PR fixes that.
…bstract PyOpView (llvm#165053) llvm#157930 changed `nb::object getOwner()` to `PyOpView getOwner()` which implicitly constructs the generic OpView against from a (possibly) concrete OpView. This PR fixes that.
…bstract PyOpView (llvm#165053) llvm#157930 changed `nb::object getOwner()` to `PyOpView getOwner()` which implicitly constructs the generic OpView against from a (possibly) concrete OpView. This PR fixes that.
This a reland of #155741 which was reverted at #157831. This version is narrower in scope - it only turns on automatic stub generation for
MLIRPythonExtension.Core._mlirand does not do anything automatically. Specifically, the only CMake code added toAddMLIRPython.cmakeis themlir_generate_type_stubsfunction which is then used only in a manual way. The API formlir_generate_type_stubsis:Downstream users should use
mlir_generate_type_stubsin coordination withdeclare_mlir_python_sourcesto turn on stub generation for their own downstream dialect extensions and upstream dialect extensions if they so choose. Standalone example shows an example.Note, downstream will also need to set
-DMLIR_PYTHON_PACKAGE_PREFIX=...correctly for their bindings.