[TVMScript] Disable black_format by default

[Hzfengsy]

Black is a popular code formatter for Python. However, it is not suitable for TVM script. This PR tries to disable it by default.

This would change the default printing behavior, love to hear opinions from the community.

cc @tqchen @Lunderberg @junrushao @cyx-6

[junrushao]

In most of the cases, formatting TVMScript results in a quite long python program spanning over a screen, so I suppose this is a better change by default.

[Lunderberg]

I've preferred having the auto-formatting of TVMScript, primarily to avoid having extremely long single lines (e.g. function declarations of end-to-end models), which would often be displayed incorrectly in paginators.

[Mousius]

I've preferred having the auto-formatting of TVMScript, primarily to avoid having extremely long single lines (e.g. function declarations of end-to-end models), which would often be displayed incorrectly in paginators.

Agree.

This would change the default printing behavior, love to hear opinions from the community.

It would've been good to allow more time for Community Participation.

[junrushao]

@Lunderberg it makes sense to have shorter lines in function signature, but my main worry is that it may lead to too fragmented line breakings like this: https://github.com/apache/tvm/blob/main/tests/python/unittest/test_tvmscript_roundtrip.py#L374, which happens a bit more frequently than long function signatures considering our arith analysis in most of the cases. Therefore, I usually turn off black_format when developing TIR schedules/passes.

I don’t have strong opinion on the default value of this flag though. Personally I always write “.show(black_format=False)” by default. It doesn’t bother me a lot, but considering there seem more cases it’s better to be turned off in the test file I shared, it might be better to do so by default.

[Lunderberg]

@junrushao True, it does tend to depend on what type of IR somebody tends to look at. Relax IR tends to be very flat, due to the restricted form of the DataflowBlock. The TIR examples tend to be more nested, especially with many loops over high-dimensionality buffers.

That said, from the example linked there are a number of improvements we could make that would make the TVMScript more readable, with the ease of black-auto-formatting coming along as a side effect.

• Avoid extraneous parentheses. Currently, TVMScript gives explicit parentheses for operations, even when the parentheses are unnecessary. Edit: The example has extra parentheses that TVMScript now avoids. ((x_outer * 32768) + (x_c * 1024)) + (k_outer * 4) could instead be x_outer * 32768 + x_c * 1024 + k_outer * 4
• The T.grid syntax is only used for serial loops. If it had an additional keyword-only argument for the loop types, other nested for x in T.parallel(0, 32): for y in T.serial(0, 1024): could instead be written as for x,y in T.grid(32, 1024, type='PS').
• T.ramp and T.broadcast can be printed as python slices into a buffer, rather than explicitly written. This lets C_global[T.ramp((x_c_init * 32), 1, 32)] = T.broadcast(T.float32(0), 32) be written as C_global[x_c_init*32: x_c_init*32 + 32] = T.float32(0) Edit: The current TVMScript printer handles this case.

There are also lowering passes we could implement that would improve performance, with improved TVMScript readability as a side-effect.

• Automatic loop fusion. If two nested loops have the same type, that type is not kVectorized, and the loop iterators are always used as the expression iter_outer*extent_inner + iter_inner, then the loops can be fused into a single loop.

Edit: It looks like a large number of the changes I was brainstorming on are already implemented, but are not included in all TVMScript examples in the unit tests. For curiosity, I re-printed the linked example, both with and without black-auto-formatting (link), and the improved printing makes the TVMScript more readable, and more amenable to auto-formatting.

[Lunderberg]

That said, those are each longer plans. An easier solution for the moment may be to have an environment variable serving as the default, rather than a single default for all developers.

def show(..., black_format=None, ...):
    if black_format is None:
        black_format = bool(int(os.environ.get('TVM_BLACK_FORMAT','1')))

[Lunderberg]

I submitted #15762, which determines the default from an environment variable.

[junrushao]

Thanks @Lunderberg! 100% agreed with your comments, and particularly, feeling the same as you that the previous generation of TVMScript printer comes with many subtle issues making it not good looking or non-pythonic at all, as you mentioned, extra parentheses, non-fused loops, and ramp/broadcast syntax. The environment variable based approach is even better to me. Happy to get it in!

I have been thinking about this issue as well in the recent days, as it's something we all care about a lot. I think all of us agree with the formatting perspective, and my personal source of pain is mainly about the line width. How about @Hzfengsy? If line-width is the only thing we have different opinions, we can make it configurable using an environment variable.

Let's also move the discussion to @Lunderberg's new PR for more visbility!

Thanks again Eric for being always super thoughtful!