You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "YuchenJin (via GitHub)" <gi...@apache.org> on 2023/02/07 01:27:13 UTC
[GitHub] [tvm] YuchenJin opened a new pull request, #13926: [Unity] Relax BlockBuilder and ExprMutator
YuchenJin opened a new pull request, #13926:
URL: https://github.com/apache/tvm/pull/13926
This PR adds BlockBuilder: the core data structure to construct Relax AST, and ExprMutator: performs AST mutation for implementing transformation passes.
Co-Authored-by: Tianqi Chen <ti...@gmail.com>
Co-Authored-by: Altan Haan <al...@cs.washington.edu>
Co-Authored-by: Andrew Liu <an...@gmail.com>
Co-Authored-by: Hongyi Jin <32...@qq.com>
Co-Authored-by: Jiawei Liu <ja...@gmail.com>
Co-Authored-by: Junru Shao <ju...@gmail.com>
Co-Authored-by: Lesheng Jin <34...@users.noreply.github.com>
Co-Authored-by: masahi <ma...@gmail.com>
Co-Authored-by: Prakalp Srivastava <pr...@octoml.ai>
Co-Authored-by: Ruihang Lai <ru...@cs.cmu.edu>
Co-Authored-by: Siyuan Feng <Hz...@sjtu.edu.cn>
Co-Authored-by: Steven S. <Lyubomirsky slyubomirsky@octoml.ai>
Co-Authored-by: Sunghyun Park <49...@users.noreply.github.com>
Co-Authored-by: Yixin Dong <ub...@gmail.com>
Co-Authored-by: Yong Wu <yo...@gmail.com>
Co-Authored-by: Ziheng Jiang <zi...@apache.org>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on pull request #13926: [Unity] Relax BlockBuilder and ExprMutator
Posted by "gromero (via GitHub)" <gi...@apache.org>.
gromero commented on PR #13926:
URL: https://github.com/apache/tvm/pull/13926#issuecomment-1420631047
@junrushao Is the idea that these PRs (like this one) being merged into `unity` branch will get at some point in the future be merged into `main` branch as-is?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao merged pull request #13926: [Unity] Relax BlockBuilder and ExprMutator
Posted by "junrushao (via GitHub)" <gi...@apache.org>.
junrushao merged PR #13926:
URL: https://github.com/apache/tvm/pull/13926
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tvm-bot commented on pull request #13926: [Unity] Relax BlockBuilder and ExprMutator
Posted by "tvm-bot (via GitHub)" <gi...@apache.org>.
tvm-bot commented on PR #13926:
URL: https://github.com/apache/tvm/pull/13926#issuecomment-1420036281
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* No users to tag found in teams: `unity` <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #13926: [Unity] Relax BlockBuilder and ExprMutator
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #13926:
URL: https://github.com/apache/tvm/pull/13926#issuecomment-1420744226
We have not yet decided on how `unity` will get merged into main. Likely the community will need to look at the unity branch as a whole to get enough context for merging into the main. It is unlikely to happen at a single point in time(or even a matter of days), or in the context of one commit. We need a long, extended period of time for branch developers to take feedback and provide examples, code tutorials, walkthroughs, and demos to get buy-in from the community. Branch development offers that flexibility and an extended period of time.
Coming back to the branch establishment stage. It is indeed hard for community members who are not yet in the related code areas to be able to digest and provide feedback under the context of a single commit in the scaffolding commits. By design, scaffolding commits need a broader context that will appear in the future when connecting things together and are subject to future iterations(and rewrites in needed cases such as rebasing to main).
Nevertheless, some initial agility around scaffolding commits is helpful to establish the branch and unblock developments. In the meantime, we would like to experiment with different ways to enable agility with quality. For example, encouraging community members to continue giving feedback after the merging points and requiring the merger to be responsible for reacting to these comments after the merge.
Additionally, there are more valuable ways to get the community to give feedback and continuously bring iteration suggestions. We can also use forms of interaction, such as discussion posts, demos, code example walkthroughs, community meetups to get us that context through extended period of time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #13926: [Unity] Relax BlockBuilder and ExprMutator
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #13926:
URL: https://github.com/apache/tvm/pull/13926#issuecomment-1420987354
First of all, it is important indeed important to group things into reasonably isolated components that have reasonable structure.
That does not mean, however, that such logical structures do not require a broader context to be able to understand and iterate. For example, knowing IR nodes themselves is indeed a valuable point of assessment. But to fully access it, we will need to look at the context of how an IR node interacts with parser, printer, and passes. These are contexts that would be helpful to obtain over an extended course of time.
Coming back to the state of branch establishment. There are generally two different ways of establishing the module.
- W0: Start with an initial rough subset of implementations, then iterate to expand the feature
- As time goes by, these modules have features that get expanded into a broader set. The scaffolding commits are still going to be large. Then small commits are built on top.
- W1: Have a reasonable subset component that gets planned with a decent set of features, documents, and tests. Such scaffolding PRs can indeed contain more lines of code (and requires levels of expertise of the overall architecture) mainly because that represents a squashed view of part of history in W0.
As someone who has ramped up many OSS projects and modules, I would like to say that such a period is not a normal steady state but sometimes can be necessary for the ramping period both in W0 and W1 for us to flexibly bring in pieces. Right now, given our current understanding of unity, we are in a W1-style branch establishment stage.
Coming back to the technical assessment of this PR. Objectively, I think it is sufficiently isolated in terms of a logical component, `BlockBuilder` which comes with struct info deduction for the full IR. And related minimum operator and te-emission support so to enable a good amount of test coverage. It is indeed long in terms of lines of code, but from an architectural pov clear in terms of how things are isolated.
If there is an actionable suggestion on further isolating things, this could be great feedback, and we could say rewrite the history of the branch to introduce such a split. The same goes for, say, a suggestion on commit messages. These follow-up comments do have a lot of meanings and are valuable. As again, we can view them as review suggestions to the branch.
Similarly, with some additional context through an extended period of time, we help community members gain a deeper understanding to give more concrete, actionable suggestions. These feedbacks are still valuable and sometimes even more informative throughout development.
The flexibility of branch development offers us a way to react to those comments, and we welcome them wholeheartedly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #13926: [Unity] Relax BlockBuilder and ExprMutator
Posted by "Mousius (via GitHub)" <gi...@apache.org>.
Mousius commented on PR #13926:
URL: https://github.com/apache/tvm/pull/13926#issuecomment-1420894510
> Coming back to the branch establishment stage. It is indeed hard for community members who are not yet in the related code areas to be able to digest under the context of a single scaffolding commit. By design, a scaffolding commit connect to a broader context that will appear in the future(such as how IR node is being used, printed and parsed exactly) when connecting things together and are subject to future iterations(and rewrites in needed cases such as rebasing to main).
If we cannot review changes in isolation, that indicates the changes are not well broken down and delivered for review. There are foundational issues with this PR, such as being over 8,000 lines, a vague commit message and being merged in 5 hours - all of which work to exclude reviewers who may not see the PR or feel their reviews have no meaning once the PR gets merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org