Add type stubs for pylibcudf #17258
Add type stubs for pylibcudf #17258
Conversation
github-actions
bot
added
Python
| class gpumemoryview: | ||
| def __init__(self, data: Any) -> None: ... | ||
| @property | ||
| def __cuda_array_interface__(self) -> Mapping[str, Any]: ... |
There was a problem hiding this comment.
Could use TypedDict here if we really care about advertising the key names.
There was a problem hiding this comment.
I am agnostic to this and will defer to you on whether you think the extra work is worthwhile.
| class ColumnMetadata: | ||
| name: str | ||
| children_meta: list[ColumnMetadata] |
| class OrcColumnStatistics: | ||
| @property | ||
| def number_of_values(self) -> int | None: ... | ||
| @property | ||
| def has_null(self) -> bool | None: ... | ||
| def __getitem__(self, item: str) -> Any: ... | ||
| def __contains__(self, item: str) -> bool: ... | ||
| def get[T](self, item: str, default: None | T = None) -> T | None: ... | ||
|
|
||
| class ParsedOrcStatistics: |
There was a problem hiding this comment.
We can't construct these directly, so no __init__ annotation.
There was a problem hiding this comment.
Looks like you added these later anyway? Why?
|
|
||
| class SourceInfo: | ||
| def __init__( | ||
| self, sources: list[str] | list[os.PathLike] | list[Datasource] |
There was a problem hiding this comment.
This is a lie, it's actually list[str | os.PathLike], but list is invariant, so the type checkers complain. If we had Sequence[str | os.PathLike] that would be fine, because Sequence is contravariant, but these functions really do want lists, and will complain for some Sequence that is not a list.
wence-
force-pushed
the
wence/fea/pylibcudf-typing
branch
2 times, most recently
from
ae4421a
to
111ff09
Compare
wence-
force-pushed
the
wence/fea/pylibcudf-typing
branch
from
111ff09
to
1801379
Compare
There was a problem hiding this comment.
Overall looks great. I left some inline comments while I skimmed the whole thing under the assumption that I would probably spot some issues in some files while missing them in others. The big points that might apply to many places are:
- Can we remove default parameter values from stub files?
- Can we get rid of enum values in stub files (I expect no, but it would be nice)?
- We should try and make absolute vs relative import styles consistent, but I'm OK with doing that in a follow-up to avoid cluttering this PR further.
| def product() -> Aggregation: ... | ||
| def min() -> Aggregation: ... | ||
| def max() -> Aggregation: ... | ||
| def count(null_handling: NullPolicy = NullPolicy.INCLUDE) -> Aggregation: ... |
There was a problem hiding this comment.
Can we elide default parameters? They don't add anything for typing, and that is mypy's recommendation:
Stub files are written in normal Python syntax, but generally leaving out runtime logic like variable initializers, function bodies, and default arguments.
It might also simplify the requirements for the automation scripts.
There was a problem hiding this comment.
We can elide default values by doing def count(null_handling: NullPolicy = ...).
However, type stubs are not just for type checking, but also LSP. In the latter case, having the default value encoded is useful since I can immediately see from the signature what behaviour I get if I don't provide the argument.
I will see if I can automatically put the right value in here.
| class gpumemoryview: | ||
| def __init__(self, data: Any) -> None: ... | ||
| @property | ||
| def __cuda_array_interface__(self) -> Mapping[str, Any]: ... |
There was a problem hiding this comment.
I am agnostic to this and will defer to you on whether you think the extra work is worthwhile.
| from pylibcudf.scalar import Scalar | ||
|
|
||
| class BPEMergePairs: | ||
| def __init__(self, merge_pairs: Column) -> None: ... |
There was a problem hiding this comment.
Is there a way to default the return type annotation for __init__ (and perhaps other special methods if they make sense)? It's redundant otherwise.
There was a problem hiding this comment.
No, I don't think so: pyright complains in the untyped circumstance.
There was a problem hiding this comment.
Actually, scratch that, yes, it's ok: https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md
vyasr
added
feature request
Description
Having looked at a bunch of the automation options, I just did it by hand.
A followup will add some automation to add docstrings (so we can see those via LSP integration in editors) and do some simple validation.
Checklist