junrushao1994 commented on PR #12435: URL: https://github.com/apache/tvm/pull/12435#issuecomment-1214714154
@leandron Thanks for leaving the messages! Agreed that it seems a bit too rushy merging those PRs, and that's why I have stopped doing so and left the message saying "I don't want to merge too many PRs at once into mainline". To provide some context on those PRs: @cyx-6 is our summer intern, who implemented the parser over the 3 months. However, he was unable to upstream anything previously due to the fact that our RFC has been blocked for extensive discussion. At the time of submitting those PRs, it's been approaching the end of his internship, and the motivation for batch submission isn't for immediate merging, but instead, it's a fair demonstration of the concrete outcome of his work. We are completely aligned in terms of the requirement of PRs: - A1. PRs should be split into reviewable chunks; - A2. An API should be tested (as long as it is testable), if not already; - A3. Readable commit messages should be provided. A1: I believe we are already doing exemplar work dividing PRs into pieces each less than 100 lines. We did tons of planning to make sure the code is straightforward and easy to read. A2: We all agree that all the public APIs should be tested. For those PRs already submitted, we checked carefully and made there that there exists at least one unittest corresponding to each API exposed. For example, `T.assume` added in this PR corresponds to the test [here](https://github.com/apache/tvm/blob/bb513866ad70fa20eb0c37ca339d330d6a76c747/tests/python/unittest/test_tir_transform_remove_assume.py#L34). To provide further context, those APIs have already existed in the system for a while, and the only work done in those PRs is to expose them explicitly so that it's more friendly to python tooling (i.e. more discoverable by pylint). We will make sure a link is included to unittests that correspond to each API we exposed in further PRs. A3: I have been taking care of commit message drafting when merging those PRs, [here](https://github.com/apache/tvm/commit/bb513866ad70fa20eb0c37ca339d330d6a76c747) for example. I plan to send @cyx-6 with the latest commit messages I drafted so that he could update the messages himself. Again, thanks for bringing this up, and it's definitely a valid concern! We are very much aligned in building a healthy codebase and community. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
