This is an archive of the discontinued LLVM Phabricator instance.

Differential D150782

[NFC][Py Reformat] Reformat python files in mlir subdir
ClosedPublic

Authored by thieta on May 17 2023, 7:55 AM.

Details

Reviewers
jhenderson
barannikov88
JDevlieghere
MatzeB
mehdi_amini
antiagainst
rriddle
aartbik
ftynse
ThomasRaoux
dcaballe
nicolasvasilache
herhut
Commits
rGf9008e6366c2: [NFC][Py Reformat] Reformat python files in mlir subdir
Summary

This is an ongoing series of commits that are reformatting our
Python code.

Reformatting is done with black.

If you end up having problems merging this commit because you
have made changes to a python file, the best way to handle that
is to run git checkout --ours <yourfile> and then reformat it
with black.

If you run into any problems, post to discourse about it and
we will try to help.

RFC Thread below:

https://discourse.llvm.org/t/rfc-document-and-standardize-python-code-style

Diff Detail

Event Timeline

thieta created this revision.May 17 2023, 7:55 AM
Herald added a reviewer: antiagainst. · View Herald TranscriptMay 17 2023, 7:55 AM
Herald added a reviewer: rriddle. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ThomasRaoux. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: bviyer, hanchung, jsetoain and 29 others. · View Herald Transcript
thieta requested review of this revision.May 17 2023, 7:55 AM
Herald added a reviewer: nicolasvasilache. · View Herald TranscriptMay 17 2023, 7:56 AM
Herald added a reviewer: herhut. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: • pcwang-thead, stephenneuendorffer, nicolasvasilache. · View Herald Transcript
Harbormaster completed remote builds in B232613: Diff 523052.May 17 2023, 8:20 AM
thieta updated this revision to Diff 523679.May 19 2023, 12:37 AM

Updated diff to include all the missing files.

Rebase

Herald added a subscriber: jdoerfert. · View Herald TranscriptMay 19 2023, 12:37 AM
mehdi_amini added a comment.May 19 2023, 1:01 AM

Reformatting is done with black.

Can you make it more clear by providing the exact invocation?

If you end up having problems merging this commit because you

have made changes to a python file, the best way to handle that
is to run git checkout --ours <yourfile> and then reformat it
with black.

ours and theirs are reversed in a rebase: anyone who has on-going work and rebase on main will lose their work with git checkout --ours when encountering a conflict during a rebase, wouldn't they?

Harbormaster completed remote builds in B233101: Diff 523679.May 19 2023, 1:01 AM
mehdi_amini added a comment.May 19 2023, 1:02 AM

(otherwise, fine with me, but @ftynse should probably approve this)

ftynse added a comment.May 22 2023, 8:54 AM

MLIR has been using YAPF with Google style for a while for Python code. Running that results in a relatively clean diff (https://reviews.llvm.org/D151112) compared to near-complete reformatting here. I wonder if it would be preferable to stick to the existing style here. If not, we should also remove the format files like this one: https://github.com/llvm/llvm-project/blob/main/mlir/python/.style.yapf. I'll post on discourse.

mehdi_amini added a comment.May 22 2023, 3:50 PM
In D150782#4361096, @ftynse wrote:

MLIR has been using YAPF with Google style for a while for Python code. Running that results in a relatively clean diff (https://reviews.llvm.org/D151112) compared to near-complete reformatting here. I wonder if it would be preferable to stick to the existing style here. If not, we should also remove the format files like this one: https://github.com/llvm/llvm-project/blob/main/mlir/python/.style.yapf. I'll post on discourse.

I am more concerned about divergence within the project than a one time diff here (it'll be easier to benefit from a pre-merge linter, have a unified doc and workflow for contributors, etc), and it is true that historically the LLVM project has been discouraging mass-fixes like this one, but MLIR has been more aggressive with this kind of mass clang-tidy / clang-format fixes as well.

Let's be more practical: looking at the diff, do you see regressions in readability / maintainability with the new format @ftynse ?
I see mostly neutral to positive changes.

There a things were neither the old nor the new one are optimal to me, but neither are obviously worse than the other, for example:

if (
    isinstance(attribute, types.FunctionType)
    and attribute_name.startswith(function_prefix)
):
    module_functions.append(attribute)

becomes:

if isinstance(attribute, types.FunctionType) and attribute_name.startswith(
    function_prefix
):
    module_functions.append(attribute)

I would likely prefer to split on the and somehow:

if (isinstance(attribute, types.FunctionType) and
    attribute_name.startswith(function_prefix)):
    module_functions.append(attribute)

Anyway, seems like the migration to black in LLVM is already pretty far, unless we find problems with it in the diff, I think we should just land this!

thieta updated this revision to Diff 524578.May 22 2023, 11:41 PM

Rebase

Harbormaster completed remote builds in B233782: Diff 524578.May 22 2023, 11:59 PM
ftynse added a comment.May 23 2023, 1:00 AM

I am more concerned about divergence within the project than a one time diff here (it'll be easier to benefit from a pre-merge linter, have a unified doc and workflow for contributors, etc), and it is true that historically the LLVM project has been discouraging mass-fixes like this one, but MLIR has been more aggressive with this kind of mass clang-tidy / clang-format fixes as well.

I'm on the same page regarding long-term divergence here and personally not opposed to the change, just want to make everyone is aware of the change.

Let's be more practical: looking at the diff, do you see regressions in readability / maintainability with the new format @ftynse ?

I prefer two-space over four-space indentation given how bad Python is with line splitting, but this is nitpicking.

thieta updated this revision to Diff 525498.May 25 2023, 2:19 AM

rebase

thieta added a comment.May 25 2023, 2:20 AM

Can we land this now? It's the one outstanding diff for closing out this reformat.

Harbormaster completed remote builds in B234435: Diff 525498.May 25 2023, 2:58 AM
mehdi_amini added a comment.May 25 2023, 3:44 PM

Please do! Thanks for driving this through, and thanks for the patience.

thieta updated this revision to Diff 525948.May 25 2023, 11:05 PM

rebase

This revision was not accepted when it landed; it landed in state Needs Review.May 25 2023, 11:05 PM
This revision was landed with ongoing or failed builds.
Closed by commit rGf9008e6366c2: [NFC][Py Reformat] Reformat python files in mlir subdir (authored by thieta). · Explain Why
This revision was automatically updated to reflect the committed changes.
thieta added a commit: rGf9008e6366c2: [NFC][Py Reformat] Reformat python files in mlir subdir.
Harbormaster completed remote builds in B234773: Diff 525948.May 25 2023, 11:29 PM

Large Diff

This large diff affects 163 files. Files without inline comments have been collapsed. Expand All Files

Revision Contents

PathSize
mlir/
benchmark/
python/
29 lines
30 lines
examples/
standalone/
test/
CAPI/
2 lines
43 lines
python/
4 lines
19 lines
python/
mlir/
_mlir_libs/
189 lines
dialects/
99 lines
57 lines
20 lines
575 lines
54 lines
213 lines
46 lines
199 lines
262 lines
428 lines
187 lines
545 lines
62 lines
227 lines
linalg/
opdsl/
97 lines
lang/
373 lines
1202 lines
846 lines
288 lines
1078 lines
200 lines
44 lines
43 lines
ops/
2175 lines
6 lines
transform/
18 lines
58 lines
67 lines
runtime/
175 lines
test/
CAPI/
2 lines
Conversion/
GPUToCUDA/
2 lines
GPUToROCm/
2 lines
Examples/
Toy/
Ch6/
4 lines
Ch7/
4 lines
2 lines
standalone/
9 lines
Integration/
Dialect/
Async/
CPU/
2 lines
LLVMIR/
CPU/
X86/
2 lines
18 lines
SparseTensor/
CPU/
14 lines
GPU/
CUDA/
2 lines
python/
4 lines
227 lines
211 lines
83 lines
108 lines
435 lines
tools/
106 lines
53 lines
taco/
4 lines
14 lines
20 lines
14 lines
14 lines
57 lines
8 lines
26 lines
30 lines
4 lines
tools/
3702 lines
90 lines
560 lines
52 lines
50 lines
743 lines
118 lines
128 lines
Vector/
CPU/
AMX/
6 lines
ArmSME/
2 lines
ArmSVE/
2 lines
X86Vector/
6 lines
GPU/
CUDA/
2 lines
GPU/
CUDA/
TensorCore/
2 lines
2 lines
ROCM/
4 lines
29 lines
Unit/
38 lines
lib/
Dialect/
Test/
2 lines
Transform/
2 lines
Tools/
PDLL/
2 lines
Transforms/
2 lines
190 lines
mlir-cpu-runner/
9 lines
mlir-pdll-lsp-server/
2 lines
mlir-pdll/
4 lines
mlir-spirv-cpu-runner/
2 lines
mlir-vulkan-runner/
2 lines
python/
3 lines
dialects/
18 lines
13 lines
416 lines
32 lines
90 lines
13 lines
linalg/
opdsl/
12 lines
21 lines
7 lines
69 lines
78 lines
297 lines
233 lines
237 lines
4 lines
12 lines
19 lines
239 lines
33 lines
82 lines
30 lines
352 lines
237 lines
634 lines
198 lines
119 lines
58 lines
sparse_tensor/
165 lines
16 lines
214 lines
266 lines
116 lines
328 lines
89 lines
684 lines
integration/
dialects/
linalg/
700 lines
ir/
563 lines
378 lines
483 lines
794 lines
189 lines
908 lines
159 lines
46 lines
301 lines
140 lines
122 lines
209 lines
227 lines
203 lines
183 lines
1405 lines
234 lines
358 lines
6 lines
188 lines
tblgen-lsp-server/
2 lines
utils/
gdb-scripts/
327 lines
421 lines
jupyter/
mlir_opt_kernel/
1 line
26 lines
81 lines
lldb-scripts/
9 lines
mbr/
mbr/
1 line
19 lines
18 lines
8 lines
spirv/
2048 lines

Diff 525949

mlir/benchmark/python/benchmark_sparse.py

mlir/benchmark/python/common.py

mlir/examples/standalone/test/CAPI/lit.local.cfg

mlir/examples/standalone/test/lit.cfg.py

mlir/examples/standalone/test/python/lit.local.cfg

mlir/examples/standalone/test/python/smoketest.py

mlir/python/mlir/_mlir_libs/__init__.py

mlir/python/mlir/dialects/_arith_ops_ext.py

mlir/python/mlir/dialects/_bufferization_ops_ext.py

mlir/python/mlir/dialects/_builtin_ops_ext.py

mlir/python/mlir/dialects/_func_ops_ext.py

mlir/python/mlir/dialects/_linalg_ops_ext.py

mlir/python/mlir/dialects/_loop_transform_ops_ext.py

mlir/python/mlir/dialects/_memref_ops_ext.py

mlir/python/mlir/dialects/_ml_program_ops_ext.py

mlir/python/mlir/dialects/_ods_common.py

mlir/python/mlir/dialects/_pdl_ops_ext.py

mlir/python/mlir/dialects/_scf_ops_ext.py

mlir/python/mlir/dialects/_structured_transform_ops_ext.py

mlir/python/mlir/dialects/_tensor_ops_ext.py

mlir/python/mlir/dialects/_transform_ops_ext.py

mlir/python/mlir/dialects/linalg/opdsl/dump_oplib.py

mlir/python/mlir/dialects/linalg/opdsl/lang/affine.py

mlir/python/mlir/dialects/linalg/opdsl/lang/comprehension.py

mlir/python/mlir/dialects/linalg/opdsl/lang/config.py

mlir/python/mlir/dialects/linalg/opdsl/lang/dsl.py

mlir/python/mlir/dialects/linalg/opdsl/lang/emitter.py

mlir/python/mlir/dialects/linalg/opdsl/lang/scalar_expr.py

mlir/python/mlir/dialects/linalg/opdsl/lang/types.py

mlir/python/mlir/dialects/linalg/opdsl/lang/yaml_helper.py

mlir/python/mlir/dialects/linalg/opdsl/ops/core_named_ops.py

mlir/python/mlir/dialects/python_test.py

mlir/python/mlir/dialects/transform/__init__.py

mlir/python/mlir/execution_engine.py

mlir/python/mlir/ir.py

mlir/python/mlir/runtime/np_to_memref.py

mlir/test/CAPI/lit.local.cfg

mlir/test/Conversion/GPUToCUDA/lit.local.cfg

mlir/test/Conversion/GPUToROCm/lit.local.cfg

mlir/test/Examples/Toy/Ch6/lit.local.cfg

mlir/test/Examples/Toy/Ch7/lit.local.cfg

mlir/test/Examples/lit.local.cfg

mlir/test/Examples/standalone/lit.local.cfg

mlir/test/Integration/Dialect/Async/CPU/lit.local.cfg

mlir/test/Integration/Dialect/LLVMIR/CPU/X86/lit.local.cfg

mlir/test/Integration/Dialect/LLVMIR/CPU/lit.local.cfg

mlir/test/Integration/Dialect/SparseTensor/CPU/lit.local.cfg

mlir/test/Integration/Dialect/SparseTensor/GPU/CUDA/lit.local.cfg

mlir/test/Integration/Dialect/SparseTensor/python/lit.local.cfg

mlir/test/Integration/Dialect/SparseTensor/python/test_SDDMM.py

mlir/test/Integration/Dialect/SparseTensor/python/test_SpMM.py

mlir/test/Integration/Dialect/SparseTensor/python/test_elementwise_add_sparse_output.py

mlir/test/Integration/Dialect/SparseTensor/python/test_output.py

mlir/test/Integration/Dialect/SparseTensor/python/test_stress.py

mlir/test/Integration/Dialect/SparseTensor/python/tools/np_to_sparse_tensor.py

mlir/test/Integration/Dialect/SparseTensor/python/tools/sparse_compiler.py

mlir/test/Integration/Dialect/SparseTensor/taco/lit.local.cfg

mlir/test/Integration/Dialect/SparseTensor/taco/test_MTTKRP.py

mlir/test/Integration/Dialect/SparseTensor/taco/test_SDDMM.py

mlir/test/Integration/Dialect/SparseTensor/taco/test_SpMM.py

mlir/test/Integration/Dialect/SparseTensor/taco/test_SpMV.py

mlir/test/Integration/Dialect/SparseTensor/taco/test_Tensor.py

mlir/test/Integration/Dialect/SparseTensor/taco/test_scalar_tensor_algebra.py

mlir/test/Integration/Dialect/SparseTensor/taco/test_tensor_complex.py

mlir/test/Integration/Dialect/SparseTensor/taco/test_tensor_types.py

mlir/test/Integration/Dialect/SparseTensor/taco/test_true_dense_tensor_algebra.py

mlir/test/Integration/Dialect/SparseTensor/taco/tools/mlir_pytaco.py

mlir/test/Integration/Dialect/SparseTensor/taco/tools/mlir_pytaco_io.py

mlir/test/Integration/Dialect/SparseTensor/taco/tools/mlir_pytaco_utils.py

mlir/test/Integration/Dialect/SparseTensor/taco/tools/mlir_sparse_compiler.py

mlir/test/Integration/Dialect/SparseTensor/taco/tools/testing_utils.py

mlir/test/Integration/Dialect/SparseTensor/taco/unit_test_tensor_core.py

mlir/test/Integration/Dialect/SparseTensor/taco/unit_test_tensor_io.py

mlir/test/Integration/Dialect/SparseTensor/taco/unit_test_tensor_utils.py

mlir/test/Integration/Dialect/Vector/CPU/AMX/lit.local.cfg

mlir/test/Integration/Dialect/Vector/CPU/ArmSME/lit.local.cfg

mlir/test/Integration/Dialect/Vector/CPU/ArmSVE/lit.local.cfg

mlir/test/Integration/Dialect/Vector/CPU/X86Vector/lit.local.cfg

mlir/test/Integration/Dialect/Vector/GPU/CUDA/lit.local.cfg

mlir/test/Integration/GPU/CUDA/TensorCore/lit.local.cfg

mlir/test/Integration/GPU/CUDA/lit.local.cfg

mlir/test/Integration/GPU/ROCM/lit.local.cfg

mlir/test/Integration/lit.local.cfg

mlir/test/Unit/lit.cfg.py

mlir/test/lib/Dialect/Test/lit.local.cfg

mlir/test/lib/Dialect/Transform/lit.local.cfg

mlir/test/lib/Tools/PDLL/lit.local.cfg

mlir/test/lib/Transforms/lit.local.cfg

mlir/test/lit.cfg.py

mlir/test/mlir-cpu-runner/lit.local.cfg

mlir/test/mlir-pdll-lsp-server/lit.local.cfg

mlir/test/mlir-pdll/lit.local.cfg

mlir/test/mlir-spirv-cpu-runner/lit.local.cfg

mlir/test/mlir-vulkan-runner/lit.local.cfg

mlir/test/python/develoment_files.py

mlir/test/python/dialects/arith_dialect.py

mlir/test/python/dialects/async_dialect.py

mlir/test/python/dialects/builtin.py

mlir/test/python/dialects/complex_dialect.py

mlir/test/python/dialects/func.py

mlir/test/python/dialects/gpu.py

mlir/test/python/dialects/linalg/opdsl/arguments.py

mlir/test/python/dialects/linalg/opdsl/assignments.py

mlir/test/python/dialects/linalg/opdsl/doctests.py

mlir/test/python/dialects/linalg/opdsl/emit_convolution.py

mlir/test/python/dialects/linalg/opdsl/emit_fill.py

mlir/test/python/dialects/linalg/opdsl/emit_matmul.py

mlir/test/python/dialects/linalg/opdsl/emit_misc.py

mlir/test/python/dialects/linalg/opdsl/emit_pooling.py

mlir/test/python/dialects/linalg/opdsl/lit.local.cfg

mlir/test/python/dialects/linalg/opdsl/metadata.py

mlir/test/python/dialects/linalg/opdsl/shape_maps_iteration.py

mlir/test/python/dialects/linalg/ops.py

mlir/test/python/dialects/math_dialect.py

mlir/test/python/dialects/memref.py

mlir/test/python/dialects/ml_program.py

mlir/test/python/dialects/ods_helpers.py

mlir/test/python/dialects/pdl_ops.py

mlir/test/python/dialects/python_test.py

mlir/test/python/dialects/quant.py

mlir/test/python/dialects/scf.py

mlir/test/python/dialects/shape.py

mlir/test/python/dialects/sparse_tensor/dialect.py

mlir/test/python/dialects/sparse_tensor/passes.py

mlir/test/python/dialects/tensor.py

mlir/test/python/dialects/transform.py

mlir/test/python/dialects/transform_loop_ext.py

mlir/test/python/dialects/transform_structured_ext.py

mlir/test/python/dialects/vector.py

mlir/test/python/execution_engine.py

mlir/test/python/integration/dialects/linalg/opsrun.py

mlir/test/python/ir/affine_expr.py

mlir/test/python/ir/affine_map.py

mlir/test/python/ir/array_attributes.py

mlir/test/python/ir/attributes.py

mlir/test/python/ir/blocks.py

mlir/test/python/ir/builtin_types.py

mlir/test/python/ir/context_managers.py

mlir/test/python/ir/debug.py

mlir/test/python/ir/diagnostic_handler.py

mlir/test/python/ir/dialects.py

mlir/test/python/ir/exception.py

mlir/test/python/ir/insertion_point.py

mlir/test/python/ir/integer_set.py

mlir/test/python/ir/location.py

mlir/test/python/ir/module.py

mlir/test/python/ir/operation.py

mlir/test/python/ir/symbol_table.py

mlir/test/python/ir/value.py

mlir/test/python/lit.local.cfg

mlir/test/python/pass_manager.py

mlir/test/tblgen-lsp-server/lit.local.cfg

mlir/utils/gdb-scripts/prettyprinters.py

mlir/utils/generate-test-checks.py

mlir/utils/jupyter/mlir_opt_kernel/__main__.py

mlir/utils/jupyter/mlir_opt_kernel/install.py

mlir/utils/jupyter/mlir_opt_kernel/kernel.py

mlir/utils/lldb-scripts/mlirDataFormatters.py

mlir/utils/mbr/mbr/__init__.py

mlir/utils/mbr/mbr/discovery.py

mlir/utils/mbr/mbr/main.py

mlir/utils/mbr/mbr/stats.py

mlir/utils/spirv/gen_spirv_dialect.py