robustify parametrize default name #113856

pmeier · 2023-11-16T09:39:19Z

Stack from ghstack (oldest at bottom):

#113340 was reverted initially due to a bad default parametrization name. The test looked like

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

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

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

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

[ghstack-poisoned]

pytorch-bot · 2023-11-16T09:39:22Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/113856

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 6f840f1 with merge base 5fb1d8f ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

ghstack-source-id: fbce670132357a159ded80611b8ddb73c8cb50db Pull Request resolved: #113856

pmeier · 2023-11-16T09:54:55Z

torch/testing/_internal/common_utils.py

        """ Returns a string representation for the given arg that is suitable for use in test function names. """
        if isinstance(value, torch.dtype):
            return dtype_name(value)
        elif isinstance(value, torch.device):
            return str(value)
        # Can't use isinstance as it would cause a circular import
-        elif value.__class__.__name__ == 'OpInfo' or value.__class__.__name__ == 'ModuleInfo':
+        elif type(value).__name__ in {'OpInfo', 'ModuleInfo'}:


Driveby simplification.

pmeier · 2023-11-16T09:55:24Z

torch/testing/_internal/common_utils.py

            return value.formatted_name
        else:
-            # Include name and value separated by underscore.
-            return f"{name}_{str(value).replace('.', '_')}"
+            return f"{name}{idx}"


This is the actual fix. Everything else are just modifications to get access to the index here.

#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. [ghstack-poisoned]

ghstack-source-id: 7440bd933c5f503d69282529f577cee9e6948418 Pull Request resolved: #113856

pmeier · 2023-11-16T11:16:36Z

torch/testing/_internal/common_utils.py

            return value.formatted_name
-        else:
-            # Include name and value separated by underscore.
+        elif isinstance(value, (int, float, str)):


We keep the previous naming scheme for "safe" primitive type.

#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. [ghstack-poisoned]

ghstack-source-id: 5b81b9d3cf6a8fd0eece8f1f79285365db5306a5 Pull Request resolved: #113856

malfet

Looks fine, though again, I think excessive parametrization is detrimental to UX (for example adding two lambdas to the list of parameters will result in two tests having identical names)

pmeier · 2023-11-16T20:55:26Z

@pytorchbot merge

pytorchmergebot · 2023-11-16T20:57:19Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

robustify parametrize default name

9c264e0

[ghstack-poisoned]

pmeier requested a review from a team as a code owner November 16, 2023 09:39

pmeier mentioned this pull request Nov 16, 2023

cleanup test parametrization #113855

Closed

pmeier added a commit that referenced this pull request Nov 16, 2023

robustify parametrize default name

eba8039

ghstack-source-id: fbce670132357a159ded80611b8ddb73c8cb50db Pull Request resolved: #113856

pmeier added topic: not user facing topic category release notes: devx labels Nov 16, 2023

pytorchbot added the open source label Nov 16, 2023

pmeier requested a review from malfet November 16, 2023 09:54

pmeier commented Nov 16, 2023

View reviewed changes

pmeier added a commit that referenced this pull request Nov 16, 2023

robustify parametrize default name

de0d58f

ghstack-source-id: 7440bd933c5f503d69282529f577cee9e6948418 Pull Request resolved: #113856

pmeier commented Nov 16, 2023

View reviewed changes

pmeier added a commit that referenced this pull request Nov 16, 2023

robustify parametrize default name

92de2df

ghstack-source-id: 5b81b9d3cf6a8fd0eece8f1f79285365db5306a5 Pull Request resolved: #113856

malfet approved these changes Nov 16, 2023

View reviewed changes

huydhn approved these changes Nov 16, 2023

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 16, 2023

pytorchmergebot added the merging label Nov 16, 2023

pytorchmergebot added Merged and removed merging labels Nov 16, 2023

pytorchmergebot closed this in 769f924 Nov 16, 2023

facebook-github-bot deleted the gh/pmeier/46/head branch November 20, 2023 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

robustify parametrize default name #113856

robustify parametrize default name #113856

pmeier commented Nov 16, 2023 •

edited

Loading

pytorch-bot bot commented Nov 16, 2023 •

edited

Loading

pmeier Nov 16, 2023

pmeier Nov 16, 2023

pmeier Nov 16, 2023

malfet left a comment

pmeier commented Nov 16, 2023

pytorchmergebot commented Nov 16, 2023

robustify parametrize default name #113856

robustify parametrize default name #113856

Conversation

pmeier commented Nov 16, 2023 • edited Loading

pytorch-bot bot commented Nov 16, 2023 • edited Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/113856

✅ No Failures

pmeier Nov 16, 2023

Choose a reason for hiding this comment

pmeier Nov 16, 2023

Choose a reason for hiding this comment

pmeier Nov 16, 2023

Choose a reason for hiding this comment

malfet left a comment

Choose a reason for hiding this comment

pmeier commented Nov 16, 2023

pytorchmergebot commented Nov 16, 2023

Merge started

pmeier commented Nov 16, 2023 •

edited

Loading

pytorch-bot bot commented Nov 16, 2023 •

edited

Loading