You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "Lunderberg (via GitHub)" <gi...@apache.org> on 2024/04/12 12:42:52 UTC
[PR] [TVMScript] Optionally use `ruff format` instead of `black` [tvm]
Lunderberg opened a new pull request, #16876:
URL: https://github.com/apache/tvm/pull/16876
The `ruff format` tool is significantly faster than the `black` formatter. For some particularly long TVMScript modules, using it can reduce the time required to show a formatted module from ~5 minutes to ~1 minute. This commit updates the `.show()` function to apply the optionally formatting using `ruff format` if available, falling back to `black` otherwise.
--
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
Re: [PR] [TVMScript] Optionally use `ruff format` instead of `black` [tvm]
Posted by "quic-sanirudh (via GitHub)" <gi...@apache.org>.
quic-sanirudh merged PR #16876:
URL: https://github.com/apache/tvm/pull/16876
--
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
Re: [PR] [TVMScript] Optionally use `ruff format` instead of `black` [tvm]
Posted by "quic-sanirudh (via GitHub)" <gi...@apache.org>.
quic-sanirudh commented on code in PR #16876:
URL: https://github.com/apache/tvm/pull/16876#discussion_r1566946710
##########
python/tvm/script/highlight.py:
##########
@@ -92,7 +95,73 @@ def cprint(
print(highlight(printable, Python3Lexer(), Terminal256Formatter(style=style)))
-def _format(code_str: str) -> str:
+@functools.lru_cache
+def _get_formatter(formatter: Optional[str] = None):
+ def get_ruff_formatter():
+ if shutil.which("ruff") is None:
+ return None
+
+ def formatter(code_str):
+ proc = subprocess.Popen(
+ ["ruff", "format", "--stdin-filename=TVMScript"],
+ stdin=subprocess.PIPE,
+ stdout=subprocess.PIPE,
+ encoding="utf-8",
+ )
+ stdout, _stderr = proc.communicate(code_str)
+ return stdout
+
+ return formatter
+
+ def get_black_formatter():
+ try:
+ # pylint: disable=import-outside-toplevel
+ import black
+ except ImportError:
+ return None
+
+ def formatter(code_str):
+ return black.format_str(code_str, mode=black.FileMode())
+
+ return formatter
+
+ def get_fallback_formatter():
+ def formatter(code_str):
+ with warnings.catch_warnings():
+ warnings.simplefilter("once", UserWarning)
+ ruff_install_cmd = sys.executable + " -m pip install ruff"
Review Comment:
Should we restrict to a specific version of ruff similar to a specific black version we've specified below?
--
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
Re: [PR] [TVMScript] Optionally use `ruff format` instead of `black` [tvm]
Posted by "quic-sanirudh (via GitHub)" <gi...@apache.org>.
quic-sanirudh commented on code in PR #16876:
URL: https://github.com/apache/tvm/pull/16876#discussion_r1567479484
##########
python/tvm/script/highlight.py:
##########
@@ -92,7 +95,73 @@ def cprint(
print(highlight(printable, Python3Lexer(), Terminal256Formatter(style=style)))
-def _format(code_str: str) -> str:
+@functools.lru_cache
+def _get_formatter(formatter: Optional[str] = None):
+ def get_ruff_formatter():
+ if shutil.which("ruff") is None:
+ return None
+
+ def formatter(code_str):
+ proc = subprocess.Popen(
+ ["ruff", "format", "--stdin-filename=TVMScript"],
+ stdin=subprocess.PIPE,
+ stdout=subprocess.PIPE,
+ encoding="utf-8",
+ )
+ stdout, _stderr = proc.communicate(code_str)
+ return stdout
+
+ return formatter
+
+ def get_black_formatter():
+ try:
+ # pylint: disable=import-outside-toplevel
+ import black
+ except ImportError:
+ return None
+
+ def formatter(code_str):
+ return black.format_str(code_str, mode=black.FileMode())
+
+ return formatter
+
+ def get_fallback_formatter():
+ def formatter(code_str):
+ with warnings.catch_warnings():
+ warnings.simplefilter("once", UserWarning)
+ ruff_install_cmd = sys.executable + " -m pip install ruff"
Review Comment:
Right yeah, that makes more sense. It is very hard to keep them in sync. Perhaps after we remove the version info from the warning message, we can just add a line saying something like **"For version compatibility with CI, please check docker/Dockerfile.ci_lint"** if the version info has to be specified.
Thanks for the response.
--
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
Re: [PR] [TVMScript] Optionally use `ruff format` instead of `black` [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on code in PR #16876:
URL: https://github.com/apache/tvm/pull/16876#discussion_r1567426092
##########
python/tvm/script/highlight.py:
##########
@@ -92,7 +95,73 @@ def cprint(
print(highlight(printable, Python3Lexer(), Terminal256Formatter(style=style)))
-def _format(code_str: str) -> str:
+@functools.lru_cache
+def _get_formatter(formatter: Optional[str] = None):
+ def get_ruff_formatter():
+ if shutil.which("ruff") is None:
+ return None
+
+ def formatter(code_str):
+ proc = subprocess.Popen(
+ ["ruff", "format", "--stdin-filename=TVMScript"],
+ stdin=subprocess.PIPE,
+ stdout=subprocess.PIPE,
+ encoding="utf-8",
+ )
+ stdout, _stderr = proc.communicate(code_str)
+ return stdout
+
+ return formatter
+
+ def get_black_formatter():
+ try:
+ # pylint: disable=import-outside-toplevel
+ import black
+ except ImportError:
+ return None
+
+ def formatter(code_str):
+ return black.format_str(code_str, mode=black.FileMode())
+
+ return formatter
+
+ def get_fallback_formatter():
+ def formatter(code_str):
+ with warnings.catch_warnings():
+ warnings.simplefilter("once", UserWarning)
+ ruff_install_cmd = sys.executable + " -m pip install ruff"
Review Comment:
I'm uncertain, but would lean instead toward removing the specific version of `black`, if we want consistency. Specifying a specific version of `black` is useful, since then it can be matched to the CI's version, in case a developer also uses `black` either for format-on-save or as a pre-commit hook. However, the version number in the warning has become out of sync with the CI's version (warning message was added in November 2022, but the CI was bumped from `22.3.0` to `22.12.0` in March 2023).
Since there isn't really a good way to keep them in sync (nor any `ruff` version in the CI with which to be in sync), I'd lean toward removing the versions from the warning message altogether.
--
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