You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/07/27 04:09:17 UTC
[GitHub] [tvm] ganler opened a new pull request, #12197: [UX] highlight tvm script
ganler opened a new pull request, #12197:
URL: https://github.com/apache/tvm/pull/12197
See https://github.com/tlc-pack/relax/pull/185
cc: @YuchenJin @junrushao1994 @Hzfengsy
--
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] junrushao1994 merged pull request #12197: [UX] highlight tvm script
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged PR #12197:
URL: https://github.com/apache/tvm/pull/12197
--
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] ganler commented on pull request #12197: [UX] highlight tvm script
Posted by GitBox <gi...@apache.org>.
ganler commented on PR #12197:
URL: https://github.com/apache/tvm/pull/12197#issuecomment-1197293248
@tkonolige Thanks for the suggestions.
To put some background: The reason why I use these magic RGB values is to match exact themes in jupyter notebook (or vscode) so they will look _**compatible**._ In jupyter notebook, the colors will be displayed as what the RGB values exactly are since it uses HTML to render things. While for terminals, I guess it will map things to a nearest 3/4/8-bit colors.
That said I fully agree that we want the theme look **_compatible_** for both notebook users and terminal users. My proposal is to: 1) first set them as different theme styles (including the theme you suggested that we have 3 tvm-builtin themes where 2 for jupyter notebook environemnts and 1 for terminal env); 2) we set the default style to a magic string, say "auto", and then we detect if the user is doing experiements in a notebook (if notebook -> "light"; if terminal -> ansi colors).
--
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] areusch commented on pull request #12197: [UX] highlight tvm script
Posted by GitBox <gi...@apache.org>.
areusch commented on PR #12197:
URL: https://github.com/apache/tvm/pull/12197#issuecomment-1197315605
@ganler thanks for this PR! would be great to add highlighting to TVMScript.
on the python deps: unfortunately we're still working through a PR to generate a virtualenv from gen_requirements.txt, so you'll also need to add it to `docker/install/ubuntu_install_python_package.sh` for now.
regarding the actual PR, some thoughts: in general it'd be good to avoid adding dependencies to the TVM core unless they're clearly beneficial to a large set of code paths. the approach here seems to be to lex the stringified TVMScript and then highlight based on tokens. I wonder if it would be possible to instead modify the TVMScript printer? The benefit of doing this is that then, you could automatically determine whether you wanted to highlight TVMScript by checking whether it was being `operator<<` to a TTY. otherwise, i wonder if it might be better to put this in contrib (and not add pygments to the core tvm dep list) or create a `setup.py` "extra" to distinguish between dependencies needed just to run inference and dependencies needed for work on the compiler.
--
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] areusch commented on pull request #12197: [UX] highlight tvm script
Posted by GitBox <gi...@apache.org>.
areusch commented on PR #12197:
URL: https://github.com/apache/tvm/pull/12197#issuecomment-1197315915
cc @yelite @junrushao1994 who might have ideas whether the printer should handle this
--
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] tkonolige commented on a diff in pull request #12197: [UX] highlight tvm script
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #12197:
URL: https://github.com/apache/tvm/pull/12197#discussion_r931514245
##########
python/gen_requirements.py:
##########
@@ -64,6 +64,7 @@
(
"Base requirements needed to install tvm",
[
+ "Pygments",
Review Comment:
You need to add a version constraint for Pygments in the `CONSTRAINTS` array lower in this file.
--
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] tkonolige commented on pull request #12197: [UX] highlight tvm script
Posted by GitBox <gi...@apache.org>.
tkonolige commented on PR #12197:
URL: https://github.com/apache/tvm/pull/12197#issuecomment-1197276646
Would it be possible to use ansi colors instead of hardcoding specific rgb values. That way the printed code would match the user's terminal colors?
Here is with rgb colors:
<img width="863" alt="Screen Shot 2022-07-27 at 12 27 57 PM" src="https://user-images.githubusercontent.com/1501680/181356103-d21c4f99-5783-4e84-bb98-0511ecc3274b.png">
And here is with ansi colors. Notice how it matches my prompt and other text in my terminal.
<img width="858" alt="Screen Shot 2022-07-27 at 12 27 51 PM" src="https://user-images.githubusercontent.com/1501680/181356107-d8a65256-e71b-44f5-88d4-d46eb64c8298.png">
This is the theme I used:
```
Keyword: "bold ansimagenta",
Keyword.Namespace: "ansicyan",
Keyword.Type: "ansibrightblue",
Name.Function: "bold ansiyellow",
Name.Class: "bold ansiblue",
Name.Decorator: "italic ansibrightmagenta",
String: "ansibrightyellow",
Number: "ansibrightgreen",
Operator: "ansired",
Operator.Word: "ansibrightcyan",
Comment: "italic ansigray",
```
--
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] yelite commented on pull request #12197: [UX] highlight tvm script
Posted by GitBox <gi...@apache.org>.
yelite commented on PR #12197:
URL: https://github.com/apache/tvm/pull/12197#issuecomment-1197379128
I think it's reasonable to let 3rd party library to handle the highlighting. It will take non-trivial effort to implement highlight in the TVMScript printer by ourselves. This also applies to other nice-to-have features like code formatting. Users might want the printed TVMScript to be free from very long lines and we can use black to format the output.
IMO all those extra dependencies should be optional and the `show` function should just fallback to plain printing if those dependencies are not installed and then emit a warning message.
--
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] ganler commented on pull request #12197: [UX] highlight tvm script
Posted by GitBox <gi...@apache.org>.
ganler commented on PR #12197:
URL: https://github.com/apache/tvm/pull/12197#issuecomment-1197481985
Some updates:
**Fallback as plain text and warn** thanks to @yidawang and @areusch
If Pygments is not installed, we print plain text and post a warning:
<img width="1014" alt="image" src="https://user-images.githubusercontent.com/38074777/181385896-e0250db1-0304-40ce-87b3-1160242ef8df.png">
**ANSI colors for terminal users** thanks to @tkonolige
If the environment is not notebook (i.e., terminal or something), we use a style filled with ansi colors (the colors are tuned to close to the default theme of notebook).
Dark (iterm2):
<img width="1265" alt="Screen Shot 2022-07-27 at 6 21 31 PM" src="https://user-images.githubusercontent.com/38074777/181389041-fbb24197-cd38-458c-ae7e-576e870d3609.png">
Light (macOS terminal):
<img width="1124" alt="Screen Shot 2022-07-27 at 6 21 59 PM" src="https://user-images.githubusercontent.com/38074777/181389099-61eaff67-37ec-46d4-b216-51d023a9a5b5.png">
--
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] ganler commented on pull request #12197: [UX] highlight tvm script
Posted by GitBox <gi...@apache.org>.
ganler commented on PR #12197:
URL: https://github.com/apache/tvm/pull/12197#issuecomment-1197338540
Thanks @areusch for the suggestions!
I agree that we want to avoid dependencies if possible. As you said, one way is to implement the functionality from scratch.
> I wonder if it would be possible to instead modify the TVMScript printer?
It is possible with some effort to build a highlighting functionality from scratch: 1) lexer; 2) highlighting terminals by generating ansi escape code; and 3) make things configurable.
> in general it'd be good to avoid adding dependencies to the TVM core unless they're clearly beneficial to a large set of code paths.
Agreed. I think we should not put this under "core". Meanwhile, "Pygments" is actually a very fundamental and reliable library in Python community that has been actively developed for years. For example, latest pypi can use `rich` (which depends on "Pygments") to display the progress bars.
Summing up the bits above, I suggest that we still use Pygments to hightlight TVM script but we don't put it as a required dependency. Instead we can put some log to suggest users to install "Pygments" if they have not done it so far or simply put it in `extras_require`.
--
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