search

tlc-pack / relax

Apache License 2.0
193 stars 58 forks source link

[Fix][Printer] Fix PrintFinal dispatch for Relax dedicated Expr #351

Closed MasterJH5574 closed 1 year ago

MasterJH5574 commented 1 year ago

PR #306 introduces dedicated Relax Exprs, making Relax sub-class Expr independent of Relay Expr. This PR is a fix on the text printer accordingly.

Prior to this PR, TextPrinter::PrintFinal only dispatch VarNode, FunctionNode and ShapeExprNode to Relax printer, leaving all other kinds of Exprs to Relay text printer. This is fine at the time before PR #306, when we were reusing Exprs from Relay. But since #306 this becomes an issue, because Relax Exprs are not properly dispatched to Relax printer. As a result, when we print a CallNode or TupleNode on Python side, or structural-equality check finds mismatch on Tuples, the PrettyPrint as well as PrintFinal cannot dispatch the Expr, and will result in error like

  Check failed: (can_dispatch(n)) is false: NodeFunctor calls un-registered function on type relax.expr.Tuple

which is not supposed and is also the least informative.

So this PR fixes the issue by adding our Relax Exprs to the dispatch rule. There are corresponding unit tests to make sure the printer issue is not likely to happen again.

cc @Hzfengsy @YuchenJin @tqchen

MasterJH5574 commented 1 year ago

[Upd: Done] Waiting for #349 to merge first and update the ConstantNode unit test.

tqchen commented 1 year ago

one note perhaps we need to move PrintFatal to dispatch to script printing later. (cc @junrushao)

MasterJH5574 commented 1 year ago

This one is now ready for review.

MasterJH5574 commented 1 year ago

Thanks @YuchenJin! Just addressed your comments with one more case of constants, and string comparison for BindingBlock and SeqExpr. Please take another look :-)

YuchenJin commented 1 year ago

Thanks @MasterJH5574 for patching the printing, it has bothered many of us for a while.