[TVMScript] Support show_meta
#13934
Conversation
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment. Generated by tvm-bot |
junrushao
force-pushed
the
feature/2023-02-08/printer-misc-update
branch
from
026c3ef to
869bab2
Compare
This PR adds the functionality to roundtrip metadata during printing. Users may turn on the flag below to allow the printer to dump metadata to screen. ```python ir_node.show(show_meta=True, ...) ```
junrushao
force-pushed
the
feature/2023-02-08/printer-misc-update
branch
from
869bab2 to
3d02274
Compare
leandron
left a comment
leandron
left a comment
There was a problem hiding this comment.
A follow-up change is required, following what was discussed in https://discuss.tvm.apache.org/t/reminder-of-our-code-review-guidelines/14388
| def _relax_script(obj: Object, config: PrinterConfig) -> str: | ||
| func = get_global_func("script.printer.ReprPrintRelax") | ||
| return func(obj, config) | ||
|
|
||
|
|
There was a problem hiding this comment.
Hi @junrushao - it seems these functions accidentally were merged into main as opposed to unity - can you push a follow-up PR and change it to the appropriate branch?
It is one of the cases that a change was merged quite soon (in about 5h I think) so not many people were able to look into it.
There was a problem hiding this comment.
In this case, I don;t think there is any issue future proofing the particular implementation. Given that non of the existing works are disrupted.
Ultimately i would lean more towards the maintainer of the modules, given they volunteered their time and effort building the module in a scoped way. So unless this s disrupting some of the existing modules, i would suggest let the maintainers make the call
There was a problem hiding this comment.
Doing a bit more digging, this is actually technical debt/bug as script.printer.ReprPrintRelax is not something that exists in this code base, rather than any future proofing, so I'd like for this to be seriously considered for a fix.
Moreover, it is not really scoped as it is half here, and half in a separate branch.
There was a problem hiding this comment.
it is scoped in a sense that the name itself clearly differentiates itself from other functions and is confined within the printer module.
From a technical pov, it is common for code to have plugin registries that queries and runs function when available. Atm there does not seems to be a user facing regression triggered by the particular internal function, if there are I certainly trust the related maintainers for a fix. So i do not see it as a bug given my experience in this area.
Given this is an internal API here, i think the overall impact is scoped and minimum in this module (script printer) with clear name, folder structure etc, so again i will let the maintainers to make the call
Prior to this commit, the `tvm.testing.CompareBeforeAfter` utility performed string replacement to define the names of the before/after/expected examples. Since apache#13934 allowed the name to be explicitly passed to the printer, this should be used instead.
Prior to this commit, the `tvm.testing.CompareBeforeAfter` utility performed string replacement to define the names of the before/after/expected examples. Since #13934 allowed the name to be explicitly passed to the printer, this should be used instead.
5 participants
This PR adds the functionality to roundtrip metadata during printing. Users may turn on the flag below to allow the printer to dump metadata to screen.