Repository navigation
ENH: Add set of tests to type check the public APIs #45252
Description
From discussion on pandas typing meeting on January 7, 2022 (notes at https://docs.google.com/document/d/1tGbTiYORHiSPgVMXawiweGJlBw5dOkVJLY-licoBmBU/)
Follow up to discussion here: #28142 (comment)
Goal is to have a set of tests that make sure that users of the pandas API writing correct code work with whatever type stubs we publish.
Example of an approach that is suggested was developed within the pandas-stubs project based on various code snippets which are then tested with mypy : https://github.com/VirtusLab/pandas-stubs/tree/master/tests/snippets
@zkrolikowski-vl and @joannasendorek from that project have offered to help (nudge, nudge)
Ideally we would add this testing framework to the CI before starting to bring over the stubs.
Activity
simonjayhawkins commentedon Jan 8, 2022
see #40202 for a less verbose but more primitive approach.
Using pytest is perhaps more robust. How easy is is to parametrize testing of types? I assume that parametrization cannot be used for static checks.
The method in #40202 has several issues. If anyone knows of other approaches to testing types, would much appreciate posting suggestions here.
simonjayhawkins commentedon Jan 8, 2022
from #40202 (comment)
repeating here so that this option is not overlooked if the solution gets too complex.
twoertwein commentedon Jan 8, 2022
One potentially easier to implement option is to have something like mypy_primer but for pandas. Typeshed uses mypy_primer to automatically comment on each PR whether it would change type annotations as inferred by mypy.
"Pandas_primer" would help to 1) prevent involuntary changes to public type annotations and to 2) verify that intended changes actually have an effect.
zkrolikowski-vl commentedon Jan 10, 2022
In pandas-stubs, we've decided to go with the simplest method, which doesn't require any extra setup or testing constructs. Here are some of the other tools we've looked at (not including dynamic type-checkers):
Coming back to our solution, there's a way to test negative scenarios in our approach but it's far from elegant:
This will cause
mypynot to return errors when the line in-fact is incorrect,whereas using it incorrectly
will result in an error
The issue that remains is that our whole approach is static and I don't see it being easily integrated with parametric list of use-cases.
mypy_primer seems like an another interesting approach. Potentially difficult to integrated into pandas CI but might be worth the extra effort.
Overall it seems like either pytest-mypy-testing/ @simonjayhawkins 's (with some work) or our (pandas-stubs) solution might be a semi-immediate fit for pandas use-case.
@simonjayhawkins if I may ask: Why was your work discontinued? Was it just inconvenient at the time?
Dr-Irv commentedon Jan 10, 2022
Here is why I like the approach taken by the
pandas-stubsproject: https://github.com/VirtusLab/pandas-stubs/tree/master/tests/snippetsmypytest#type: ignorecomments and use the mypywarn_unused_ignoresoption to make sure that theignoreline is correct (and tested with future changes to the stubs)From my experience in submitting a bunch of PR's to the Microsoft
python-type-stubsproject, which has no testing for the pandas stubs, I can tell you that I am always concerned that a fix I submit for the stubs will break something else. If they had included something like what thepandas-stubsproject did, it would make life a lot easier to do more test-driven development of the stubs. This is especially true when you make a change inpandas/_typing.pythat changes one of the top-level types. It's hard to know if you have then made some downstream stub too narrow or too wide.joannasendorek commentedon Jan 10, 2022
The approach we took in pandas-stubs was the most pragmatical for our usage and capacity we have to maintain the project. We can assume that any stubs will not be perfect and when people will first start using them, they will get some errors (I applied our stubs for one of the thousand lines project and made 10 fixes to pandas-stubs based on that only). So whenever this happens, we would first re-create the false negative case provided, modify the stubs and make sure the exact usage is corrected. From this moment, this particular usage is guarded so people using the project shouldn't notice regression and we can add only small improvements at once.
simonjayhawkins commentedon Jan 10, 2022
abandoned due to #40202 (comment). In pandas we tend not merge stuff if there are any objections.
I assumed from that position and the notes from the meeting...
that the pandas-stubs would be integrated into out pytest tests, but I guess I got that wrong from not attending the meeting.
@Dr-Irv I assume from #45252 (comment) that your objections to previous attempt no longer apply?
Dr-Irv commentedon Jan 10, 2022
What we decided is that the testing approach from
pandas-stubswould be integrated, but we would use the Microsoft stubs as a starting point, because they are more complete than thepandas-stubs.I'm not sure which objections you are referring to....
What I said there is that if we could annotate all of our existing tests (which means adding
#type: ignorestatements in lots of places), that would be a way to test the stubs. But I think that would take a lot of effort.Since then, I've seen the
pandas-stubsapproach and I think it is easier to start from there, and expand the methodology in there to cover many use cases of any stubs we decide to publish.simonjayhawkins commentedon Jan 10, 2022
specifically from #40202 (comment)
IIUC the panda-stubs approach, if not using pytest, is not so different from #40202. i.e. we need to enumerate all the possibilities and have a separate set of tests.
also from #40202 (comment)
The pandas-stubs testing approach, if not integrated into the current testing, also does not satisfy this.
Dr-Irv commentedon Jan 10, 2022
I was more referring to using the
reveal_type()aspect of the testing.I don't think we have to enumerate all the possibilities. We can start with a set of tests that represent "common" usage, and if the stubs are not covering some particular use case (by being too wide or too narrow), we can just add to those stubs.
That's correct. But it's a first step forward. I would like it if we could put typing into our testing code, and just run
mypyon that, but it's a much larger effort that just starting with thepandas-stubstesting approach.simonjayhawkins commentedon Jan 10, 2022
+1
At the end of the day it's not so different than the previous attempt. We would at least have some testing and the tests in place (esp for the overloads).
changing those tests at a later date from say
to
would not be difficult and would improve the robustness of the checks.
23 remaining items
twoertwein commentedon Apr 30, 2022
@Dr-Irv I think you mentioned that the MS stub started using a not-yet supported argument of
reveal_typeto assert the type. Mypy 0.950 now supports something similar:typing_extensions.assert_typepython/mypy#12584 (andtyping.assert_typein future python versions).Dr-Irv commentedon Apr 30, 2022
@twoertwein Thanks. It appears that
assert_type()was also added topyrightas well. This will clean up various tests in the MS stubs and thepandas-stubsrepo that I'm working on here: https://github.com/pandas-dev/pandas-stubssimonjayhawkins commentedon May 2, 2022
cool. this will overcome the deficiencies of the "assignment" method that I commented on in #45252 (comment) and now align the testing with the numpy method (that was done in #40202)?
simonjayhawkins commentedon May 2, 2022
@Dr-Irv can/should we move this issue (and maybe others) to https://github.com/pandas-dev/pandas-stubs or best to keep that to stub specific issues since I think we could also use the test framework to test the pandas code too. see my comment in mailing list thread.
simonjayhawkins commentedon May 2, 2022
I guess there are two ways to do this.
or both. if there is testing against the pandas codebase in pandas-stubs, 1. is also more likely to succeed if automated.
Dr-Irv commentedon May 2, 2022
I believe that to be the case.
Dr-Irv commentedon May 2, 2022
I could see it going either way. From the pandas development standpoint, we would want to make sure that changes to pandas in a PR don't cause the tests in
pandas-stubsto fail. So keeping this issue here makes sense. On the other hand, from thepandas-stubsviewpoint, we'd have to figure out how to manage testing the stubs that test against the released version of pandas, versus testing stubs that test against themainbranch of pandas, so having an issue there makes sense as well.There are other issues here that are purely about public type stubs that could possibly be moved over (and maybe closed).
twoertwein commentedon May 2, 2022
I really like the idea of testing
pandaswith its own annotations andpandas-stubs, but I'm honestly not sure whetherpandas-stubscan be used to checkpandaswithout havingpandas-stubs-specific ignore comments inpandas(or some patch/sed magic).I don't think this is a particularly good example, but it might demonstrate the potential issue:
__getitem__will probably never return all possible types inpandas-stubs(#46616), I could imagine that (after__getitem__is typed with unions inpandas), that mypy will complain about unnecessary comparisons when usingpandas-stubs.Instead of checking
pandaswithpandas-stubs, it might be an option to somehow (don't ask me how) enforce thatpandas-stubsonly contains annotations where different annotations are intended (for example__getitem__) and functions that are not annotated inpandas. Functions that have suitable annotations inpandascould simply(?) be imported frompandas(ifpandasis a dependency ofpandas-stubs).Dr-Irv commentedon May 2, 2022
I think we are not talking about using the
pandas-stubsto test pandas source. I think the idea is that we make sure that changes topandas-stubswork with both released versions of pandas, and in-development versions of pandas, and, conversely, that changes topandasin development don't break the stubs. The stubs are tested withmypy,pyright, but alsopytest, so you could imagine a change inpandasbreaking apytesttest.I don't think that you can have some stubs in a
pandas-stubspackage, and the rest in the source code. Not sure the type checkers will manage that.twoertwein commentedon May 2, 2022
If the stubs explicitly import them (and re-export them as public functions in
__all__or withas) it should work. But havingpandasas a dependency ofpandas-stubsmight not be great.simonjayhawkins commentedon May 3, 2022
Just to be clear, my comment #45252 (comment) regards the test framework only.
In the discussion about the challenges of making pandas stubs public (mailing list, dev meeting and elsewhere), my support for having a separate stubs package was on the basis that we would have a test framework that could help keep the standalone stubs package and the pandas codebase more consistent.
Other than the test framework, we should probably keep the standalone pandas-stubs and inline annotations in pandas decoupled. This will allow us to use all the latest typing features in pandas-stubs as they become available. It will also allow us to continue to add annotations to the pandas code without further constraints (it's quite a challenge already relative to the ease of creating stubs)
I suspect that we may need to comment out failing tests (on the pandas side) with a TODO in some cases or just allow the test job to fail if we copy over the tests, but I hope that we wouldn't need to change code on the pandas side or stubs on the pandas-stubs side to share the tests.
Dr-Irv commentedon May 3, 2022
I think we are all in agreement on keeping the public stubs separate.
I'm not sure that the tests in
pandas-stubswould be good to use as typing tests against the pandas code base for the following reason. I intend to addassert_typeto those tests. For some methods/functions, the public stubs may return a more narrow type than the internal types declare. As an example, consider the discussion we had about whetherpd.to_datetime()should indicate that it can returnNaTTypein all the overloads. For the public stubs, we dont' want to indicate that - it would make users docastall the time. But internally, we may want to indicate the full wider type. So a test inpandas-stubswould do anassert_typeon the narrower type, but if you use that test on the pandas code base, that test would fail.simonjayhawkins commentedon May 3, 2022
I think that is exactly the reason to use the same tests. IMO we should make the annotations as consistent as possible and part of the original argument on why we did not have separate stubs before now.
I still disagree with this approach, but am happy to await user feedback once the stubs are public on whether this is a significant issue.
I do expect that we would need to have some allowed failures at first, but fixing these would make the pandas-stubs and in-line type annotations more consistent.
Dr-Irv commentedon Jul 9, 2023
@simonjayhawkins @twoertwein
I'm thinking we can close this, as we are now testing the type checking of the published
pandas-stubsAPI's as part of the CI in that project. Would like your opinion.twoertwein commentedon Jul 9, 2023
Could also leave this issue open if we hope to have consistency checks between pandas and pandas-stubs.