Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[testing] Insufficient coverage in test suite #1090

Open
kshitij12345 opened this issue Dec 23, 2022 · 5 comments
Open

[testing] Insufficient coverage in test suite #1090

kshitij12345 opened this issue Dec 23, 2022 · 5 comments

Comments

@kshitij12345
Copy link
Contributor

kshitij12345 commented Dec 23, 2022 •
edited
Loading

In functorch test suite, we use sample_inputs to get samples from an OpInfo. The problem is that sample_inputs may or may not cover all the case/overloads for an operator. I think we should use reference_inputs which super set of sample_inputs and more comprehensive. (Though this will increase the test times).

Switching sample_inputs to reference_inputs leads to bunch of failure for test_op_has_batch_rule including the ones mentioned in #1080 #1069

Refer to pytorch/pytorch#91355 for failures.

cc: @zou3519
@kshitij12345 kshitij12345 mentioned this issue Dec 23, 2022
Closed
@zou3519
Copy link
Contributor

zou3519 commented Dec 27, 2022

Good catch!

Do we have a sense of how much this increases testing time by?

@kshitij12345
Copy link
Contributor Author

Will confirm this in a day or two.

@kshitij12345
Copy link
Contributor Author

Timings are posted here : pytorch/pytorch#91355 (comment)

pytorchmergebot pushed a commit to pytorch/pytorch that referenced this issue Jan 6, 2023
@kshitij12345 @pytorchmergebot
[functorch] test: try using reference_inputs in vmap tests (#91355)
a51090d 
Ref pytorch/functorch#1090

Timings:

`test_vmap_exhaustive`

After PR
```
== 1168 passed, 55 skipped, 2353 deselected, 153 xfailed in 195.07s (0:03:15) ==
```

Before PR
```
== 1134 passed, 55 skipped, 2316 deselected, 150 xfailed in 77.18s (0:01:17) ==
```

`test_op_has_batch_rule`

After PR
```
== 988 passed, 57 skipped, 2353 deselected, 331 xfailed in 144.70s (0:02:24) ==
```

Before PR
```
== 969 passed, 57 skipped, 2316 deselected, 313 xfailed in 65.86s (0:01:05) ==
```

Pull Request resolved: #91355
Approved by: https://github.com/zou3519
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this issue Jan 6, 2023
@kshitij12345 @pytorchmergebot
[functorch] test: try using reference_inputs in vmap tests (#91355)
2354ff5 
Ref pytorch/functorch#1090

Timings:

`test_vmap_exhaustive`

After PR
```
== 1168 passed, 55 skipped, 2353 deselected, 153 xfailed in 195.07s (0:03:15) ==
```

Before PR
```
== 1134 passed, 55 skipped, 2316 deselected, 150 xfailed in 77.18s (0:01:17) ==
```

`test_op_has_batch_rule`

After PR
```
== 988 passed, 57 skipped, 2353 deselected, 331 xfailed in 144.70s (0:02:24) ==
```

Before PR
```
== 969 passed, 57 skipped, 2316 deselected, 313 xfailed in 65.86s (0:01:05) ==
```

Pull Request resolved: #91355
Approved by: https://github.com/zou3519
@lezcano
Copy link
Contributor

lezcano commented Jan 12, 2023

This one should probably be closed

@kshitij12345
Copy link
Contributor Author

I want to check other tests in functorch/test_ops.py like test_vmapvjp and friends. So keeping it open till then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@lezcano @zou3519 @kshitij12345