-
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
[Doc][Relay] Add VM doc #3188
[Doc][Relay] Add VM doc #3188
Conversation
Will give this a review soon |
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.
In the "Instruction Set" section, some of the variables are put in the code block, but others are not. Please keep them consistent.
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.
I've left mostly grammar and style feedback. On a high level, I think this document does a good job of establishing the context for the Relay VM, but could stand to be more specific about what exactly the VM does to solve the problems that the interpreter and graph runtime have; I don't think that would require very large additions to the text.
Co-Authored-By: Steven S. Lyubomirsky <slyubomirsky@gmail.com> Co-Authored-By: 雾雨魔理沙 <lolisa@marisa.moe> Co-Authored-By: Logan Weber <36520469+weberlo@users.noreply.github.com> Co-Authored-By: Zhi <5145158+zhiics@users.noreply.github.com>
Thanks everyone! Addressed all comments. Please take another look. |
Co-Authored-By: 雾雨魔理沙 <lolisa@marisa.moe>
Co-Authored-By: Logan Weber <36520469+weberlo@users.noreply.github.com>
Co-Authored-By: Logan Weber <36520469+weberlo@users.noreply.github.com>
Co-Authored-By: Yong Wu <55wuyong@163.com>
ping reviewers(@jroesch @tqchen @zhiics @icemelon9 @MarisaKirisame @yongwww @joshpoll @weberlo @slyubomirsky @junrushao1994 @nhynes), please take another look at this PR. thanks! |
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.
One minor suggestion, but I think besides that suggestion (and besides the open TODO), this is a good overview
LGTM |
* [Doc][Relay] Add VM doc * Add Apache header * Apply suggestions from code review Co-Authored-By: Steven S. Lyubomirsky <slyubomirsky@gmail.com> Co-Authored-By: 雾雨魔理沙 <lolisa@marisa.moe> Co-Authored-By: Logan Weber <36520469+weberlo@users.noreply.github.com> Co-Authored-By: Zhi <5145158+zhiics@users.noreply.github.com> * Junru's comment * More fix * More fix * More fix * last fix * Apply suggestions from code review Co-Authored-By: 雾雨魔理沙 <lolisa@marisa.moe> * Apply suggestions from code review Co-Authored-By: Logan Weber <36520469+weberlo@users.noreply.github.com> * Add code links * Remove unused bp * Update docs/dev/virtual_machine.rst Co-Authored-By: Logan Weber <36520469+weberlo@users.noreply.github.com> * Explain TODO * Yong's comment Co-Authored-By: Yong Wu <55wuyong@163.com> * Comment
* [Doc][Relay] Add VM doc * Add Apache header * Apply suggestions from code review Co-Authored-By: Steven S. Lyubomirsky <slyubomirsky@gmail.com> Co-Authored-By: 雾雨魔理沙 <lolisa@marisa.moe> Co-Authored-By: Logan Weber <36520469+weberlo@users.noreply.github.com> Co-Authored-By: Zhi <5145158+zhiics@users.noreply.github.com> * Junru's comment * More fix * More fix * More fix * last fix * Apply suggestions from code review Co-Authored-By: 雾雨魔理沙 <lolisa@marisa.moe> * Apply suggestions from code review Co-Authored-By: Logan Weber <36520469+weberlo@users.noreply.github.com> * Add code links * Remove unused bp * Update docs/dev/virtual_machine.rst Co-Authored-By: Logan Weber <36520469+weberlo@users.noreply.github.com> * Explain TODO * Yong's comment Co-Authored-By: Yong Wu <55wuyong@163.com> * Comment
This doc is a merge of #2915 and #2810.
cc: @jroesch @tqchen @zhiics @icemelon9 @MarisaKirisame @yongwww @joshpoll @weberlo @slyubomirsky @junrushao1994 @nhynes