Lunderberg edited a comment on pull request #8542: URL: https://github.com/apache/tvm/pull/8542#issuecomment-888492592
Agreed, this ended up being a much larger impact than I had expected, and this PR shouldn't be merged until/unless we have consensus on it. (This comment ended up being a bit longer than I had expected, as I was looking up some history for my own knowledge, and figured I might as well write it down.) Regarding changes to how unit tests are parametrized, when they are marked with `@pytest.mark.gpu`, here's the history as best as I know. 1. (State prior to listed changes.) Unit tests explicitly apply to a particular target. The CI explicitly checks for the presence of `@pytest.mark.gpu`, from the [`-m` gpu` argument](https://github.com/apache/tvm/blob/main/tests/scripts/task_python_unittest_gpuonly.sh#L20). A test can run over multiple targets by explicitly looping over `tvm.testing.enabled_targets()`. 2. #6331 added the `@tvm.testing.parametrize_targets` decorator, to parametrize across all enabled targets. A unit-test decorated with `parametrize_targets` has an independent test run for each item in `tvm.testing.enabled_targets()`. These independent tests are marked with the appropriate marks (e.g. `tvm.testing.requires_cuda`, which includes `pytest.mark.gpu`) based on the target in use. 3. #8010 added parametrized fixtures for `target` and `dev`. The goal was to reduce the amount of boilerplate needed for any given unit test. A unit test that accepts arguments of `target` or `dev` is automatically parametrized over the enabled targets. This behavior is described in the [tvm-rfc repo, PR#7](https://github.com/apache/tvm-rfcs/pull/7). There's some conversation on finalizing exactly how and what should be cached, but the RFC is up-to-date with the latest conversation and implementation. Relevant to the GPU-marking, this followed the same convention as #6331, marking tests as requiring a GPU if the target parameter is a GPU target. This worked, but allowed for accidentally forgetting an annotation for a target-specific test. This particular PR was motivated by #8387 , which added a missing annotation of `@requires_gpu` to a test that explicitly used `pytest.mark.parametrize`. In order to remove that as a potential failure mode, this PR identifies cases where the test has been explicitly parametrized over targets, and adds the same marks as if it had used `@tvm.testing.parametrize_targets`, including `@tvm.testing.requires_gpu` if it is a GPU target. That is why the ONNX-specific tests started running with this PR, because the parametrization of `target` then received the target-specific marks. I think the source of my misunderstanding was that `@tvm.testing.requires_gpu` serves two purposes. The first is to indicate that the test should be skipped if no GPU is present. The second purpose (which I hadn't known) is to indicate that a test should be run in the GPU step of the CI. By adding the target-specific marks, I had been assuming that their lack would be accidental rather than intentional. -- 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]
