You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "Hzfengsy (via GitHub)" <gi...@apache.org> on 2023/09/08 06:12:59 UTC
[GitHub] [tvm] Hzfengsy opened a new pull request, #15706: [TVMScript] Disable `black_format` by default
Hzfengsy opened a new pull request, #15706:
URL: https://github.com/apache/tvm/pull/15706
`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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on pull request #15706: [TVMScript] Disable `black_format` by default
Posted by "junrushao (via GitHub)" <gi...@apache.org>.
junrushao commented on PR #15706:
URL: https://github.com/apache/tvm/pull/15706#issuecomment-1711144061
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #15706: [TVMScript] Disable `black_format` by default
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #15706:
URL: https://github.com/apache/tvm/pull/15706#issuecomment-1712497455
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #15706: [TVMScript] Disable `black_format` by default
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #15706:
URL: https://github.com/apache/tvm/pull/15706#issuecomment-1721761751
https://github.com/apache/tvm/pull/15762
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on pull request #15706: [TVMScript] Disable `black_format` by default
Posted by "junrushao (via GitHub)" <gi...@apache.org>.
junrushao commented on PR #15706:
URL: https://github.com/apache/tvm/pull/15706#issuecomment-1721978241
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao merged pull request #15706: [TVMScript] Disable `black_format` by default
Posted by "junrushao (via GitHub)" <gi...@apache.org>.
junrushao merged PR #15706:
URL: https://github.com/apache/tvm/pull/15706
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #15706: [TVMScript] Disable `black_format` by default
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #15706:
URL: https://github.com/apache/tvm/pull/15706#issuecomment-1721410559
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.
```python
def show(..., black_format=None, ...):
if black_format is None:
black_format = bool(int(os.environ.get('TVM_BLACK_FORMAT','1')))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #15706: [TVMScript] Disable `black_format` by default
Posted by "Mousius (via GitHub)" <gi...@apache.org>.
Mousius commented on PR #15706:
URL: https://github.com/apache/tvm/pull/15706#issuecomment-1712526326
> 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](https://github.com/apache/tvm/blob/main/docs/contribute/code_review.rst#community-participation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on pull request #15706: [TVMScript] Disable `black_format` by default
Posted by "junrushao (via GitHub)" <gi...@apache.org>.
junrushao commented on PR #15706:
URL: https://github.com/apache/tvm/pull/15706#issuecomment-1712540425
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #15706: [TVMScript] Disable `black_format` by default
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #15706:
URL: https://github.com/apache/tvm/pull/15706#issuecomment-1721406739
@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. `((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)`
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org