cleanup test parametrization #113855
Closed
cleanup test parametrization #113855
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/113855
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit f64d352 with merge base 5fb1d8f ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
lezcano
approved these changes
Nov 16, 2023
huydhn
approved these changes
Nov 16, 2023
pytorchmergebot
pushed a commit
that referenced
this pull request
Nov 16, 2023
#113340 was reverted initially due to a bad default parametrization name. The test looked like ```python @common_utils.parametrize( "type_fn", [ type, lambda obj: obj.__class__, ], ) def test_access_class_method_from_user_class(self, type_fn): ``` This is a valid parametrization, but results in these default test names: ```bash ❯ pytest test/dynamo/test_export.py -k test_access_class_method_from_user_class --co -q test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn_<class 'type'> test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn_<function ExportTests_<lambda> at 0x7f3be5de0c10> ``` Ignoring the whitespace in the test names, which can lead to other issues down the line, the problem in #113340 was that the lambda parameter included a memory address. IIUC, internally, the tests are not collected and run in the same process. Meaning, the address of the lambda and in turn the test name is no longer valid on the runner. This is fixed earlier in the stack by giving the parametrization an explicit name with `subtest`, but this PR is about preventing issues in the default case. `pytest` solves this by simply using the name of the parameter plus its index as id in the test name: ```python import pytest class Foo: def __repr__(self): return str(id(self)) @pytest.mark.parametrize( "bar", [ pytest.param(type), pytest.param(lambda obj: obj.__class__), pytest.param(Foo()), ], ) def test_foo(bar): pass ``` ``` ❯ pytest main.py --co -q main.py::test_foo[type] main.py::test_foo[<lambda>] main.py::test_foo[bar2] ``` `pytest` has better defaults for `type` and `lambda` than we do, but is has a safe default for custom objects. This PR aligns our default test name with `pytest`. Using the parametrization from above again, we now collect ```bash ❯ pytest test/dynamo/test_export.py -k test_access_class_method_from_user_class --co -q test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn0 test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn1 ``` which might not be as expressive at first glance, but at least prevents bugs. Pull Request resolved: #113856 Approved by: https://github.com/malfet, https://github.com/huydhn ghstack dependencies: #113855
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
Cleanup from #113340 (comment).
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @aakhundov @kadeng