You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "apeskov (via GitHub)" <gi...@apache.org> on 2024/03/19 08:42:48 UTC
[PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
apeskov opened a new pull request, #16741:
URL: https://github.com/apache/tvm/pull/16741
Unfortunately, relax GPU tests are excluded from CI right now.
The testing script uses flag "-m gpu" to run gpu tests, but actual tests have no such mark.
--
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
Re: [PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #16741:
URL: https://github.com/apache/tvm/pull/16741#issuecomment-2066664192
(Aside from all my pontificating, I think this PR is ready to merge once the CI passes.)
I took a quick glance at the failing cases, and it looks like [this line](https://github.com/apache/tvm/blob/main/tests/python/relax/test_transform_codegen_pass.py#L370) should be changed from `def main(x: R.Tensor):` to `def main(x: R.Tensor([4], "int64")):`.
1. The `test_no_op_for_call_to_tir` was added, and passed in local testing. Due to the markers, it wasn't actually tested in CI.
2. Recent PR added well-formed checking in TVMScript.
3. Recent PR added StructInfo for PrimFuncs, allowing a discrepancy between "R.Tensor" and "T.Buffer(T.int64(4), "int64")` to be identified.
4. This PR fixed the marks, allowing the test to run in CI.
So the test worked when it was added, should have been updated in subsequent PRs, but due to the missing pytest marks, was never noticed until 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
Re: [PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
Posted by "apeskov (via GitHub)" <gi...@apache.org>.
apeskov commented on code in PR #16741:
URL: https://github.com/apache/tvm/pull/16741#discussion_r1568848341
##########
tests/python/relax/test_codegen_cudnn.py:
##########
@@ -36,12 +36,8 @@ def reset_seed():
has_cudnn = tvm.get_global_func("relax.ext.cudnn", True)
Review Comment:
You re absolutely right. Will remove it.
--
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
Re: [PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
Posted by "ibsidorenko (via GitHub)" <gi...@apache.org>.
ibsidorenko commented on PR #16741:
URL: https://github.com/apache/tvm/pull/16741#issuecomment-2069830379
Hi, team!
Are we ready to merge this PR? It will be great to restore testing of Relax GPU tests in CI!
--
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
Re: [PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on code in PR #16741:
URL: https://github.com/apache/tvm/pull/16741#discussion_r1538074865
##########
tests/python/relax/test_codegen_cudnn.py:
##########
@@ -36,12 +36,8 @@ def reset_seed():
has_cudnn = tvm.get_global_func("relax.ext.cudnn", True)
-cudnn_enabled = pytest.mark.skipif(
- not has_cudnn,
- reason="cuDNN not enabled.",
-)
-pytestmark = [cudnn_enabled]
+pytestmark = [*tvm.testing.requires_cudnn.marks()]
Review Comment:
Nitpick: The `tvm.testing.requires_*` marks can be used as marks directly, rather than needing to be explicitly expanded with `foo.marks()`. In addition, the `pytestmark` can be a single marker, rather than a list. Between these two, this can cleaned up as:
```python
pytestmark = tvm.testing.requires_cudnn
```
##########
tests/python/relax/test_codegen_cudnn.py:
##########
@@ -36,12 +36,8 @@ def reset_seed():
has_cudnn = tvm.get_global_func("relax.ext.cudnn", True)
Review Comment:
Do we still need `has_cudnn` now that it isn't used in `pytest.mark.skipif`?
--
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
Re: [PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on code in PR #16741:
URL: https://github.com/apache/tvm/pull/16741#discussion_r1569089772
##########
tests/python/relax/test_codegen_cudnn.py:
##########
@@ -36,12 +36,8 @@ def reset_seed():
has_cudnn = tvm.get_global_func("relax.ext.cudnn", True)
-cudnn_enabled = pytest.mark.skipif(
- not has_cudnn,
- reason="cuDNN not enabled.",
-)
-pytestmark = [cudnn_enabled]
+pytestmark = [*tvm.testing.requires_cudnn.marks()]
Review Comment:
Good point. I thought I had made `Feature` be a subclass of `Mark`, but I guess I never did. In that case, I like your simplification.
--
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
Re: [PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
Posted by "apeskov (via GitHub)" <gi...@apache.org>.
apeskov commented on code in PR #16741:
URL: https://github.com/apache/tvm/pull/16741#discussion_r1568846620
##########
tests/python/relax/test_codegen_cudnn.py:
##########
@@ -36,12 +36,8 @@ def reset_seed():
has_cudnn = tvm.get_global_func("relax.ext.cudnn", True)
-cudnn_enabled = pytest.mark.skipif(
- not has_cudnn,
- reason="cuDNN not enabled.",
-)
-pytestmark = [cudnn_enabled]
+pytestmark = [*tvm.testing.requires_cudnn.marks()]
Review Comment:
I'm not sure that it's possible. Global object `pytestmark` should has type `Mark` or `List[Mark]` but our TVM markers has type `tvm.testing.utils.Feature`. So direct assignment, like you suggest above, will lead to type check error:
> TypeError: got <tvm.testing.utils.Feature object at 0x7ff96f507340> instead of Mark
Moreover, the idea of this change was to utilise hierarchical structure of tvm testing features. `pytest.mark.gpu` mark is a root of this hierarchy. So I have to extract all markers from current feature and all parents. That is exactly method `marks()` do.
But you are right this line can be slightly simplified by removing explicit list generator, like:
> pytestmark = tvm.testing.requires_cudnn.marks()
--
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
Re: [PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #16741:
URL: https://github.com/apache/tvm/pull/16741#issuecomment-2066641060
Thank you on the explanation of the different options. I agree that changing all markers in all locations is far too heavy of a lift for any one PR, and should be avoided.
For long-term solutions, I think it would make sense to have the "cpu" and "gpu" indicate which environment should be used, and not which functionality is being tested. That way, we could distinguish between a test that compiles TensorRT (doesn't require a GPU) and a test that executes TensorRT (does require a GPU). The former would run in the `cpu/pr-head` step, even though it is validating functionality required for GPUs, and the latter would run in the `gpu/pr-head` step. However, this will require adding the compile-time support to the CI's CPU container, which is a larger step overall.
--
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
Re: [PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
Posted by "apeskov (via GitHub)" <gi...@apache.org>.
apeskov commented on PR #16741:
URL: https://github.com/apache/tvm/pull/16741#issuecomment-2071042407
@Lunderberg could you please merge this one if you have no objections.
--
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
Re: [PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
Posted by "apeskov (via GitHub)" <gi...@apache.org>.
apeskov commented on PR #16741:
URL: https://github.com/apache/tvm/pull/16741#issuecomment-2063720877
Hi @Lunderberg thank you for reviewing this PR.
> The tvm.testing.requires_* marks all provide the pytest.mark.gpu flag, but they aren't yet used in all cases.
Yes I know this issue. And I absolutely agree that this PR contains logically incorrect test marks. Some tests are marked like "require_XXX" but in fact they don't. But this incorrectness was introduced not by me. A lot of test from files touched by me verify pattern matching functionality. Pattern matching is pure python code and doesn't require any cmake enabled features like specific code generator or runtime support. But they still has marker like "requires_tensorrt_codegen". So I changed one incorrect marker by another.
The reason of that was in a need to enable tests in CI. I saw 4 way to do that:
1. To make all test mark correct. Produce one more Jenkins job with enabled compilation with cuda/cublas/cutlass/tensorrt on virtual machine without GPU. And launch there only GPU specific unit tests which doesn't required real GPU. The main question how to filter tests which should be launched there.
2. Same as first but instead of creation new one Jenkins job integrate all this stuff into "cpu/pr-head". Have to change build script to add all coda dependencies. It introduces incorrectness of meaning, CI job with "cpu" in name builds "gpu" specific code.
3. Same as second but use "gpu/pr-head". Same problem as for first way. Have to change launch script and add all specific marker filter like "-m "gpu or cudagraph or tensorcore or cutlass or ***". I guess it's impossible to keep this command line correct. The new one tests and "tvm.testing.Features" will be messed in this list.
4. Just keep situation as is. And use "gpu" marker as a root marker for all type of tests we would like to launch in CI job "gpu/pr-head".
I selected the fourth way as easiest. And I realise that it's imperfect solution. If you have any other ideas how to enable all test in CI and keep testing code correct please point that.
--
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
Re: [PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #16741:
URL: https://github.com/apache/tvm/pull/16741#issuecomment-2071113698
Looks like it is still making its way through CI, but I can merge it after the CI passes.
--
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
Re: [PR] Restore "pytest.mark.gpu" for RELAX tests [tvm]
Posted by "echuraev (via GitHub)" <gi...@apache.org>.
echuraev merged PR #16741:
URL: https://github.com/apache/tvm/pull/16741
--
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