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

Remove generate_stubs.py + hard-coded stubs #109

Merged
merged 12 commits into from
Mar 28, 2022

Conversation

honno
Copy link
Member

@honno honno commented Mar 23, 2022

Following on from #104, this PR utilises the spec repo in the remaining areas of the test suite, and thus removes the need for generate_stubs.py and it's generated files. But also notably, this PR includes a reworks of the test_signatures.py tests.

I removed the use of sample arguments. Now that we've basically got full coverage of the functions/methods, we should lean on those "primary" tests to actually test interesting input arrangements (test_arange being a prime example). Instead I lean on inspect tools. This is not perfect as it looks to only work with Python-y functions, as opposed to say some PyTorch compiled functions (I wonder if this is something @asmeurer came across before with NumPy ufuncs). Still for the reasons above, I believe efforts to test by passing actual values should be focused in the primary tests. For those situations the tests just skip, don't think it's as big of a deal now. Generally updating the current test_signatures.py proved a bit difficult for me heh.

I had initially left test_signatures.py out of the introduced --ci flag because of what the primary tests try to do. I see now we've definitely missed some areas of testing parameter kind (e.g. pos-only, kw-only, both) in these primary tests, so these tests are now back in—just long-term we should aim to not add these tests to --ci.

@honno honno changed the title Signature tests use new stubs, change in scope Remove generate_stubs.py + hard-coded stubs, change scope of test_signatures.py Mar 23, 2022
@asmeurer
Copy link
Member

Yeah, the reason it was done this way was indeed because inspect only works on pure Python functions, which we can't expect libraries to be using. In fact, I would expect only numpy.array_api and maybe dask to use them. Everything else will most likely be using compiled functions that aren't inspect compatible.

@asmeurer
Copy link
Member

So if we want to augment the existing tests with inspect for the cases where it works we can. I don't know if there's much benefit to it, other than it being more robust and may being able to test some things that direct testing cannot (like whether arguments are positional-only). But we should definitely keep the manual tests as well, as they are the only thing that will work for libraries like base numpy or pytorch.

@asmeurer
Copy link
Member

Also what is --ci? I think I missed that one.

@honno
Copy link
Member Author

honno commented Mar 24, 2022

But we should definitely keep the manual tests as well, as they are the only thing that will work for libraries like base numpy or pytorch.

The problem I was finding is that it's tricky to generalise for these scenarios myself. We are doing this anyway for our primary tests, so it's looking like a maintenance burden to repeat this kind of testing. Your test_signatures.py seems to do most/all of this, but due to the complicated nature of "passing args in certain ways to test pos-only/kw-only/both" I just had to give up trying to modify your work heh.

I definitely understand that my solution here is lacking for compiled funcs and etc., but as I said we want to test this all in the primary tests anyway. The current solution here was relatively painless and I think much easier to follow due to inspect.Signature idiomatics, so it hurts less if it's ultimately "throwaway".

Also what is --ci? I think I missed that one.

Ah, I introduced the --ci flag to run only the primary and special cases tests. The idea is for CI, you can ignore the other test cases as they are redundant for the purposes of checking compliance. This is especially helpful when you having a faulty function, which will cascade errors to the many respective type promotion and signature tests, that can bloat up a list of skips.

@honno
Copy link
Member Author

honno commented Mar 24, 2022

@asmeurer I've removed my changes for now (i.e. the inspect stuff), and updated the original test_signatures.py to use the new stubs. It's a bit hacky, but I want to revisit these tests anyway, and I really want to take a break from the stubbing stuff haha. Happy with this PR on my end.

@asmeurer
Copy link
Member

I said we want to test this all in the primary tests anyway.

Maybe this is a more general problem, but it's useful to have test_signatures as a separate test. It's a very simple smoke test that you can use as the first thing on a library to see what functions are missing, which keyword arguments are missing, and which functions are present but have the wrong signature. If test_signatures fails, the other tests have no hope of working at all.

I agree the current way it's tested isn't great. Quite often you get errors that are due to the special values that are chosen, or you get some exception from the library instead of the AssertionError you'd like. An inspect based test could be valuable for libraries where it works if just for that reason.

@honno honno merged commit 9816011 into data-apis:master Mar 28, 2022
@honno honno mentioned this pull request Apr 12, 2022
@honno honno changed the title Remove generate_stubs.py + hard-coded stubs, change scope of test_signatures.py Remove generate_stubs.py + hard-coded stubs Apr 28, 2022
@honno honno deleted the stubbing-n-sig-tests branch February 28, 2024 13:21
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 this pull request may close these issues.

2 participants