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.

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

Signature tests not catching missing keyword arguments #175

Closed
asmeurer opened this issue Mar 17, 2023 · 2 comments · Fixed by #177
Closed

Signature tests not catching missing keyword arguments #175

asmeurer opened this issue Mar 17, 2023 · 2 comments · Fixed by #177
Assignees

Comments

@asmeurer
Copy link
Member

For array_api_compat.torch, the triu function is missing the k keyword argument. But none of the tests catch this:

array_api_tests/test_has_names.py::test_has_names[creation-triu] PASSED                                                                                           [ 50%]
array_api_tests/test_signatures.py::test_func_signature[triu] SKIPPED (Signature for triu() is not inspectable and is too troublesome to test for otherwise)      [100%]

This is a great example of something that the old signature tests would handle just fine. I just tested it in 2022.03.22, the version before #110 was merged, and it finds the error just fine.

E               AssertionError: Unexpected exception TypeError("triu() got an unexpected keyword argument 'k'"): triu() should accept the keyword-only argument 'k'

So let's please just reintroduce these old signature tests as a fallback. They are much better than nothing and hardly "too troublesome". We are missing real bugs because of this.

@asmeurer
Copy link
Member Author

And this is at least the second instance of this data-apis/array-api-compat#22. Who knows how many other such issues there are.

@honno
Copy link
Member

honno commented Mar 28, 2023

They are much better than nothing and hardly "too troublesome".

To clarify, it's too troublesome to test with the same guarantees when testing inspectable signatures (e.g. trying different combinations of pos and/or keyword arguments for every arg that an Array API function supports).

Fallbacks are understandable but its weird to maintain given we (ideally) have primary tests for every function that implicitly check signatures anyway... but if they're not all getting implemented (like triu() 1) then lets go for it. I think I'd like a separate test case (i.e. test_signature_basic) to really hammer home the lack of thoroughness on our part, and maybe even only for the known uninspectable functions in the ecosystem (read: in torch).

Footnotes

  1. I didn't even know we had triu() in 2021.12 haha, must of missed it when I was (evidently not) implementing the remaining creation tests way back when.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants