[TVMScript] Doc Base Class & DocPrinter Scaffolding #11971
Conversation
junrushao
changed the title
| std::unique_ptr<DocPrinter> GetPythonDocPrinter(const DocPrinterOptions& options) { | ||
| return std::make_unique<PythonDocPrinter>(options); | ||
| } | ||
|
|
||
| TVM_REGISTER_GLOBAL("script.printer.PrintDocAsPython") |
There was a problem hiding this comment.
Let's think about API consistency between python and c++: if a public API exists in c++, then it should exist also in python, and vice versa. This is a strict standard in non-legacy TVM codebase (e.g. TIR scheduling, MetaSchedule)
In our particular case, I would propose:
- renaming from the
PrintDocAsPython, useDocToScriptPythonas the public C++ / python API GetPythonDocPrinteris Relatively not super useful (only used by the entry function of TVMScript printer) and there is no actual need to expose it in any header file (correct me if I was wrong)
There was a problem hiding this comment.
These suggestions sound good to me. I made the following change:
- Expose a
DocToPythonScriptfunction in the public C++/Python API (I swapped 'Python' and 'Script' to make this sound more natural IMO. Let me know if you want to use the original name you suggested in the comment) - Hide the base DocPrinter to a private header (as is in the previous version of code, just state here for clarity)
- Remove
GetPythonDocPrinter
| (LiteralDoc('""'), r'"\"\""'), | ||
| (LiteralDoc("\n\t\\test\r"), r'"\n\t\\test\r"'), | ||
| # TODO: make the roundatrippable problem caused by utf8 | ||
| pytest.param(LiteralDoc("\x88"), r'"\x88"', marks=pytest.mark.xfail), |
There was a problem hiding this comment.
quick question: why are these marked as xfail?
There was a problem hiding this comment.
There were three xfail tests here. I removed the latter two about float number per our discussion yesterday (we want to handle float number precise printing in the IRDocsifier).
This one (on line 45) is about the utf8 encoding. LiteralDoc("\x88") will print '\xc2\x88' (the utf8 encoded bytes for "\x88"). This is not roundtrippable.
To fix this, we need to either:
- Add utf8 decoding to StrEscape
- Output in the form of
b'\xC2\x88'.encode()'
I don't think unicode support is important to our use case and it doesn't help us reach feature parity with the old printer at all. So I don't want to solve it for now, but just leave an xfail test as a note
There was a problem hiding this comment.
Thanks for the detailed explanation!
This PR addes: - Doc base class - DocPrinter base class - PythonDocPrinter - LiteralDoc and its support in DocPrinter Tracking issue: apache#11912
This PR addes: - Doc base class - DocPrinter base class - PythonDocPrinter - LiteralDoc and its support in DocPrinter Tracking issue: apache#11912
This PR addes: - Doc base class - DocPrinter base class - PythonDocPrinter - LiteralDoc and its support in DocPrinter Tracking issue: apache#11912
This PR addes:
Tracking issue: #11912
cc @junrushao1994 @gbonik