[mlir][python] add dict-style to IR attributes #163200
Conversation
Wheest
requested review from
ftynse,
makslevental,
rolfmorel and
stellaraccident
as code owners
|
@llvm/pr-subscribers-mlir Author: Perry Gibson (Wheest) ChangesIt makes sense that Attribute dicts/maps should behave like dicts in the Python bindings. Previously this was not the case. Full diff: https://github.com/llvm/llvm-project/pull/163200.diff 2 Files Affected:
diff --git a/mlir/lib/Bindings/Python/IRCore.cpp b/mlir/lib/Bindings/Python/IRCore.cpp
index 7b1710656243a..a94674079bf1c 100644
--- a/mlir/lib/Bindings/Python/IRCore.cpp
+++ b/mlir/lib/Bindings/Python/IRCore.cpp
@@ -2730,6 +2730,16 @@ class PyOpAttributeMap {
operation->get(), toMlirStringRef(name)));
}
+ template <typename F>
+ auto forEachAttr(F fn) {
+ intptr_t n = mlirOperationGetNumAttributes(operation->get());
+ for (intptr_t i = 0; i < n; ++i) {
+ MlirNamedAttribute na = mlirOperationGetAttribute(operation->get(), i);
+ MlirStringRef name = mlirIdentifierStr(na.name);
+ fn(name, na.attribute);
+ }
+ }
+
static void bind(nb::module_ &m) {
nb::class_<PyOpAttributeMap>(m, "OpAttributeMap")
.def("__contains__", &PyOpAttributeMap::dunderContains)
@@ -2737,7 +2747,42 @@ class PyOpAttributeMap {
.def("__getitem__", &PyOpAttributeMap::dunderGetItemNamed)
.def("__getitem__", &PyOpAttributeMap::dunderGetItemIndexed)
.def("__setitem__", &PyOpAttributeMap::dunderSetItem)
- .def("__delitem__", &PyOpAttributeMap::dunderDelItem);
+ .def("__delitem__", &PyOpAttributeMap::dunderDelItem)
+ .def("__iter__",
+ [](PyOpAttributeMap &self) {
+ nb::list keys;
+ self.forEachAttr([&](MlirStringRef name, MlirAttribute) {
+ keys.append(nb::str(name.data, name.length));
+ });
+ return nb::iter(keys);
+ })
+ .def("keys",
+ [](PyOpAttributeMap &self) {
+ nb::list out;
+ self.forEachAttr([&](MlirStringRef name, MlirAttribute) {
+ out.append(nb::str(name.data, name.length));
+ });
+ return out;
+ })
+ .def("values",
+ [](PyOpAttributeMap &self) {
+ nb::list out;
+ self.forEachAttr([&](MlirStringRef, MlirAttribute attr) {
+ out.append(PyAttribute(self.operation->getContext(), attr)
+ .maybeDownCast());
+ });
+ return out;
+ })
+ .def("items", [](PyOpAttributeMap &self) {
+ nb::list out;
+ self.forEachAttr([&](MlirStringRef name, MlirAttribute attr) {
+ out.append(
+ nb::make_tuple(nb::str(name.data, name.length),
+ PyAttribute(self.operation->getContext(), attr)
+ .maybeDownCast()));
+ });
+ return out;
+ });
}
private:
diff --git a/mlir/test/python/ir/operation.py b/mlir/test/python/ir/operation.py
index cb4cfc8c8a6ec..9d49cb1d25f9e 100644
--- a/mlir/test/python/ir/operation.py
+++ b/mlir/test/python/ir/operation.py
@@ -569,14 +569,31 @@ def testOperationAttributes():
# CHECK: Attribute value b'text'
print(f"Attribute value {sattr.value_bytes}")
+ # Python dict-style iteration
# We don't know in which order the attributes are stored.
- # CHECK-DAG: NamedAttribute(dependent="text")
- # CHECK-DAG: NamedAttribute(other.attribute=3.000000e+00 : f64)
- # CHECK-DAG: NamedAttribute(some.attribute=1 : i8)
- for attr in op.attributes:
- print(str(attr))
+ # CHECK-DAG: dependent
+ # CHECK-DAG: other.attribute
+ # CHECK-DAG: some.attribute
+ for name in op.attributes:
+ print(name)
+
+ # Basic dict-like introspection
+ # CHECK: True
+ print("some.attribute" in op.attributes)
+ # CHECK: False
+ print("missing" in op.attributes)
+ # CHECK: Keys: ['dependent', 'other.attribute', 'some.attribute']
+ print("Keys:", sorted(op.attributes.keys()))
+ # CHECK: Values count 3
+ print("Values count", len(op.attributes.values()))
+ # CHECK: Items count 3
+ print("Items count", len(op.attributes.items()))
+
+ # Dict() conversion test
+ d = {k: v.value for k, v in dict(op.attributes).items()}
+ # CHECK: Dict mapping {'dependent': 'text', 'other.attribute': 3.0, 'some.attribute': 1}
+ print("Dict mapping", d)
- # Check that exceptions are raised as expected.
try:
op.attributes["does_not_exist"]
except KeyError:
|
|
One general remark around handling of this in the bindings is that there is some underlying changes planned on the whole infra around attributes for the migration to properties which are to incur some future important breaking changes to code that exposes inherent attributes in a dictionary fashion. |
Thanks, I wasn’t aware of this proposal. From what I gather, it moves an operation’s inherent data into a separate property dictionary, while the attribute dictionary will continue to hold discardable data. Since the attribute dict still exists in dictionary form, the feature in this PR should remain functional. Once the property dict is merged, we’d likely want to expose equivalent behavior there too. |
It's been coming for 2y, but we've been slow to make significant progress, but still felt it was a good idea to raise awareness on it :)
Almost: inherent properties aren't necessarily a dictionary, it can be anything: it's "raw C++" and may not offer a key-value pair easy access. Note also: this is for a large part already implemented. The fact that you exposes an "attributes" API is relying on the code in MLIR dynamically trying to merge it all in a DictionnaryAttr on the fly. |
The Python bindings use the C API so I don't see how we can support arbitrary "raw C++". |
Given that we will have to update a lot of this code when the migration is complete there's no reason to block on it (the migration) for an improvement in UX that's only marginally coupled. |
Through generation of C bindings for properties? |
If you're improving the UX of an API we should instead deprecate and replace, then I don't agree. |
When you have a replacement, we can consider your alternative. |
As usual I'm not sure where you draw the authority to block other people's work given purely "feelings" based arguments. As opposed to feelings, here are facts:
I look forward to your discussion but until then (until you provide actual technical reasons), I'd ask you to remove the block. |
There was a problem hiding this comment.
Looks fine to me.
As far as I can tell, this change doesn't really further engrain the usage of the non-properties attributes API. It just completes the set of methods the attribute map, as a Python dict-like, is expected to have. That is, the existing implementation, i.e. the other methods, have as much of a dependency on this API as the new methods.
While it would be nice to make progress on the properties-aware API, I don't see why this particular PR would need to be blocked until that progress is made.
|
This changes existing support to be less surprising for Python users, but doesn't add anything new.
Yes but also not necessarily issue for Python API: for inherent attributes one can generate a dictionary still and populate it. This is within the control of bindings. So the functionality here can still be provided in that world with a change on how bindings are generated. And users can be migrated without breaking iterators here. For unregistered operations I believe there is just a single attrdict. And it doesn't preclude whether one then iterates over intrinsic vs discardable, or both, but that's again possible to stage so that there are less breakages of users. Python API doesn't have stability guarantees, but we can avoid breaking many.
That would seem to require a superset of what Nanobind can do (automatically generate C API and then generate Python API to access arbitrary C++ via C API). That seems really tricky. Attributes work as most are built up of very simple and constrained set. This is a hard blocker for migration to Properties but not related to this change IMHO. |
|
This:
And:
And:
Are really strong reasons to accept this change for what it is, allowing current users (attributes) to have a better experience, since there's no telling when (or even if) the migration to properties will happen. This is a common situation in LLVM, where multiple implementations co-exist for some time, and it's up to the maintainers to keep up with the churn. Since @makslevental volunteered to do that work, I see no reason to block this PR from merging. |
|
I'm a bit puzzled by the line of comments here, which don't seem like trying to build a shared understanding here and making progress. 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. 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. 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). |
The answer has already been provided to you: because this patch is wholly orthogonal to that work. Let me put it in terms of a very simple metaphor: if the foundation of my house needs replacing, I can still fix the lock on my door (to replace a faulty mechanism, to prevent thieves, to make ingress/egress easier for laborers working on the foundation, etc) even if the new foundation will require eventually a new door frame.
I don't know about others in this PR but I certainly do not feel encouraged when someone shows up and demands people interrupt their work to satisfy arbitrary, unrelated goals. In fact, I feel extremely discouraged about my (and others') ability to make meaningful progress on work without constantly having to satisfy someone's demands. Furthermore, as we always say: "patches always welcome". If you have been dissatisfied for an entire 2 years with the relative lag of the bindings behind core (in our disuse of the new APIs), you should have felt at liberty to send PRs rectifying our disuse! And your own dissatisfaction! In fact you should feel free even now, today, right this moment to send any such PRs!
If your actual concern is the user facing API, then I feel you do not understand the patch itself (as I often feel at these impasses) - this PR does not in any way change the actual user facing API. What it does is complete the currently incomplete implementation of the Thus, I am once again asking you, politely, to remove the block, and move this discussion elsewhere (e.g., a discourse post). |
7 participants
It makes sense that Attribute dicts/maps should behave like dicts in the Python bindings. Previously this was not the case.