Page MenuHomePhabricator

[mlir][WIP] Initial python bindings for C API
ClosedPublic

Authored by zhanghb97 on Aug 6 2020, 3:42 PM.

Details

Summary

Based on the boiler-plate, this patch implements the initial python bindings for C API.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2020, 3:42 PM
zhanghb97 requested review of this revision.Aug 6 2020, 3:42 PM
ftynse added a comment.Aug 7 2020, 8:12 AM

I would really like to see some high-level proposal here. I don't think just transposing C API to Python is the right approach, but I might be convinced otherwise (e.g., we want to expose API one-to-one and then write extra wrapper code in Python instead of writing it in C++ inside the bindings, although I think the amount of C++ is relatively low and the amount of sanity it brings improves debugability significantly).

In particular:

  • how much we want to diverge from the C API to be more Pythonic, and where this should be implemented?
  • what is the ownership model and how it is reflected in the API? (currently, some constructors give the caller a non-owned objects and some others give the caller an owned object with no clear difference)
  • how do we operate on indexed and iterable objects?
  • how do we layer libraries (e.g., do we really want mlir.ir.blah)?
  • what is the error model (Python has exceptions, but C does not so we resort to returning nulls instead).
mlir/lib/Bindings/Python/BindingModules.cpp
24 ↗(On Diff #283752)

How does one destroy a context?

30 ↗(On Diff #283752)

Please run clang-format on your code.

30 ↗(On Diff #283752)

can this use std::string for the filename?

45 ↗(On Diff #283752)

I would rather expose this as a static method "parse"

48 ↗(On Diff #283752)

Why is this a property?

In general, could you provide some documentation on why certain things are considered properties and some other things are functions, and how to decide for the new functions...

56 ↗(On Diff #283752)

Why can't you just have a class, accept it by reference, and take the address of the reference when calling the C API? The object of the class, when passed to the function call, will live long enough for the function call to complete, at which point the operation will have taken ownership of anything it needs anyway.

71–73 ↗(On Diff #283752)

I don't know how much we want to replicate C API versus how much we want to have Pythonic API, personally I am strongly in favor of going full-Python because C language capabilities are extremely limited. Instead of exposing is_null, I'd rather bind __bool__ so that it can be used directly inside if conditions. Instead of exposing regions_num (which I would call num_regions if we decide to keep it), I'd rather have a special "region list" object with __iter__, __getitem__ and __len__ provided, so we can use it with loops and generators, etc. This deserves an RFC and a discussion.

90 ↗(On Diff #283752)

Can this rather take an std::vector (or a py::list) ?

mlir/lib/Bindings/Python/BindingModules.h
1 ↗(On Diff #283752)

If you want to split it into multiple files, let's go all the way and have separate files for the "IR" modules and for the "Registration" module, and also link the libraries separately. Otherwise, it does not make sense to have an additional pair of files that includes everything, just put it into the main file...

15 ↗(On Diff #283752)

I'm not sure passing a py::module by-value is the right approach.

I would really like to see some high-level proposal here. I don't think just transposing C API to Python is the right approach, but I might be convinced otherwise (e.g., we want to expose API one-to-one and then write extra wrapper code in Python instead of writing it in C++ inside the bindings, although I think the amount of C++ is relatively low and the amount of sanity it brings improves debugability significantly).

The current code implements most direct bindings to show the code structure and style, and it's easy to incremental development and change direction. I am also considering how to organize a high-level binding and planning to write a document and RFC.

As for the questions above:

  • how much we want to diverge from the C API to be more Pythonic, and where this should be implemented?

I think Pythonic concept is too vague to describe in words. In general, the mlir python module should follow python's language specification and provide idiomatic python usage, such as python OO style, iterator, exception, etc.

  • what is the ownership model and how it is reflected in the API? (currently, some constructors give the caller a non-owned objects and some others give the caller an owned object with no clear difference)

Would it be better that function returning non-owning object should not be designed as a constructor, but bound as a member function, and the ownership can be identified by function name.

  • how do we operate on indexed and iterable objects?

As for the C++ API, the iterator (__iter__) can be implemented with py::make_iterator, but I don't know how to implement it with C API so far. I think I need some suggestions here.

  • how do we layer libraries (e.g., do we really want mlir.ir.blah)?

The design of the layer libraries need to be futher discussed, I use the submodules based on the feedback. I will add the topic in the RFC.

  • what is the error model (Python has exceptions, but C does not so we resort to returning nulls instead).

I think the error model can be designed with the pybind11 custom exception mechanism. I will try this later.

mlir/lib/Bindings/Python/BindingModules.cpp
24 ↗(On Diff #283752)

I think the python destructor and garbage collection will help us, when we want to destroy a context. Do we need to bind the mlirContextDestroy as the custom destructor? It seems that __del__ may mess with the garbage collector.

30 ↗(On Diff #283752)

Do you mean to use std::string as the parameter type of the constructor?

48 ↗(On Diff #283752)

Those functions ( such as MlirOperation mlirModuleGetOperation(MlirModule); ), with only one parameter as the self in a python class will be bound as property to improve the pythonic style. Also those functions return a non-owning object, which is more suitable as a property. This will be added to the document later.

90 ↗(On Diff #283752)

Yes, a std::vector/py::list will be better.

mlir/lib/Bindings/Python/BindingModules.h
15 ↗(On Diff #283752)

Ah - it should be passed by reference here.

stellaraccident requested changes to this revision.Aug 9 2020, 12:03 PM

Sorry for the delayed review (was offline for a few days). I've made some design-level comments but not done a full review. In general, I agree with Alex's comments and have added more specific feedback. It is quite hard to do a detailed review with all of the clang-format warnings: please make sure to format before sending the patch.

Thanks for working on this.

mlir/lib/Bindings/Python/BindingModules.cpp
24 ↗(On Diff #283752)

There is a design point here worth discussion. In general, when wrapping opaque C-APIs like this, I usually create a C++ class to encapsulate any of the structs that do not have trivial ownership/semantics. In this case, if you had:

class PyMlirContext {
public:
  PyMlirContext() {
    context = mlirContextCreate(...);
  }
  ~PyMlirContext() {
      mlirContextDestroy(...);
  }

  MlirContext context;
};

And then you bound to PyMlirContext, memory management becomes automatic and works with the binding layer as expected. For complicated binding hierarchies, especially from a C-API, I have never not needed a C++ interposer class for most things, and I usually just create it straight-away vs needing to refactor a few patches down the line.

29 ↗(On Diff #283752)

This is not a complete modeling of the MLIR Location hierarchy. There are multiple ways to model it, and hard-coding it to filename/line/col will create problems for most of them. If we don't want to answer all of those questions now, I would omit a constructor entirely and instead provide a static method like for_file that takes (filename, line, col).

Also, +1 on filename being a std::string.

34 ↗(On Diff #283752)

Here and elsewhere, you need to lifetime-extend the MlirContext parameter. See the documentation on keep-alives: https://pybind11.readthedocs.io/en/stable/advanced/functions.html#keep-alive

(this comment applies to all such cases in this patch)

Note that if you don't do this, the following (very common) idiom will likely segfault:

def create_context():
  return ... some initialization...

def do_somethind():
  return mlir.ir.Location(create_context())

In this case, it is less likely to write something like that but for others, it can happen easily.

42 ↗(On Diff #283752)

If we are taking this approach (each type supports direct construction, vs construction via a method on the context), then you will need to update this section: https://mlir.llvm.org/docs/Bindings/Python/#limited-use-of-globals

I have a slight affinity for how I documented it above but could be convinced either way.

42 ↗(On Diff #283752)

You've got a life-time problem here that you will not be able to avoid with the API in this form: you actually want to be adding a keep-alive to the context that underlies the location, but you lack access/knowledge that this is safely castable by pybind to its wrapper type.

This is part of why I prefer these factory methods on the context itself (as in https://mlir.llvm.org/docs/Bindings/Python/#limited-use-of-globals): you then have a place to say something like:

.def("create_module", [](PyMlirContext& self, MlirLocation loc) {
  if (getContextFromLocation(loc) != self.context) {
    ... raise error "Cannot create module from a different context"
  }, py::keep_alive<1, 0>());

});
45 ↗(On Diff #283752)

+1

48 ↗(On Diff #283752)

FWIW, I tried to cover this in the style guide (which is also open for further discussion): https://mlir.llvm.org/docs/Bindings/Python/#properties-vs-get-methods

Imo, function (pairs) that tend to be named get*/set* on the C++ side are more naturally represented as properties in python. Some judgment obviously needs to be applied; however, in general, if tempted to create a Python method like "get_foo", whose only job is to return a contained instance of "foo", it is confusing from the Python perspective to have this as a method.

72–73 ↗(On Diff #283752)

Same comment as below: I would prefer a regions pseudo container.

75–76 ↗(On Diff #283752)

Same comment as below: This is easier to use/read with an operands pseudo-container.

81 ↗(On Diff #283752)

I would strongly prefer an "attributes" pseudo container which implemented the sequence related dunder methods (as described here: https://mlir.llvm.org/docs/Bindings/Python/#prefer-pseudo-containers)

You then have the option to either allow a polymorphic getitem, taking either an int or a str to do the lookup, or a monomorphic one only allowing int access (and providing an aux method like by_name to do the name lookup). I moderately prefer the latter out of a general dislike for APIs that require polymorphism.

89 ↗(On Diff #283752)

It seems like you are missing an operations container. Having it would make more sense on where to anchor a couple of the methods.

96–97 ↗(On Diff #283752)

Personally, I would leave out the mutation related methods in a first patch. I suspect there are multiple ways to handle them and we may want to fiddle with it.

98–99 ↗(On Diff #283752)

Similar comment: would prefer an arguments pseudo container.

This revision now requires changes to proceed.Aug 9 2020, 12:03 PM
mlir/lib/Bindings/Python/BindingModules.h
1 ↗(On Diff #283752)

I am somewhat in favor of splitting apart (my view here https://mlir.llvm.org/docs/Bindings/Python/#composable-modules) and agree that if doing it, it is better to go all the way. It is somewhat important to have some of them separable for special purpose builds (especially for heavy-weight things like registration, execution engine, etc).

zhanghb97 updated this revision to Diff 284251.Aug 9 2020, 8:24 PM
zhanghb97 marked an inline comment as not done.

Reformat the code with clang-format.

zhanghb97 added inline comments.Aug 10 2020, 2:24 AM
mlir/lib/Bindings/Python/BindingModules.h
1 ↗(On Diff #283752)

If you want to split it into multiple files, let's go all the way and have separate files for the "IR" modules and for the "Registration" module, and also link the libraries separately.

I'd like to confirm this, should I split files like that:

  • MainModule.cpp
  • IRModules.h
  • IRModules.cpp
  • RegistrationModule.h
  • RegistrationModule.cpp

and generate libraries for "IR" and "Registration" separately, then link those libraries to "MainModule"?

I would really like to see some high-level proposal here. I don't think just transposing C API to Python is the right approach, but I might be convinced otherwise (e.g., we want to expose API one-to-one and then write extra wrapper code in Python instead of writing it in C++ inside the bindings, although I think the amount of C++ is relatively low and the amount of sanity it brings improves debugability significantly).

The current code implements most direct bindings to show the code structure and style, and it's easy to incremental development and change direction. I am also considering how to organize a high-level binding and planning to write a document and RFC.

Don't wait too much to send an RFC. Many folks don't follow all commit review threads, but can contribute on the forum.

As for the questions above:

  • how much we want to diverge from the C API to be more Pythonic, and where this should be implemented?

I think Pythonic concept is too vague to describe in words. In general, the mlir python module should follow python's language specification and provide idiomatic python usage, such as python OO style, iterator, exception, etc.

"Pythonic" is a relatively well-defined term in Python :) It usually refers to following PEP 20. But yes, using common Python idioms is definitely one of the aspects.

  • what is the ownership model and how it is reflected in the API? (currently, some constructors give the caller a non-owned objects and some others give the caller an owned object with no clear difference)

Would it be better that function returning non-owning object should not be designed as a constructor, but bound as a member function, and the ownership can be identified by function name.

I need to see a specific proposal... Python is garbage-collected with objects passed around by-reference, so ownership is rarely a problem here, we just need to make sure the objects live long enough (see Stella's comments about lifetime extension) and they are properly cleaned up when no longer needed (see my comments about freeing the context). FYI, Python also has language support for context managers, originally intended for system resources that need explicit clean-up; I don't think we need them, I'd use them instead of explicit .delete() or .close() methods.

  • how do we operate on indexed and iterable objects?

As for the C++ API, the iterator (__iter__) can be implemented with py::make_iterator, but I don't know how to implement it with C API so far. I think I need some suggestions here.

Read the documentation on how an iterator is defined in Python (there are two special methods IIRC) and just define those methods. Or read the internals of make_iterator.

  • how do we layer libraries (e.g., do we really want mlir.ir.blah)?

The design of the layer libraries need to be futher discussed, I use the submodules based on the feedback. I will add the topic in the RFC.

  • what is the error model (Python has exceptions, but C does not so we resort to returning nulls instead).

I think the error model can be designed with the pybind11 custom exception mechanism. I will try this later.

mlir/lib/Bindings/Python/BindingModules.cpp
24 ↗(On Diff #283752)

Do we need to bind the mlirContextDestroy as the custom destructor?

Well, currently the bindings code _never_ calls mlirContextDestroy so the context is _never_ destroyed. Python cannot magically know that it has to call this specific function to free the memory allocated elsewhere.

I am used to do the same thing as Stella suggests: add a wrapper class that manages memory for you. Sometimes it makes sense to do that using an explicit unique_ptr or shared_ptr, this is worth discussing.

30 ↗(On Diff #283752)

Yes.

42 ↗(On Diff #283752)

If we are taking this approach (each type supports direct construction, vs construction via a method on the context),

The question I have here is how do we go around dialect-specific types and attributes. We can make them inject their constructors into the context class through __dict__ or other metamagic, but I'm not sure it's the best approach.

48 ↗(On Diff #283752)

FWIW, I tried to cover this in the style guide (which is also open for further discussion): https://mlir.llvm.org/docs/Bindings/Python/#properties-vs-get-methods

Thanks!

I was wondering specifically about getOperation, to me it's a sort of upcast not a getter: Module is-a Operation.

81 ↗(On Diff #283752)

+1

We can also have to pseudo-containers: named_attributes that is indexed by integers and contains name/attribute pairs, and attributes that is indexed by string and contains attributes. The latter should be close to frozendict.

zhanghb97 added inline comments.Aug 11 2020, 4:35 AM
mlir/lib/Bindings/Python/CMakeLists.txt
24

I have trouble of linking the libraries separately here. As the previous feedback showed, multiple files should be linked separately. When I tried like this:

add_library(MLIRIRModulesBindingsPythonExtension ${PYEXT_LINK_MODE}
  IRModules.cpp
)
... ...
target_link_libraries(MLIRIRModulesBindingsPythonExtension
  PRIVATE
  MLIRIR
  MLIRCAPIIR
  MLIRCAPIRegistration
  ${PYEXT_LIBADD}
)

add_library(MLIRBindingsPythonExtension ${PYEXT_LINK_MODE}
  MainModule.cpp
)
... ...
target_link_libraries(MLIRBindingsPythonExtension
  PRIVATE
  MLIRIR
  MLIRIRModulesBindingsPythonExtension
  ${PYEXT_LIBADD}
)

but it reported:

MainModule.cpp:(.text.PyInit__mlir+0x5f0): undefined reference to `populateIRSubmodule(pybind11::module&)'

I don't know why the MainModule can't link the IRModules library?

zhanghb97 updated this revision to Diff 285024.Aug 12 2020, 2:56 AM

Binding MlirModule with wrapper class PyMlirModule.
Prototype of the RFC:
https://llvm.discourse.group/t/rfc-first-step-python-bindings-for-c-api/1578
Meet the requirements of the first step:
https://llvm.discourse.group/t/next-steps-on-python-bindings/1570

zhanghb97 updated this revision to Diff 285311.Aug 13 2020, 3:30 AM

Add py::keep_alive to PyMlirContext::parse to extend the lifetime of context object.
Remove the binding of the PyMlirModule constructor.

stellaraccident requested changes to this revision.Aug 13 2020, 7:19 PM

Almost there. Once you resolve the above comments, let me know if you need me to land it.

mlir/lib/Bindings/Python/CMakeLists.txt
24

Can you leave it as is since it works and add a TODO? I'll clean it up.

mlir/lib/Bindings/Python/IRModules.cpp
20

Just return by value (no new)

mlir/lib/Bindings/Python/IRModules.h
10

I think your header guard is missing a BINDING segment

24

Did you mean to pass this by value?

38

I believe you want to return this by value (not as a pointer). And if you did want to use a pointer, it should return a std::unique_ptr, which would signal to pybind that it takes ownership.

This revision now requires changes to proceed.Aug 13 2020, 7:19 PM
zhanghb97 updated this revision to Diff 285630.Aug 14 2020, 5:13 AM

Use std::unique_ptr as the return type of PyMlirContext::parse.
Modify the header guard.
@stellaraccident I think I need you to help me land it, it's my first patch and I don't have commit access now. Thanks very much!

zhanghb97 added inline comments.Aug 14 2020, 5:15 AM
mlir/lib/Bindings/Python/CMakeLists.txt
24

I add a TODO here, thanks for helping me clean it up!

mlir/lib/Bindings/Python/IRModules.cpp
20

I return a pointer for the same reason as the comment above.
Now I use a std::unique_ptr here.

mlir/lib/Bindings/Python/IRModules.h
10

I think the header guard should be MLIR_BINDINGS_PYTHON_IRMODULES_H.
By the way, we could ignore the clang-tidy error about 'pybind11/pybind11.h', right?

24

The destructor of PyMlirModule is responsible to free the MlirModule object. If we pass by value here, the destructor is able to free the module member (copy of the MlirModule object), I'm not sure if the original MlirModule object will be free, which may cause a memory leak. So I pass by reference here. Is there any harm passing by reference that I didn't notice?

38

Actually, I want to return a pointer. This is because when I tried to return a value here, it reported a segmentation fault. It seems that those two PyMlirModule object ( the object in the parse function and the returned one ) may share the same MlirModule object (maybe the member (the pointer) of the MlirModule object point to same place), so when the parse function terminates, the destructor of PyMlirModule will be called and the MlirModule object will be freed, then the returned PyMlirModule object can't access the freed place.
Thus, in the parse function should avoid calling the destructor of PyMlirModule, so return a pointer is the best choice I think. And now I use the std::unique_ptr to take the ownership.

uenoku added a subscriber: uenoku.Aug 14 2020, 5:22 AM
zhanghb97 added inline comments.Aug 14 2020, 6:07 AM
mlir/lib/Bindings/Python/IRModules.h
10

If we put the file into the llvm-project/mlir/include/mlir/Bindings/Python in the future, the header guard should be MLIR_BINDINGS_PYTHON_IRMODULES_H. If we just put the file in the current directory, the header guard should be LLVM_MLIR_LIB_BINDINGS_PYTHON_IRMODULES_H.

mlir/lib/Bindings/Python/IRModules.h
24

You need to add move semantics to the class (move constructor and associated destructor changes) to make it properly pass by value. I'll see about patching it but may kick it back to you.

38

See my comment about move semantics above.

zhanghb97 updated this revision to Diff 285809.Aug 14 2020, 8:11 PM

Add move constructor of PyMlirModule.
Create a deep copy in the move constructor to avoid double free the object.
Make the parse function return a value.

zhanghb97 added inline comments.Aug 14 2020, 8:23 PM
mlir/lib/Bindings/Python/IRModules.cpp
30

To avoid double free the object by the destructor, the deep copy is needed in the move constructor. There is no C API for the clone() function so far, so I use the C++ API here to implement the deep copy. Should we implement a C API for the clone()` function, or is it happy with using C++ API here?

mlir/lib/Bindings/Python/IRModules.h
24

I have added the move constructor, and the parse function return a value now.

Refactor to proper move constructor and update comments.

stellaraccident accepted this revision.Aug 16 2020, 7:32 PM

Thank you for the attempt at a move constructor, but these things need to be done a specific way and we don't want to be cloning the module. I've reverted that part of the patch and made the PyMlirModule class move-only. Also updated some comments and other non-functional things.

This revision is now accepted and ready to land.Aug 16 2020, 7:32 PM
This revision was landed with ongoing or failed builds.Aug 16 2020, 7:36 PM
This revision was automatically updated to reflect the committed changes.