bug: Incomplete type hints

[binste]

What happened?

While evaluating Ibis at work, I found some unexpected mypy errors:

import ibis
import ibis.selectors as s
from ibis import deferred as d_

table = ibis.table({"col1": "int"})

# error: Argument 1 to "order_by" of "Table" has incompatible type "Value"; expected "str | Column | tuple[str | Column, bool] | Sequence[str] | Sequence[Column] | Sequence[tuple[str | Column, bool]] | None"  [arg-type]
table.order_by(ibis.desc("col1"))

# error: Argument 1 to "mutate" of "Table" has incompatible type "Across"; expected "Sequence[Expr] | None"  [arg-type]
table.mutate(s.across(["col1"], d_.mean()))

# error: Argument "col1_again" to "mutate" of "Table" has incompatible type "Deferred"; expected "Value"  [arg-type]
table.mutate(col1_again=d_["col1"])

I'll keep adding to this issue in case I'd find more. Also, a big thank you for making Ibis a typed library! :)

What version of ibis are you using?

Ibis 6.1
Mypy 1.4.0
Python 3.11

What backend(s) are you using, if any?

No response

Relevant log output

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[cpcloud]

@binste Thanks for the issue!

Yeah, we're not testing mypy at the moment. The vast majority of ibis was implemented before type hints became widespread and it's a lot of work to add them.

I know @jcrist has thoughts here. I think there may be a way we can ensure that our type hints are as-expected without having to annotate the entire codebase.

[cpcloud]

Hopefully this isn't a blocker for you, but let us know!

[binste]

The vast majority of ibis was implemented before type hints became widespread and it's a lot of work to add them.

I can relate to that. It's the same story for Vega-Altair where I'm currently in the process of typing at least the public API and it's not easy...

I know @jcrist has thoughts here. I think there may be a way we can ensure that our type hints are as-expected without having to annotate the entire codebase.

I think the issue title I chose is misleading. Mypy allows for gradually typing existing codebases and so it's fine if Ibis is not completely typed. Issues arise when the provided type hints are either wrong (e.g. specified types on function input arguments cannot be processed by function) or incomplete (e.g. a union of types does not cover all accepted types) where the later one seems to be the case for the 3 errors I mentioned above.

The process which we follow for Altair and what I've seen with other libraries is that

• mypy is implemented first and all existing errors are fixed
• more type hints are gradually introduced starting with the parts of the library that benefit from it the most
• once you're happy with the coverage (doesn't have to be 100%) you add the py.typed file to tell type checkers such as mypy that the type hints can be used for type checking in other code bases.

By introducing the py.typed file in #5304 without having mypy enabled, these errors now show up in other code bases but not yours and users will have to add # type: ignore comments to get mypy to pass.

Based on #2823 and the related PRs, I take it that it's not easy to fix all existing mypy issues. For what it's worth, I think you could either remove again the py.typed file so that other code bases are not affected, or introduce mypy for checking and get it to pass. You could remove type hints in the parts of the code which are tricky (or use typing.Any) so that mypy does not check them.

Running mypy ibis --ignore-missing-imports after pip install -e . gives me 130 individual errors but many of them have the same root causes.

[binste]

Happy to help out with reviewing PRs or creating some smaller ones myself to move this forward! Curious to hear what your thoughts are and your past experience with type hints in this codebase.

[binste]

I'll document here some mypy errors where I'm not familiar enough with Ibis to create a PR:

• S TypeVar in core.py and window.py needs a different default argument. It currently uses typing.Any but default must be a subtype of bound, see PEP 696.
• Some redefinitions of variables and methods
  • ibis/expr/datatypes/__init__.py:6: error: Name "dt" already defined (by an import) [no-redef]
  • ibis/expr/operations/__init__.py:6: error: Name "T" already defined (possibly by an import) [no-redef]
  • ibis/expr/types/strings.py:1513: error: Name "__mul__" already defined on line 599 [no-redef]
• Objects which do not exist:
  • `ibis/expr/analysis.py:775: error: Name "ops.Table" is not defined [name-defined]
  • ibis/expr/api.py:45: error: Module "ibis.common.typing" has no attribute "SupportsSchema" [attr-defined]

[kszucs]

Thanks @binste for working on this!

I'm going to take a closer look to the type errors you mentioned.

[binste]

Thank you @kszucs! Here's another:

There are a lot of Attributes without a default cannot follow attributes with one errors which might all have the same root cause. I'm not fully sure how to fix them but here's what I figured out.

• Annotable in ibis/common/grounds.py is decorated with dataclass_transform
• Standard dataclasses can't have arguments without defaults in subclasses if the parent class had arguments with default values, see here. I don't think this restriction applies for dataclass_transform but it still leads to this error as soon as the class inheritance has more then 1 level:

from typing_extensions import dataclass_transform


@dataclass_transform()
class Foo:
    a: int
    b: str = "default"


class Bar(Foo):
    c: str
    d: int = 0


class Baz(Bar):
    e: int
    f: str = "default"

Gives an error on Baz but not on Bar:

mypy_errors.py:17: error: Attributes without a default cannot follow attributes with one  [misc]

A solution could be to use kw_only_default argument (@dataclass_transform(kw_only_default=True) in the example above), see python/mypy#14629. As far as I understand Annotable it cannot be used with positional arguments anyway?

[kszucs]

It can be used with both positional
And keyword arguments, mixing them is also supported. It is more flexible than dataclasses in that regard, see

ibis/ibis/common/annotations.py

Line 199 in 90e852d

def merge(cls, *signatures, **annotations):

for details.

[gforsyth]

I just ran mypy ibis --ignore-missing-imports and got Found 2746 errors in 253 files (checked 603 source files)

I don't think any of the maintainers is up for the herculean effort of fixing all of those. My inclination is to remove the py.typed file.

Thoughts?

[cpcloud]

@tswast I think you added this. Can you recall why it was added?

[binste]

I just ran mypy ibis --ignore-missing-imports and got Found 2746 errors in 253 files (checked 603 source files)

I don't think any of the maintainers is up for the herculean effort of fixing all of those. My inclination is to remove the py.typed file.

Thoughts?

Sounds reasonable to me. We started using Ibis extensively in our code base but as we use mypy, we have to use quite a few type: ignore[... statements right now.

[tswast]

We also use mypy in BigQuery DataFrames, which is why I added this in the first place. We do have a good handful of typing.cast calls, but overall I find the benefits are outweighing the limitations.

[tswast]

I think this gradual plan makes sense to me. #6844 (comment)

In the places where the type hints are wrong, we should either remove them or fix them.

[tswast]

A tad more context, I'd like to be able to use type hints so that VS Code (Pylance) autocomplete has some chance of being helpful in my functions that take and return Ibis expressions. For this purpose it has been helpful, even though the types aren't perfect.

[binste]

A tad more context, I'd like to be able to use type hints so that VS Code (Pylance) autocomplete has some chance of being helpful in my functions that take and return Ibis expressions. For this purpose it has been helpful, even though the types aren't perfect.

Indeed, I also find this very useful in VS Code! Luckily, Pylance uses type hints for autocompletion even if no py.typed file is present for a library. (We see this for Vega-Altair, where Pylance uses the hints for autocompletion)

[gforsyth]

Thanks for the context all.

In my reading of the above, I don't see a benefit to having the py.typed file in the repo (since pylance doesn't need it) and there are distinct downsides. Am I missing something?

[cpcloud]

If someone would like to push this forward, please submit incremental pull requests. At the moment we're not going to take on the task of making our type hints complete.

Please open up bug reports for incorrect annotations as they arise!