MLIR python bindings: making optional attributes into optional python arguments

The tblgen-erated python bindings for operations that have Optional attributes look something like this (using FuncOp as an example):

// FuncOps.td
def FuncOp {
    ...
    let arguments = (ins <other stuff>, OptionalAttr<StrAttr>:$sym_visibility);
}
# _func_ops_gen.py (auto-generated)
class FuncOp :
    # The sym_visibility argument is positional and required
    def __init__(self, <other stuff>, sym_visibility, *, loc=None, ip = None):
        ...
        if sym_visibility is not None: attributes["sym_visibility"] = sym_visibility
        ...

The MLIR python extension mechanism for ops seems to allow one to make the optional attribute into an optional argument as follows:

# _func_ops_ext.py (manually written)
class FuncOp :
    # The visibility argument defaults to None if not provided
    def __init__(self, <other stuff>, *, visibility=None, loc=None, ip= None) :
        sym_visibility = StringAttr.get(str(visibility)) if visibility is not None else None
        # Forward to the tblgen-erated __init__() function
        super().__init__(<other stuff>, sym_visibility, loc=loc, ip=ip)
        ...

It seems like it would be possible to have tblgen generate an __init__() function with None default values for optional attributes (and perhaps default-valued attributes as well). Is this a case of “maybe a good idea, patches welcome,” or “we are intentionally not doing that?”

(This has come up in the process of adding a default-valued fastmath attribute to floating point arithmetic ops. I can use the extension mechanism to provide default arguments for existing Python code that instantiates Arithmetic ops, but I thought a tblgen approach might be more appropriate.)

Yes there is a GitHub issue ([MLIR:Python] Improve ergonomics of op constructor APIs in Python bindings · Issue #54932 · llvm/llvm-project · GitHub) related to this too and agree doing it automatically would be best.

So to answer the question: I think this would be very welcome.

I submitted a diff that addresses this - at least partially - by making all optional arguments (attributes and operands) “keyword-only” arguments in the tblgen-erated python __init__() function.

https://reviews.llvm.org/D124717

It seems like this will break any out-of-tree Python code that uses operators with optional arguments (unless the python classes had already been extended to make them optional), so it might be good to have some extra scrutiny. (Mentioning it here in case discourse will draw more objections than the diff would…)

This is very much in “patches welcome” state and not intentional. In general, for the python op apis with more than a trivial/obvious arrangement of arguments, we should be pushing people to keyword arguments. That is the only way to make these APIs robust to evolution.

Some non trivial uses have emerged downstream. Would you mind if we hold your patch for the regular work week to give people time to assess? If too disruptive, we may want to do it in a couple of steps. Specifically, I know that because of the current arrangement, some folks have been forced to over specify arguments and it would be good to flush those cases out.

@jdd @mikeurbach

No problem - let me know if I can be of any help.

This seems like an improvement. It probably won’t be till Monday but I am happy to take a look and see if there’s any implication for CIRCT. Even if it breaks the current usage, sounds like that could be worth it.

I agree. Thanks for the tag @stellaraccident. We’ll fix what breaks. Should be pretty easy.

@stellaraccident Do you want to continue to let this “soak”, or should we try to move forward with the changes?

(I’m not sure if anyone has actually reviewed the actual implementation code, which is understandable given the potential downstream impacts.)

https://reviews.llvm.org/D124717

Let me review the code today, and then I’m fine proceeding. It seems like other ideas are more nebulous. I would prefer Mike taking it for a spin in circt before landing, though.

I am happy to, sorry I’ve been dragging my heels on this one. I’m out today and tomorrow, but will prioritize for early next week.