[Mlir-commits] [mlir] [mlir][python] add dict-style to IR attributes (PR #163200)

[Mehdi Amini]

joker-eph wrote:

I'm a bit puzzled by the line of comments here, which are not really trying to build a shared understanding here. Maybe I can start to elaborate here on why I mentioned that we should discuss a bit more.

So maybe we start with different assumptions or knowledge about the state of the codebase and the general direction. 
First, the python bindings are, as the name indicates, bindings to MLIR. As such I expect that we keep a strong alignment between the evolution of MLIR concepts and APIs, and the bindings: they can't just evolve entirely separately, it's not an independent project. Of course the bindings are offering some more "pythonic" UI as well, often as layer on top of the lowest level of bindings, where things can diverge from some of the C++ layer because of the differences in the languages (I'm thinking about the builders for op with regions for example, or the use of python `with` context, etc.). 

Now, onto the state of discardable/inherent attributes, and more generally properties: it seems that there is a lot of misconceptions about where we are and where we're going. 
While introducing properties, we **also** formally separated discardable and inherent attributes and started reflecting this in the API of the `Operation` class. There are now APIs like `getDiscardableAttrDictionary()`, `getDiscardableAttrs()`, `getRawDictionaryAttrs()`,  ...  and also `getInherentAttr` intended to expose inherent attributes during the transition.
For backward compatibility, we tried to preserve (with some quirks, but mostly working) pre-existing APIs that are mixing-up discardable and inherent attributes: these are fragiles and we need to migrate away from them in order to formally deprecate them and remove them. This in-tree migration is what I referred to earlier as being incomplete: we can't remove the APIs that we're still having in-tree use for! 
However the fact that we still have in-tree users for the old API does not change the fact that we need to promote and encourage APIs that are separating access to inherent and discardable attributes, as these have different storage and really different "namespace" (you can have an attribute with the same "key" for a discardable and an inherent attribute, which would expose how buggy the old API is: one of them would overwrite the other somehow).

Now, getting to Python side, comes the question about these APIs and their exposure: the reason I'm bringing awareness here (and raising some concerns over misalignment) is that while this patch seems like a good idea (it makes something clearly more "pythonic" and is a better UI), it is doing over an API that is on its way to deprecation and for which the replacement **is available in tree for >2 years**. So I'm asking why aren't exposing the new API instead and applying the great improvements in look&feel to the APIs that we want user to actually use. Python has a 2y lag on the C++ API here, and the longer we wait the more brutal the deprecation (and removal) of the older API will be, which will make users of this API less happy, which I suspect is something we all strive to avoid (unnecessarily at least).



https://github.com/llvm/llvm-project/pull/163200