Skip to content

chore: Reuse SeriesT alias more, make more types TYPE_CHECKING-only#2545

Merged
MarcoGorelli merged 1 commit intonarwhals-dev:mainfrom
MarcoGorelli:reuse-typing
May 13, 2025
Merged

chore: Reuse SeriesT alias more, make more types TYPE_CHECKING-only#2545
MarcoGorelli merged 1 commit intonarwhals-dev:mainfrom
MarcoGorelli:reuse-typing

Conversation

@MarcoGorelli
Copy link
Member

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

  • Related issue #<issue number>
  • Closes #<issue number>

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

@MarcoGorelli MarcoGorelli marked this pull request as ready for review May 13, 2025 08:48
@dangotbanned
Copy link
Member

dangotbanned commented May 13, 2025

@MarcoGorelli some notes, not sure if you're aware of these:

  • Trailing docstrings don't work within a TYPE_CHECKING block in an editor
    • I'm not sure if they work in the API reference
    • but I could see that being an issue if they're not runtime objects 🤔
  • For the Literal aliases, if they're runtime objects we could use get_args for error messages
  • Protocol defs should be fine as long as we aren't inheriting from them
    • SupportsNativeNamespace, and DTypes all good 👍
  • Any aliases that are used to parameterize a protocol/generic/another alias at runtime have some quirks

IMO, I think what you're trying to do here is already covered by omitting these types from nw.typing.__all__

@dangotbanned
Copy link
Member

If you wanna have some super-secret typing - another option is defining them at runtime in a private module, but importing into nw.typing in a TYPE_CHECKING block?

@MarcoGorelli
Copy link
Member Author

MarcoGorelli commented May 13, 2025

thanks for your review - i checked the api reference and all looks good 👍

if we need to use get_args, i'd say that when we need to use that we take them out of TYPE_CHECKING

Any aliases that are used to parameterize a protocol/generic/another alias at runtime have some quirks

could you elaborate on this please?

@MarcoGorelli
Copy link
Member Author

i'll take some of these out and leave them for a separate discussion

@dangotbanned
Copy link
Member

dangotbanned commented May 13, 2025

thanks for your review - i checked the api reference and all looks good 👍

if we need to use get_args, i'd say that when we need to use that we take them out of TYPE_CHECKING

Thanks @MarcoGorelli 🎉

Any aliases that are used to parameterize a protocol/generic/another alias at runtime have some quirks

could you elaborate on this please?

So I think something like this works across all versions of python

from __future__ import annotations

from typing import TYPE_CHECKING
from typing import Generic
from typing import TypeVar

if TYPE_CHECKING:
    from typing_extensions import Self

    # Where these are runtime objects in `typing`
    from narwhals.typing import _1DArray
    from narwhals.typing import _NumpyScalar

NumpyT = TypeVar("NumpyT", "_1DArray", "_NumpyScalar")


class Some(Generic[NumpyT]):
    def __init__(self) -> None:
        super().__init__()

    def meth(self) -> Self:
        return self


class Another(Some["_1DArray"]): ...

But changing that to this, caused me some issues in earlier versions

from __future__ import annotations

from typing import TYPE_CHECKING
from typing import Generic
from typing import TypeVar

if TYPE_CHECKING:
    import numpy as np
    from typing_extensions import Self

    _1DArray: TypeAlias = "np.ndarray[tuple[int], Any]"  # noqa: PYI042
    _NumpyScalar: TypeAlias = "np.generic[Any]"

NumpyT = TypeVar("NumpyT", "_1DArray", "_NumpyScalar")


class Some(Generic[NumpyT]):
    def __init__(self) -> None:
        super().__init__()

    def meth(self) -> Self:
        return self


class Another(Some["_1DArray"]): ...

Seems to be working fine on 3.13 though.

I've just gone back over (#2064) and each of (https://github.com/narwhals-dev/narwhals/commits/main/narwhals/_translate.py) and I can't find the issue that popped up in CI.
Maybe I'm imagining things? 😅

Edit

Found it!

This is what you can't do

from __future__ import annotations

from typing import TYPE_CHECKING
from typing import Generic
from typing import TypeVar

if TYPE_CHECKING:
    import numpy as np

    _1DArray: TypeAlias = "np.ndarray[tuple[int], Any]"  # noqa: PYI042
    _NumpyScalar: TypeAlias = "np.generic[Any]"

    NumpyT = TypeVar("NumpyT", "_1DArray", "_NumpyScalar")


class Some(Generic["NumpyT"]):
    def __init__(self) -> None:
        super().__init__()

At runtime it causes:

TypeError                                 Traceback (most recent call last)
Cell In[2], line 15
     10     _NumpyScalar: TypeAlias = "np.generic[Any]"
     12     NumpyT = TypeVar("NumpyT", "_1DArray", "_NumpyScalar")
---> 15 class Some(Generic["NumpyT"]):
     16     def __init__(self) -> None:
     17         super().__init__()

File /Lib\/typing.py:432, in _tp_cache.<locals>.decorator.<locals>.inner(*args, **kwds)
    430 except TypeError:
    431     pass  # All real errors (not unhashable args) are raised below.
--> 432 return func(*args, **kwds)

File /Lib/typing.py:1231, in _generic_class_getitem(cls, args)
   1227     raise TypeError(
   1228         f"Parameter list to {cls.__qualname__}[...] cannot be empty"
   1229     )
   1230 if not all(_is_typevar_like(p) for p in args):
-> 1231     raise TypeError(
   1232         f"Parameters to {cls.__name__}[...] must all be type variables "
   1233         f"or parameter specification variables.")
   1234 if len(set(args)) != len(args):
   1235     raise TypeError(
   1236         f"Parameters to {cls.__name__}[...] must all be unique")

TypeError: Parameters to Generic[...] must all be type variables or parameter specification variables.

Copy link
Member

@dangotbanned dangotbanned left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MarcoGorelli, all looking good in VSCode now 😍

@MarcoGorelli
Copy link
Member Author

thanks for explaining 🙏 cheers @dangotbanned !

@MarcoGorelli MarcoGorelli merged commit dea76e5 into narwhals-dev:main May 13, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants