Page MenuHomePhabricator

Make Python MLIR Operation not iterable
AcceptedPublic

Authored by mehdi_amini on Wed, Oct 13, 1:44 AM.

Details

Summary

The current behavior is conveniently allowing to iterate on the regions of an operation
implicitly by exposing an operation as Iterable. However this is also error prone and
code that may intend to iterate on the results or the operands could end up "working"
apparently instead of throwing a runtime error.
The lack of static type checking in Python contributes to the ambiguity here, it seems
safer to not do this and require and explicit qualification to iterate (op.results, op.regions, ...).

Diff Detail

Event Timeline

mehdi_amini created this revision.Wed, Oct 13, 1:44 AM
mehdi_amini requested review of this revision.Wed, Oct 13, 1:44 AM
ftynse accepted this revision.Wed, Oct 13, 2:42 AM

I was tricked by this, too. Please update the docs/Python.md to remove the mention of operations being iterable.

This revision is now accepted and ready to land.Wed, Oct 13, 2:42 AM
mlir/python/mlir/dialects/_builtin_ops_ext.py
201

Oof, this is a nasty bug you uncovered. In general, I get worried about these ad-hoc conversions between op<->results and am undecided on whether we should try to silently convert more (and whether there is a more principled utility function we should use for that, which we can test more invariants of) or raise an error (i.e. "Passed an Operation to an API which expected value(s): Please use either the .result or .results property if intending to access a result.").

Thoughts?

mlir/test/python/dialects/math.py
19

I would argue that this test is wrong and should be fixed. Regardless of whether there is some automatic conversion, this should be:

return mlir_math.SqrtOp(F32Type.get(), arg).result
ftynse added inline comments.Wed, Oct 13, 2:32 PM
mlir/python/mlir/dialects/_builtin_ops_ext.py
201

Utility function like this one https://github.com/llvm/llvm-project/blob/main/mlir/python/mlir/dialects/_ods_common.py#L126? We are already using those in the code generated from ODS.

mlir/test/python/dialects/math.py
19

It isn't surprising to me that there is an automatic conversion to value here. Single-result operations are implicitly convertible to Value in C++, and in Python when used as arguments. (We also convert multi-result operations to lists of arguments in Python). Given that we can write std.ReturnOp(mlir_math.SqrtOp(F32Type.get(), arg)) and it does what I expect it to, having it for the DSL-style embedding makes even more sense.

mehdi_amini added inline comments.Wed, Oct 13, 3:26 PM
mlir/test/python/dialects/math.py
19

It seems that we're being pulled in between two use-cases here:

  • IR building API
  • DSL-style.

The former would prefer to be explicit (we don't pretend to be a language) while the later would want the most "natural language" style.

This revision goes in the first direction, but if we agree that this is the kind of programmability that the bindings aim to offer, we should avoid the DSL-style in every aspect of the API, otherwise this will lead to user confusion (reminds me of TF eager vs TF function).

mlir/python/mlir/dialects/_builtin_ops_ext.py
201

Yes, like that.

mlir/test/python/dialects/math.py
19

I didn't intend to argue the purity of DSL style vs other (I prefer some ergonomics and the op vs value brittleness had cost me a lot of time as a user -- and I built a lot of this stuff).

I would like to use a helper like what Alex points out: I'd prefer to not open code such checks/conversions.

19

Fair point. I guess my complaint is that this is the only thing that broke: it is an unrelated test. The implicit conversion should be tested in the builtin dialect tests along with the rest of the tests for this decorator.

ftynse added inline comments.Thu, Oct 14, 12:31 AM
mlir/test/python/dialects/math.py
19

Re testing: when https://reviews.llvm.org/D111656 lands, it will enable the python_test dialect as a side effect. We can then use that dialect to test infrastructural features such as automatic conversions using that, the builtin dialect doesn't have enough ops to test all the things we need.