-
Notifications
You must be signed in to change notification settings - Fork 81
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
[RFC] TUNIP: TVMScript Unified Printer #74
Conversation
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.
Thank you for the great RFC!
Just a few questions.
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.
LGTM! The unified representation across multiple dialects and better error reporting will greatly help the development of Relax and future IRs.
Thank you @yelite and @junrushao1994 for the great design and RFC!
very sorry for the delay here. @junrushao1994 i guess i'm still a little fuzzy on the rationale for why we want to have two ways to parse TVMScript. I know this is a bit bigger than just this RFC, but with this RFC we're further pushing the cart down this path so I'd like us to be explicit about why. Relay has a single roundtrippable serialization format, as do most languages. I think we benefit from this in that we only have one set of tests to maintain. With TIR, my understanding is that we used Python as an AST parser since language constructs in TIR somewhat closely matched to Python. I'm not sure I see why we should want to have two different ways to do this--shouldn't we just have a single way to serialize TIR and parse it that always works? It seems like we could still expose this parser to various frontends via PackedFunc. |
To clarify, Relay has two roundtrippable serialization formats: text format and json. people use text format for readability, and in most usecases, go for json reliable serialization.
First of all, the RFC focuses on the printer, so would be great if we talk about the parser in the incoming RFC, but to clarify, we are not having two ways to parse TVMScript in the same language. Instead, for metaprogramming capability, one may need to interleave host language-based IRBuilder with the parser.
To clarify, TIR is a DSL independent of any existing language. It's somewhat close to HalideIR but has been evolving for years with lots of new features, for example, blocks. Python syntax is an add-on for usability but TIR design is in no ways matched to python. Anyways, I'm happy to talk more on the upcoming metaprogramming RFC. |
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.
LGTM. Thanks @yelite !!!
what do you mean by "reliable" here? if they're truly roundtrippable, aren't they both reliable? just trying to understand :)
Is the goal then that you'd be able to serialize an IRModule which contains fragments that need to be repeated via meta-programming? then, if you did this using a host language's IRBuilder, you could quickly write some meta-programming steps in that host language to expand the program.
I agree with your statement, but I thought the repr form of TIR wasn't so easy to work with and thus the motivation for TVMScript. While I agree that TIR is technically separate, if TVMScript is a core way in which TIR is used, I'd argue we should treat them conceptually as joined (e.g. TVMScript as the recommended roundtrip text format for TIR). What are your thoughts there? |
Thanks @areusch for your response!
Let's phrase it this way: TVMScript serves as a frontend of easily constructing and manipulating TIR, Relax and any third-party IRs. It offers readability, type checking, metaprogramming, etc. In terms of serialization, my standing is that reflection-based JSON serialization is still the best way to go as it's the least error-prone if we don't need readability and manipulability.
In practice we did notice that there could be some ad-hoc issues (which are definitely fixable, not a big deal, so I'm not saying it's not good) that probably be overlooked when implementing the parser. For example, @mbs-octoml fixed a dtype issue very recently (apache/tvm#11224). Instead, reflection-based JSON serialization mechanism in TVM is definitely less bug-prone because it doesn't need to handle edge cases - the downside is that it's almost unreadable though.
Yeah with metaprogramming we are able to write bunch of code that require host-language control. The next RFC is all about metaprogramming so stay tuned :-) |
To summarize offline discussion with @areusch: Q: Is this going to unify current fragmented printing formats? Q: Is JSON serialization going to be deprecated? Q: Is this going to introduce new functionalities that make TVM more fragmented? Q: Why is this called "unified" printer?
TIR developers only need to maintain the |
thanks for summarizing @junrushao1994 . to provide some additional perspective:
|
Hey @areusch thanks for elaborating your points and these are all definitely great questions to me!
Oh I think I get your concern here!! No, we are not building more and more and more frontends to cause fragmentation. Here is the thing - we only need two forms for serialization:
Imagine you are a rust user who doesn't want to use python, our proposal makes it possible to develop a frontend in pure rust. And of course, it's not going to be our priority, i'm just stating the possibility.
Ah no, IRBuilder is not serialization, sorry for the confusion. Let's imagine a common usecases in company's prod env where python is strictly prohibited (e.g. on-device training), but our developers wrote most of the operators in python, which is the reality. How do we quickly migrate those operators to C++? Our RFC provides an answer to that - just write a codegen from Doc to C++ IRBuilder :-) Of course again, the RFC aims to open possibilities instead of implementing those functionalities.
Yeah it's definitely fair to say the PR considers Relax a lot. On the other hand, Relax doesn't need this RFC to use TVMScript. Take the current codebase as an example, it's not using the design of this proposal and it's totally fine given it's assuming there are only 2 IRs. Instead, I'm trying to convey is that we build an open infra for any vendors to integrate with their own IRs, which can be represented and compiled in the same IRModule with Relax, TIR, etc. |
@junrushao1994 ah thanks for clarifying! a few follow-ups then:
Does this mean you can import the JSON-based serialization using only libtvm.so (e.g. rust could import JSON)? If so, then, in the hypothetical prod scenario:
, what's the motivation for someone to use IRBuilder instead of just serializing the TVMScript to JSON via parse/print and then ingesting the JSON in their prod env using the JSON parser?
That makes sense, thanks for clarifying :) |
@areusch Thanks for following up!
JSON is mostly for serializing an IR after it's constructed (which users cannot manipulate), and the TVMScript format is for users to construct the IR themselves. For example, to implement a ReLU operator, TVMScript users could write a python/rust code, while it's pretty infeasible for them to write JSON with bare hand. |
Hey I'm happy to discuss more, and let's keep this RFC open until the end of this week |
@junrushao1994 ok, just want to clarify one question here:
i have heard some folks in the community interested in converting TIR to JSON (I think @SebastianBoblestETAS and @MichaelJKlaiber if memory serves), but I also think that the use cases they're interested in could be accomplished if we had a user-friendly, robust Python interface to the TIR. |
Thanks @areusch for pointing me to the thread! Definitely happy to read the discussion, and glad to see that @vinx13 unblocks the
Yeah JSON doesn't provide any extra functionality, but it's pretty stable and reliable protobuf-like serialization, which could be faster to parse/dump if we only want to save the IRs or send them around between processes without the need of readability and manipulatability. For general users, our suggestion should be like, just use TVMScript to write TIR and forget about JSON. For advanced developers, for example, we want to store TIR in tuning database, cache TIR somewhere for later usage, JSON is definitely the "to-go" approach. |
A good analogy is the ProtoBuf text format (unfortunately no public spec outside google) vs. wire format (binary format). People write proto in text format when they need to create proto manually, while the binary format is used in RPC payload for its portability and efficiency. Back to our own use case, TVMScript is the choice if user want to construct IR graph manually, but it's not portable because the TVMScript parser has a strong dependency on Python, especially after the Metaprogramming RFC (#79). On contrast, the JSON format will be portable because it only depends on the IR node definition in C++. |
I'm merging this RFC as it seems that our discussion has reached consensus, but feel free to follow-up any time! |
Rendered