This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse][python] make imports more selective
ClosedPublic

Authored by aartbik on Aug 13 2021, 1:56 PM.

Diff Detail

Event Timeline

aartbik created this revision.Aug 13 2021, 1:56 PM
aartbik requested review of this revision.Aug 13 2021, 1:56 PM
bixia added inline comments.Aug 13 2021, 3:37 PM
mlir/test/python/dialects/sparse_tensor/test_SpMM.py
3–4

I don't see any use of this import?

5

This is the same as
from mlir import ir

I couldn't find any thing in the style guide that says which one is preferable. But there are examples for the style I mentioned in the style guide and I couldn't find an example for the style used here.

6–7

Similar to the previous comment, these can be

from mlir.dialects import sparse_tensor as st
from mlir import runtime as rt

18

This comment is for the import grouping. I couldn't find anything from the google python guide, but I found something from https://www.python.org/dev/peps/pep-0008/#imports

Imports should be grouped in the following order:

Standard library imports.
Related third party imports.
Local application/library specific imports.
You should put a blank line between each group of imports.

18

from mlir.dialects.linalg.opdsl import lang
or
from mlir.dialects.linalg.opdsl import lang as dsl
?

aartbik updated this revision to Diff 366371.Aug 13 2021, 4:17 PM
aartbik marked 4 inline comments as done.

comments

mlir/test/python/dialects/sparse_tensor/test_SpMM.py
3–4

This is a kitchen sink for registering all passes as side effect (useful until every pass has its own proper import).

5

Changed. I was also not sure which one is generally preferred.

aartbik marked an inline comment as done.Aug 13 2021, 4:38 PM
aartbik added inline comments.
mlir/test/python/dialects/sparse_tensor/test_SpMM.py
18

I though about that, but then

def matmul_dsl(

  A=TensorDef(T, S.M, S.K),
  B=TensorDef(T, S.K, S.N),
  C=TensorDef(T, S.M, S.N, output=True)):
C[D.m, D.n] += A[D.m, D.k] * B[D.k, D.n]

loses a lot of its charm, since T, S, etc. need prefix
All other examples I could find use .* for this particular one.

WDYT?

aartbik updated this revision to Diff 366382.Aug 13 2021, 5:26 PM
aartbik marked an inline comment as done.

explicitly name all linalg.opdsl.lang symbols

mlir/test/python/dialects/sparse_tensor/test_SpMM.py
18

How about this for mlir.dialects.linalg.opdsl.lang? Just name everything we import, so that the actual DSL def below stays compact.

bixia added inline comments.Aug 16 2021, 7:55 AM
mlir/test/python/dialects/sparse_tensor/test_SpMM.py
24–28

I understand that you want to import the individual types to simplify the use syntax, but python style guide says this:

Use import statements for packages and modules only, not for individual classes or functions. Imports from the typing module, typing_extensions module, and the six.moves module are exempt from this rule.

aartbik marked an inline comment as done.Aug 16 2021, 10:47 AM
aartbik added inline comments.
mlir/test/python/dialects/sparse_tensor/test_SpMM.py
24–28

There seems to be various style guides floating around, and they all say slightly different things.
Okay, here is one last attempt, now with just namespace imports and full prefixes....

aartbik updated this revision to Diff 366673.Aug 16 2021, 10:48 AM
aartbik marked an inline comment as done.

names only in imports

bixia accepted this revision.Aug 16 2021, 11:27 AM
bixia added inline comments.
mlir/test/python/dialects/sparse_tensor/test_SpMM.py
18

I like the concise syntax as well. I am fine with what you have right now, or adding a comment to this import and use the concise syntax.

from mlir.dialects.linalg.opdsl.lang import *

This revision is now accepted and ready to land.Aug 16 2021, 11:27 AM
aartbik marked an inline comment as done.Aug 16 2021, 11:52 AM
aartbik added inline comments.
mlir/test/python/dialects/sparse_tensor/test_SpMM.py
18

Yeah, I am on the fence too. But let's to the right thing here.
We can always later introduce a style guide rule for our dsl imports ;-)

This revision was automatically updated to reflect the committed changes.
aartbik marked an inline comment as done.