Uh oh!
There was an error while loading. Please reload this page.
[Docs] Added documentation on pytest target parametrization. - #8638
Conversation
Lunderberg
commented
Aug 3, 2021
@mbrookhart The documentation that we had discussed, describing how tests can be run across multiple targets. @hogepodge I have this in the |
Follow-up from apache#8542, to document existing features.
b19acc1 to
64fb75dCompare
tkonolige
left a comment
There was a problem hiding this comment.
Thanks for all the hard work @Lunderberg!
Given that this document is intended for people who are going to write tests, I'd first specify how they should write the tests and then talk about how it actually works. I.e. put all the parameterize_targets and requires_ stuff first.
| - ``@pytest.mark.gpu`` - Tags a function as using GPU | ||
| capabilities. This has no effect on its own, but can be paired with | ||
| command-line arguments ``-m gpu`` or ``-m 'not gpu'`` to restrict | ||
| which tests pytest will executed. Typically not called on its own. |
There was a problem hiding this comment.
I'd specify that developers should never use this mark explicitly.
There was a problem hiding this comment.
Sounds good, change made.
| which tests pytest will executed. Typically not called on its own. | ||
| - ``@tvm.testing.uses_gpu`` - Applies ``@pytest.mark.gpu``. Needed | ||
| only for tests that explicitly loop over |
There was a problem hiding this comment.
Suggest that this is what should be used to mark tests requiring GPUs, but prefer parameterize_targets.
There was a problem hiding this comment.
Sounds good, change made.
| .. code-block:: python | ||
| # Old style, not recommend anymore |
There was a problem hiding this comment.
I'd prefer saying "do not use" vs "not recommended".
There was a problem hiding this comment.
Sounds good, change made.
| .. code-block:: python | ||
| # New style, implicitly parametrized to run on all |
There was a problem hiding this comment.
I thin you can remove all the new style comments given that this document is intended for how people should write targets now.
There was a problem hiding this comment.
Sounds good, change made.
Lunderberg
commented
Aug 10, 2021
@tkonolige All requested changes are made, how does it look now? |
jroesch
left a comment
There was a problem hiding this comment.
When @tkonolige signs off feel free to merge @Lunderberg
hogepodge
commented
Aug 10, 2021
Thanks @Lunderberg. All looks good to me. |
leandron
commented
Aug 11, 2021
Oh sorry @tkonolige I merged this because I saw others approving. Do you mind if we deal with any changes you might require, in follow-up patches? |
leandron
commented
Aug 11, 2021
This is merged now, thanks @jroesch@hogepodge@Lunderberg and @tkonolige! |
Lunderberg
commented
Aug 11, 2021
@tkonolige and I had some follow-up discussion on moving the recommended style to the top, with the explanation of how it works and why that style is preferred underneath. That way, it is better suited for developers writing/debugging tests, who would primarily want to know what to write, but don't want to get bogged down in the how/why. Since this one is merged, I'll add it in an additional PR. |
…8638) * [Docs] Added documentation on pytest target parametrization. Follow-up from apache#8542, to document existing features. * [Docs] Updated pytest parametrization documentation following review Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
…8638) * [Docs] Added documentation on pytest target parametrization. Follow-up from apache#8542, to document existing features. * [Docs] Updated pytest parametrization documentation following review Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
Follow-up from #8542, to document existing features.