Tensorflow keras layer #9707
Tensorflow keras layer #9707
Conversation
| # tf.initializers at runtime is <module 'keras.api._v2.keras.initializers' from '...'> | ||
| tensorflow.initializers | ||
|
|
||
| # Layer constructor's always have **kwargs, but only allow a few specific values. PEP 692 |
There was a problem hiding this comment.
I don't think we have PEP 692 tracker ticket yet. Will make one later if no one covers it first.
There was a problem hiding this comment.
I think PEP 692 might already have sufficient support to use it; at least mypy and pyright support it. Haven't checked pytype though.
There was a problem hiding this comment.
Testing basic 692 support:
class Foo(TypedDict, total=False):
a: int
b: str
def f(**kwargs: Unpack[Foo]) -> None:
...
f(a=1, b="2") # OK
f(a=1, b=2) # Error: b has type str
f(a=1, b="2", c=3) # Error: unexpected keyword argument "c"- pyright works fine
- mypy works if we add --enable-incomplete-feature=Unpack (otherwise it gives
error: "Unpack" support is experimental, use --enable-incomplete-feature=Unpack to enable) - pytype produces
typing_extensions.Unpack not supported yet [not-supported-yet]. No errors otherwise. - pyre is same as pytype and just produces
pep_692.py:11:16 Undefined or invalid type [11]: AnnotationUnpackis not defined as a type.
I'll open ticket to track.
| # tf.initializers as module and has that type if accessed the second way, | ||
| # but the real module file is completely different name (even package) and dynamically handled. | ||
| # tf.initializers at runtime is <module 'keras.api._v2.keras.initializers' from '...'> | ||
| tensorflow.initializers |
There was a problem hiding this comment.
This is probably most cursed weirdness of imports (pylint is unhappy too with this). A lot of other tf modules do similar thing and this part of allowlist will grow over time.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hmm optuna hit is valid, but input_dim argument is undocumented at least on layer documentation. input_dim is one of the internal arguments here. Same issue for streamlit error. input_shape argument is also not documented anywhere I can find. There's 6 more internal layer arguments that could also be added. I weakly prefer not to include arguments that do not appear in documentation, but I'm fine adding them in if desired. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
General review poke. There are a few small questions I’ve left but otherwise been ready for over week. |
| @overload | ||
| def get(identifier: str | dict[str, Any]) -> Constraint: ... | ||
| @overload | ||
| def get(identifier: _Constraint) -> Constraint | Callable[[Tensor], Tensor] | None: ... |
There was a problem hiding this comment.
Using the type alias seems unnecessary here as all types but Constraint are already covered by previous overloads.
There was a problem hiding this comment.
Fixed and made overloads non overlapping.
One question for this and similar comments is it recommended to make these generic? As,
from tensorflow.initializers import Zeros
get(Zeros()) # returns same type Zeros().I feel neutral as most common overload I think is str/dict[str, Any] case but otherwise constraint case always returns same type.
| @overload | ||
| def get(identifier: str | Initializer | dict[str, Any]) -> Initializer: ... | ||
| @overload | ||
| def get(identifier: _Initializer) -> Initializer | None: ... |
There was a problem hiding this comment.
Same as before, seems like this is only the Callable.
There was a problem hiding this comment.
Fixed. For this one re-reading source I missed one case (type[Initializer] is also allowed).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I think I've addressed all comments now. |
|
Diff from mypy_primer, showing the effect of this PR on open source code: optuna (https://github.com/optuna/optuna)
+ tests/integration_tests/test_tfkeras.py:20: error: Unexpected keyword argument "input_dim" for "Dense" [call-arg]
streamlit (https://github.com/streamlit/streamlit)
+ lib/tests/streamlit/runtime/legacy_caching/hashing_test.py: note: In member "test_tf_saved_model" of class "HashTest":
+ lib/tests/streamlit/runtime/legacy_caching/hashing_test.py:417:17: error: Unexpected keyword argument "input_shape" for "Dense" [call-arg]
- lib/tests/streamlit/runtime/legacy_caching/hashing_test.py: note: At top level:
|
Summary
Next tensorflow stub pr. Primary goal of this one was to define a reasonable initial stub for Layers, one of the most common types user interacts with. The other files are mainly types needed to cover Layer's arguments. I've included a couple layer's too. Adding a stub for 1 layer is fairly easy as most of the time only thing needed is constructor and all public methods come from parent Layer class.
I ended up adding a fair amount of stubtest allowlist and tried to give an explanation for each.
call/__call__strangeness is probably biggest one and I can explain more if it's unclear. At it's core the magic is as a user, you definedef call(self, inputs): ...but you are never supposed to call that method and instead use
__call__which does a good amount of signature introspection/argument adjustment for you.