Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[TVMScript] Use environment variable TVM_BLACK_FORMAT for .show() #15762

Merged
merged 2 commits into from
Sep 26, 2023
Merged

[TVMScript] Use environment variable TVM_BLACK_FORMAT for .show() #15762

merged 2 commits into from
Sep 26, 2023

Conversation

Lunderberg
Copy link
Contributor

Prior to this commit, the default behavior of the black_format argument in TVMScript printing has changed back and forth, based on conflicting user preferences. This commit allows the default to be specified by each using using the TVM_BLACK_FORMAT environment variable. If unspecified in a obj.show() method call, this environment variable is used to determine the default.

@Lunderberg Lunderberg mentioned this pull request Sep 15, 2023
Merged
@junrushao
Copy link
Member

Love this PR. Moving my comments from the previous one.

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!

@junrushao
Copy link
Member

BTW, line_length is something we could configure in black: https://github.com/psf/black/blob/19.3b0/black.py#L168

@Hzfengsy
Copy link
Member

Great PR! It's a great solution to make people with different tastes happy! Thanks @Lunderberg

@Lunderberg Lunderberg force-pushed the tvmscript_use_environment_variable_for_black_format_default branch from ecaca89 to 3f64cf5 Compare September 19, 2023 13:32
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Sep 21, 2023
@Lunderberg
[Hotfix] Disable test of quantized floor mod
8cc9c8d 
This unit test is failing on unrelated
PRs (e.g. apache#15762 at [CI
link](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm-arm/detail/PR-15762/4/tests)).
Local testing resulted in a failed test 44 times out of 100 iterations.
@Lunderberg Lunderberg mentioned this pull request Sep 21, 2023
Merged
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Sep 22, 2023
@Lunderberg
[Hotfix] Disable test of quantized floor mod
e343f1b 
This unit test is failing on unrelated
PRs (e.g. apache#15762 at [CI
link](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm-arm/detail/PR-15762/4/tests)).
Local testing resulted in a failed test 44 times out of 100 iterations.
junrushao pushed a commit that referenced this pull request Sep 23, 2023
@Lunderberg
[Hotfix][TFLite] Disable test of quantized floor mod (#15802)
ab3511a 
This unit test is failing on unrelated PRs (e.g. #15762 at [CI link](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm-arm/detail/PR-15762/4/tests)). Local testing resulted in a failed test 44 times out of 100 iterations.
@Lunderberg Lunderberg force-pushed the tvmscript_use_environment_variable_for_black_format_default branch from 2c38d10 to 4987c42 Compare September 23, 2023 13:13
@Lunderberg
[TVMScript] Use environment variable TVM_BLACK_FORMAT for .show()
965bc30 
Prior to this commit, the default behavior of the `black_format`
argument in TVMScript printing has changed back and forth, based on
conflicting user preferences.  This commit allows the default to be
specified by each using using the `TVM_BLACK_FORMAT` environment
variable.  If unspecified in a `obj.show()` method call, this
environment variable is used to determine the default.
@Lunderberg
Fix lint error
1f787c6
@Lunderberg Lunderberg force-pushed the tvmscript_use_environment_variable_for_black_format_default branch from 4987c42 to 1f787c6 Compare September 25, 2023 15:11
@Lunderberg Lunderberg merged commit d5fab9e into apache:main Sep 26, 2023
5 checks passed
@Lunderberg Lunderberg deleted the tvmscript_use_environment_variable_for_black_format_default branch September 26, 2023 20:06
vinx13 pushed a commit to vinx13/tvm that referenced this pull request Oct 17, 2023
@Lunderberg @vinx13
[TVMScript] Use environment variable TVM_BLACK_FORMAT for .show() (ap…
b9d3922 
…ache#15762)

Prior to this commit, the default behavior of the `black_format`
argument in TVMScript printing has changed back and forth, based on
conflicting user preferences.  This commit allows the default to be
specified by each using using the `TVM_BLACK_FORMAT` environment
variable.  If unspecified in a `obj.show()` method call, this
environment variable is used to determine the default.
@ysh329 ysh329 mentioned this pull request Oct 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hzfengsy Hzfengsy Hzfengsy approved these changes

@junrushao junrushao Awaiting requested review from junrushao

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Lunderberg @junrushao @Hzfengsy