Allow generic type hinting for Tuple space#1537
Allow generic type hinting for Tuple space#1537Coolgolf1 wants to merge 2 commits intoFarama-Foundation:mainfrom
Conversation
|
This is a cool PR, thanks @Coolgolf1 Therefore, I would prefer for us to solve the general type hinting before merging this PR. |
|
Thanks @pseudo-rnd-thoughts, that makes sense! If you're open to improving the type hinting, I'd be happy to help and contribute. I'm new to open source, and I'd love to participate or help in any way I can. |
jorenham
left a comment
jorenham
left a comment
There was a problem hiding this comment.
Hi there,
I took a look and left some suggestions. Hope you don't mind :)
|
|
||
| class Tuple(Space[tuple[Any, ...]], typing.Sequence[Any]): | ||
|
|
||
| _TSpaces = TypeVarTuple("_TSpaces") |
There was a problem hiding this comment.
nitpick: The (informal) convention for type variable names is to add T as suffix, so _SpacesT.
| try: | ||
| # Python 3.11+ | ||
| from typing import TypeVarTuple, Unpack | ||
| except ImportError: # pragma: no cover | ||
| # For older Python versions supported by Gymnasium | ||
| from typing_extensions import TypeVarTuple, Unpack |
There was a problem hiding this comment.
Static type-checkers typically (pun intended) don't understand try: blocks like this one. They do, however, understand if sys.version_info >= (3, 11): guards.
| # Help type-checkers understand that Tuple[Box, Discrete].spaces is (Box, Discrete) | ||
| if TYPE_CHECKING: | ||
| spaces: tuple[Unpack[_TSpaces]] | ||
| else: | ||
| spaces: tuple[Space[Any], ...] |
There was a problem hiding this comment.
I don't think this is needed, because Unpack is imported, so it should be available at runtime.
|
|
||
| @overload | ||
| def __init__( | ||
| self, | ||
| spaces: Iterable[Space[Any]], | ||
| seed: int | typing.Sequence[int] | np.random.Generator | None = None, | ||
| ): ... | ||
|
|
There was a problem hiding this comment.
nitpick: The convention is to have no newlines between overloads of the same function.
| @overload | |
| def __init__( | |
| self, | |
| spaces: Iterable[Space[Any]], | |
| seed: int | typing.Sequence[int] | np.random.Generator | None = None, | |
| ): ... | |
| @overload | |
| def __init__( | |
| self, | |
| spaces: Iterable[Space[Any]], | |
| seed: int | typing.Sequence[int] | np.random.Generator | None = None, | |
| ): ... |
| # Promote list and ndarray to tuple for contains check | ||
| x = tuple(x) |
There was a problem hiding this comment.
this change seems irrelevant?
| try: | ||
| from typing import assert_type # type: ignore[attr-defined] | ||
| except ImportError: # pragma: no cover | ||
| # type: ignore[import-not-found] | ||
| from typing_extensions import assert_type |
There was a problem hiding this comment.
In a TYPE_CHECKING block you can always import typing_extensions; even if it's not installed. So there's no need for this try: block (or any of the type ignores for that matter).
| if TYPE_CHECKING: | ||
| assert_type(obs_space.spaces, tuple[MultiDiscrete, Box]) |
There was a problem hiding this comment.
Like you said, this is a no-op at runtime. So typing.assert_type would be more appropriate here (i.e. without the TYPE_CHECKING guard)
| space = gym.spaces.Tuple( | ||
| (gym.spaces.Box(0, 1), gym.spaces.Box(-1, 0, (2,)))) |
There was a problem hiding this comment.
this change seems unnecessary
| space = gym.spaces.Tuple( | ||
| (gym.spaces.Box(0, 1), gym.spaces.Box(-1, 0, (1,)))) |
There was a problem hiding this comment.
this change seems unnecessary
3 participants
Description
This PR enables developers to specify the "sub-types" of a
Tuplespace using standard type hinting. Example:observation_space: spaces.Tuple[spaces.MultiDiscrete, spaces.Box].Fixes #1476
Type of change
Checklist:
pre-commitchecks withpre-commit run --all-files(seeCONTRIBUTING.mdinstructions to set it up)