DISC: Use of assertions in user-facing code

[dsaxton]

There are a variety of places in the code base where we have some bare assertions that can be somewhat confusing when encountered by end-users (e.g., #35509). As @WillAyd pointed out these are sometimes useful in convincing mypy of something's type, but is this nonetheless something we want to avoid as a general rule? If indeed the assertion is valid then it shouldn't actually raise, but if it can then probably we could come up with a more appropriate error type / message than an empty assertion.

[jbrockmendel]

IIUC these are largely mypy-motivated. on those im going to follow @simonjayhawkins's lead

[simonjayhawkins]

see also #32785 for related discussion or asserts for mypy. (we have had a few discussions on this in the past in different places.)

IIUC these are largely mypy-motivated.

in the issue referenced, it appears to not be a mypy related assert, as a assert/cast is not necessary for type narrowing in this case.

but is this nonetheless something we want to avoid as a general rule?

the assertion did cause as regression, but at the same time did reveal that a wrong path was being taken (#37023 (comment)) and a gap in our testing.

In this case the assert was helpful (even if not for end users) since although the assert added should not have been necessary since the slobj argument of _slice is typed as slice and we would expect mypy to report this at the call site.

The revealed type of indexer in __getitem__ is 'Any' ( 'reveal_type' always outputs 'Any' in unchecked functions)

As we add more types (and refine the return types) asserts and casts could become less necessary, however there are other issues such as is_* functions not narrowing types.

If indeed the assertion is valid then it shouldn't actually raise, but if it can then probably we could come up with a more appropriate error type / message than an empty assertion.

These assertions are generally to detect bugs in internal pandas code and not users code, so in many cases not necessary imo.

[dsaxton]

I can see how these could be useful in finding paths that aren't being tested directly, but am more wondering whether it's appropriate to release them into the wild (I don't know if there's an easy way to remove assertions in release code).

If there are any undetected bugs at release time then I would say the assertions have not succeeded in finding them, at least during development. To me it seems allowing these bugs to then break end user code through a bare assertion (and then hopefully leading to an issue being created on GitHub) may not be the best approach, especially since the bug might not be critical for what the user is trying to accomplish.

[WillAyd]

You can use the -O flag to remove asserts. Fairly certain that applies to distributed wheels as well, though I would think better to keep. I admittedly don’t know if there is a concrete problem we are trying to solve

…

Sent from my iPhone

On Oct 26, 2020, at 9:49 AM, Daniel Saxton ***@***.***> wrote:  I can see how these could be useful in finding paths that aren't being tested directly, but am more wondering whether it's appropriate to release them into the wild (I don't know if there's an easy way to remove assertions in release code). If there are any undetected bugs at release time then I would say the assertions have not succeeded in finding them, at least during development. To me it seems allowing these bugs to then break end user code through a bare assertion (and then hopefully leading to an issue being created on GitHub) may not be the best approach, especially since the bug might not be critical for what the user is trying to accomplish. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[rhshadrach]

IMO asserts should be used to check an assumption that means there is a bug when incorrect. As an end user, if I see code raising an assert, it makes me more aware that I may have stumbled on a bug rather than incorrect usage. Asserts can be incorrectly placed resulting in unwelcome behavior, but that's the same with any line of code.

[jorisvandenbossche]

General typing question: why not use cast instead of assert isinstance ?
(or stated differently: what are the differences and when should we use which of those two generally speaking?)

[WillAyd]

cast eliminates type safety. assert maintains type safety, but at the cost of potential runtime errors. If mypy had a static_assert similar to C it would most likely be ideal for our code base, but that doesn’t yet exist. See python/mypy#5687

…

Sent from my iPhone

On Dec 7, 2020, at 2:52 AM, Joris Van den Bossche ***@***.***> wrote:  General typing question: why not use cast instead of assert isinstance ? (or stated differently: what are the differences and when should we use which of those two generally speaking?) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.