You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/08/15 01:42:55 UTC

[GitHub] [tvm] cyx-6 opened a new pull request, #12435: [TIR] Expose misc TIR `op`s to python

cyx-6 opened a new pull request, #12435:
URL: https://github.com/apache/tvm/pull/12435

   `op` list:
   `assume`
   `undef`
   `likely`
   Used and tested for future TVMScript parser


-- 
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] leandron commented on pull request #12435: [TIR] Expose Misc TIR operations to python

Posted by GitBox <gi...@apache.org>.

leandron commented on PR #12435:
URL: https://github.com/apache/tvm/pull/12435#issuecomment-1214712645

> 1. These operations are widely uesd in current unittest sets with `T.*` currently. And the current parser directly generates `tir::Call` for these operations, refer to [here](https://github.com/apache/tvm/blob/bb513866ad70fa20eb0c37ca339d330d6a76c747/python/tvm/script/parser.py#L934). However, in the new parser, we will call these operations in `op.py`, as we have just added. The current unittest set will automatically be tests for these newly expoesd `op`s. And these newly added `op`s have passed CI with our new parser as well, which proves their correctness. So we do not add extra unittests this time. Of course, in general cases, we do have to add related unittests for these `op`s.

Please correct me if what I say is wrong, but, my assumption is that all these PRs are making TIR operations that before were accessible in C++, to be available in Python. If so, there should be at least one unittest that make that call in Python, and check whether the expected API call is made.

I understand other test layers for the underneath APIs are already tested for correctness - that's out of scope for this particular PR, but it really would make sense to have a test for the API being exposed here, in Python.

Specifically, we should make sure that basic things are in place, such as the parameters sent are aligned with what the API expects, but more importantly, if the API changes, at least this simple test will break. So I think having these tests are fundamental for us to merge these PRs.

> 2. Sure. I will update these commit messages and pay attention to it next time.

Thanks.

--
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] junrushao1994 commented on pull request #12435: [TIR] Expose Misc TIR operations to python

Posted by GitBox <gi...@apache.org>.

junrushao1994 commented on PR #12435:
URL: https://github.com/apache/tvm/pull/12435#issuecomment-1214751667

   @leandron We updated the PR to make sure it's crystal clear that we hold up to the highest standard in A1, A2 and A3. Your timely review is much appreciated!


-- 
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] junrushao1994 commented on pull request #12435: [TIR] Expose Misc TIR operations to python

Posted by GitBox <gi...@apache.org>.

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: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] junrushao1994 closed pull request #12435: [TIR] Expose Misc TIR operations to python

Posted by GitBox <gi...@apache.org>.

junrushao1994 closed pull request #12435: [TIR] Expose Misc TIR operations to python
URL: https://github.com/apache/tvm/pull/12435


-- 
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] cyx-6 commented on pull request #12435: [TIR] Expose Misc TIR operations to python

Posted by GitBox <gi...@apache.org>.

cyx-6 commented on PR #12435:
URL: https://github.com/apache/tvm/pull/12435#issuecomment-1214689340

   @leandron Thanks for commenting!
   
   1.  These operations are widely uesd in current unittest sets with `T.*` currently. And the current parser directly generates `tir::Call` for these operations, refer to [here](https://github.com/apache/tvm/blob/bb513866ad70fa20eb0c37ca339d330d6a76c747/python/tvm/script/parser.py#L934). However, in the new parser, we will call these operations in `op.py`, as we have just added. The current unittest set will automatically be tests for these newly expoesd `op`s. And these newly added `op`s have passed CI with our new parser as well, which proves their correctness. So we do not add extra unittests this time. Of course, in general cases, we do have to add related unittests for these `op`s.
   2. Sure. I will update these commit messages and pay attention to it next 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] junrushao1994 commented on pull request #12435: [TIR] Expose Misc TIR operations to python

Posted by GitBox <gi...@apache.org>.

junrushao1994 commented on PR #12435:
URL: https://github.com/apache/tvm/pull/12435#issuecomment-1216258587

   Thanks @leandron for working together with us on this PR!


-- 
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] junrushao1994 commented on pull request #12435: [TIR] Expose Misc TIR operations to python

Posted by GitBox <gi...@apache.org>.

junrushao1994 commented on PR #12435:
URL: https://github.com/apache/tvm/pull/12435#issuecomment-1214720612

   See our discussion thread [here](https://github.com/apache/tvm/pull/12435). Let's do one PR at a 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] junrushao1994 commented on pull request #12435: [TIR] Expose Misc TIR operations to python

Posted by GitBox <gi...@apache.org>.

junrushao1994 commented on PR #12435:
URL: https://github.com/apache/tvm/pull/12435#issuecomment-1214994146

   @cyx-6 some possible extra tests we could add (per discussion with @leandron): assemble a TIR call node using the API exposed and assert its op’s name


-- 
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] junrushao1994 merged pull request #12435: [TIR] Expose Misc TIR operations to python

Posted by GitBox <gi...@apache.org>.

junrushao1994 merged PR #12435:
URL: https://github.com/apache/tvm/pull/12435


-- 
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