[mlir][python] Make types in register_(dialect|operation) more narrow. #115307
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR makes the `pyClass`/`dialectClass` arguments of the pybind11 functions `register_dialect` and `register_operation` as well as their return types more narrow, concretely, a `py::type` instead of a `py::object`. As the name of the arguments indicate, they have to be called with a type instance (a "class"). The PR also updates the typing stubs of these functions (in the corresponding `.pyi` file), such that static type checkers are aware of the changed type. With the previous typing information, `pyright` raised errors on code generated by tablegen. Signed-off-by: Ingo Müller <[email protected]>
ingomueller-net
requested review from
ftynse,
makslevental and
stellaraccident
as code owners
Member
|
@llvm/pr-subscribers-mlir Author: Ingo Müller (ingomueller-net) ChangesThis PR makes the Full diff: https://github.com/llvm/llvm-project/pull/115307.diff 2 Files Affected:
diff --git a/mlir/lib/Bindings/Python/MainModule.cpp b/mlir/lib/Bindings/Python/MainModule.cpp
index 8da1ab16a4514b..7c27021902de31 100644
--- a/mlir/lib/Bindings/Python/MainModule.cpp
+++ b/mlir/lib/Bindings/Python/MainModule.cpp
@@ -58,7 +58,7 @@ PYBIND11_MODULE(_mlir, m) {
// Registration decorators.
m.def(
"register_dialect",
- [](py::object pyClass) {
+ [](py::type pyClass) {
std::string dialectNamespace =
pyClass.attr("DIALECT_NAMESPACE").cast<std::string>();
PyGlobals::get().registerDialectImpl(dialectNamespace, pyClass);
@@ -68,9 +68,9 @@ PYBIND11_MODULE(_mlir, m) {
"Class decorator for registering a custom Dialect wrapper");
m.def(
"register_operation",
- [](const py::object &dialectClass, bool replace) -> py::cpp_function {
+ [](const py::type &dialectClass, bool replace) -> py::cpp_function {
return py::cpp_function(
- [dialectClass, replace](py::object opClass) -> py::object {
+ [dialectClass, replace](py::type opClass) -> py::type {
std::string operationName =
opClass.attr("OPERATION_NAME").cast<std::string>();
PyGlobals::get().registerOperationImpl(operationName, opClass,
diff --git a/mlir/python/mlir/_mlir_libs/_mlir/__init__.pyi b/mlir/python/mlir/_mlir_libs/_mlir/__init__.pyi
index 42694747e5f24f..03449b70b7fa38 100644
--- a/mlir/python/mlir/_mlir_libs/_mlir/__init__.pyi
+++ b/mlir/python/mlir/_mlir_libs/_mlir/__init__.pyi
@@ -8,5 +8,5 @@ class _Globals:
def append_dialect_search_prefix(self, module_name: str) -> None: ...
def _check_dialect_module_loaded(self, dialect_namespace: str) -> bool: ...
-def register_dialect(dialect_class: type) -> object: ...
-def register_operation(dialect_class: type, *, replace: bool = ...) -> object: ...
+def register_dialect(dialect_class: type) -> type: ...
+def register_operation(dialect_class: type, *, replace: bool = ...) -> type: ...
|
Groverkss
pushed a commit
to iree-org/llvm-project
that referenced
this pull request
Nov 15, 2024
llvm#115307) This PR makes the `pyClass`/`dialectClass` arguments of the pybind11 functions `register_dialect` and `register_operation` as well as their return types more narrow, concretely, a `py::type` instead of a `py::object`. As the name of the arguments indicate, they have to be called with a type instance (a "class"). The PR also updates the typing stubs of these functions (in the corresponding `.pyi` file), such that static type checkers are aware of the changed type. With the previous typing information, `pyright` raised errors on code generated by tablegen. Signed-off-by: Ingo Müller <[email protected]>
Contributor
|
FYI, I think this may cause issues with older versions of pybind11. At least, we saw this failure on CIRCT with pybind11 2.9.1: https://github.com/llvm/circt/actions/runs/11921194262/job/33224788868?pr=7847#step:8:137 I could reproduce that failure with that specific version, and confirmed that reverting this change avoids the failure. For CIRCT, I think we'll probably just use a more modern pybind11 in CI/CD. I did not see this on pybind11 2.11.1, which is what I normally have locally. |
mikeurbach
added a commit
to circt/images
that referenced
this pull request
Nov 21, 2024
We have had issues in 2.10, and it appears a change upstream is no longer compatible with version 2.9: llvm/llvm-project#115307 This updates to 2.11.2, the next minor version, which I have used locally for a long time. This patch version came out months ago.
mikeurbach
added a commit
to llvm/circt
that referenced
this pull request
Nov 22, 2024
This was needed when llvm/llvm-project@3494ee9 landed. Similar changes were made upstream here: https://github.com/llvm/llvm-project/pull/80309/files. Basically, we now expect the width and value arguments to the APInt constructor to be consistent. There is some implicit extension or truncation depending on the signedness of the value, and now a flag to opt into it. With this change I've simply opted into the flag where necessary, rather than thinking hard about how to compute the correct width given the value. In places where the `isSigned` flag took the default value of false, I kept that when I had to explicitly set that flag. There was exactly one place (CalyxHelpers.cpp), where I actually flipped the value of `isSigned`. The comment in the code made me think this erroneously thought the flag was "isUnsigned". Also updated pybind11 version requirement to avoid this: llvm/llvm-project#115307 (comment) Finally, updated a couple handshake cocotb tests to convert to float for comparison to avoid type mismatches.
Contributor
Author
|
Thanks for reporting, @mikeurbach. I can verify: on my machine, MLIR does not build anymore with pybind11 v2.9.1 or v2.9.2. The next version, 2.10.0, does build. I just created #117314 to increase the required version for MLIR. |
ingomueller-net
added a commit
that referenced
this pull request
Nov 23, 2024
This PR updates the minimal required version of pybind11 from 2.9.0 to 2.10.0. New new version is almost 2.5 years old, which is half a year less than the previous version. This change is necessary to support the changes introduced in #115307, which does not compile with pybind11 v.2.9. Signed-off-by: Ingo Müller <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes the
pyClass/dialectClassarguments of the pybind11 functionsregister_dialectandregister_operationas well as their return types more narrow, concretely, apy::typeinstead of apy::object. As the name of the arguments indicate, they have to be called with a type instance (a "class"). The PR also updates the typing stubs of these functions (in the corresponding.pyifile), such that static type checkers are aware of the changed type. With the previous typing information,pyrightraised errors on code generated by tablegen.