-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
print ast w/o metadata #2533
print ast w/o metadata #2533
Conversation
include/tvm/relay/expr.h
Outdated
* additional comment block to an expr. | ||
* \return The text representation. | ||
*/ | ||
std::string Print(const NodeRef& node, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us simply overload operator<< for relayExpr in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tqchen Yeah, I tried overloading before, but there is already an overloaded one here: https://github.com/dmlc/tvm/blob/master/src/lang/expr.cc#L46 So I simply used Print.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tqchen Sorry, you were saying Expr. That should work. I will update the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we want to have default be the pretty-printed version of the program? It is valuable to the debug representation as well, how do we do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jroesch I agree that metadata can be useful for debugging, but I think pretty-printing could probably provide enough information most of the time if I understand correctly. People can still have the detailed information using the previous way for debugging if needed.
@tqchen @ZihengJiang thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you feel that there is a need to avoid c++ printing for now. we can first remove the operator<< in C++ and only land the python side of changes
@zhiics can you remove the << in C++ for now and let us first merge in the python side, which is the mostly convenient part? |
@tqchen Removed. Please take another look. Thanks. |
@zhiics need rebase |
@tqchen rebased. |
As described in #2461 , we want to have a separate
Print
function in C++ and overloaded__str__
in python to omit metadata when print relay nodes.CC' @tqchen, @ZihengJiang, @jroesch, @yzhliu please review. Thanks.