Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tensorflow: Add missing members to the tensorflow.keras.layers module. #11333

Merged
merged 27 commits into from
Mar 13, 2024

Conversation

hoel-bagard
Copy link
Contributor

Some of the added stubs are taken from here.

This comment has been minimized.

1 similar comment

This comment has been minimized.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_layers branch from 9e8a90c to 7e2db33 Compare January 28, 2024 09:41

This comment has been minimized.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_layers branch 2 times, most recently from 08c3a39 to 52e0d83 Compare January 28, 2024 09:51

This comment has been minimized.

1 similar comment

This comment has been minimized.

@hoel-bagard
Copy link
Contributor Author

hoel-bagard commented Jan 28, 2024

@hmc-cs-mdrissi I've tried to add the MultiHeadAttention's __call__ to the stubtest_allowlist.txt, but it is still detected as an error. Did I misunderstand how to add functions to the stubtest allowlist ?

Edit: after spending a bunch of time trying to make CI pass without success, I've removed MultiHeadAttention from the PR.

@JelleZijlstra
Copy link
Member

FYI this has a merge conflict now.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_layers branch from c226323 to 9cd6010 Compare January 31, 2024 09:53

This comment has been minimized.

[pre-commit.ci] auto fixes from pre-commit.com hooks

Move some layers to keras.layers.preprocessing

fix: fix imports
@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_layers branch from 5ee5f57 to a05968f Compare January 31, 2024 10:46

This comment has been minimized.

1 similar comment

This comment has been minimized.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_layers branch 2 times, most recently from a05968f to 35e3b72 Compare January 31, 2024 13:28

This comment has been minimized.

1 similar comment

This comment has been minimized.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_layers branch 6 times, most recently from f9685a2 to b17c74a Compare January 31, 2024 14:03
attempt to fix MultiHeadAttention's __call__
@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_layers branch from b17c74a to 35056a7 Compare January 31, 2024 14:05
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@hoel-bagard
Copy link
Contributor Author

@JelleZijlstra It seems like pytype is causing CI to fail again. Is it happening in other PRs ?

@JelleZijlstra
Copy link
Member

This failure looks different than before. Presumably it's triggered by commit 37c668b, since the one before that is green. However, the failures on that commit (https://github.com/python/typeshed/actions/runs/7941246262/job/21683390603) also look different from those on the most recent commit (https://github.com/python/typeshed/actions/runs/7941516630/job/21683958032?pr=11333). @rchen152 I'm afraid I have to ping you again.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@hoel-bagard
Copy link
Contributor Author

@rchen152
Copy link
Collaborator

This failure looks different than before. Presumably it's triggered by commit 37c668b, since the one before that is green. However, the failures on that commit (https://github.com/python/typeshed/actions/runs/7941246262/job/21683390603) also look different from those on the most recent commit (https://github.com/python/typeshed/actions/runs/7941516630/job/21683958032?pr=11333). @rchen152 I'm afraid I have to ping you again.

Looking. Sorry about all the weird pytype errors!

@rchen152
Copy link
Collaborator

I have a pytype fix out for review, but I'm out the rest of this week; I'll try to do a release first thing on Monday.

rchen152 added a commit to google/pytype that referenced this pull request Feb 27, 2024
Fixes a bug that affects typeshed's pytype_test. See
python/typeshed#11333 for context. I wasn't able to
write a test for this because it's triggered by some complicated combination of
mutually dependent pyi files and mutually recursive aliases.

PiperOrigin-RevId: 609414132
@rchen152
Copy link
Collaborator

Ok, I think pytype is finally happy.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra
Copy link
Member

But now there is a merge conflict. If you fix that, I can merge.

@hoel-bagard
Copy link
Contributor Author

@JelleZijlstra @rchen152 I merged main into the branch, but this caused pytype to fail again.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra
Copy link
Member

Traceback (most recent call last):
  File "/home/runner/work/typeshed/typeshed/./tests/pytype_test.py", line 88, in run_pytype
    loader.finish_and_verify_ast(ast)
  File "/opt/hostedtoolcache/Python/3.11.8/x64/lib/python3.11/site-packages/pytype/load_pytd.py", line 641, in finish_and_verify_ast
    self._resolver.verify(mod_ast)
  File "/opt/hostedtoolcache/Python/3.11.8/x64/lib/python3.11/site-packages/pytype/load_pytd.py", line 314, in verify
    raise BadDependencyError(str(e), name) from e
pytype.load_pytd.BadDependencyError: Unreplaced NamedType: 'tf.DType', referenced from 'tensorflow'

@hoel-bagard
Copy link
Contributor Author

@JelleZijlstra Is it because there's something wrong with the PR ?

The PR doesn't use tensorflow.DType, and CI was passing before merging with main, so I don't know what's the cause of it failing now. I tried to look up what the error means in the pytype docs but couldn't find it.

Looking at tensorflow/__init__.pyi, there is a comment about DType causing a crash in pytype in the past. Could this be related ?

# Explicit import of DType is covered by the wildcard, but
# is necessary to avoid a crash in pytype.
from tensorflow.dtypes import *
from tensorflow.dtypes import DType as DType

@JelleZijlstra
Copy link
Member

Pytype errors can be somewhat opaque. I wouldn't be surprised if this PR affects DType in some indirect way. To check this further I tried to merge main into some of your other open PRs about tensorflow, but seems like GitHub is having an outage at the moment.

@rchen152
Copy link
Collaborator

Try doing this (remove the extra DType import in __init__.pyi, import numpy and DType differently in _aliases.pyi for the definition of DTypeLike):

diff --git a/stubs/tensorflow/tensorflow/__init__.pyi b/stubs/tensorflow/tensorflow/__init__.pyi
index 308690fe7..db10b0756 100644
--- a/stubs/tensorflow/tensorflow/__init__.pyi
+++ b/stubs/tensorflow/tensorflow/__init__.pyi
@@ -22,10 +22,7 @@ from tensorflow._aliases import AnyArray, DTypeLike, ShapeLike, Slice, TensorCom
 from tensorflow.autodiff import GradientTape as GradientTape
 from tensorflow.core.protobuf import struct_pb2
 
-# Explicit import of DType is covered by the wildcard, but
-# is necessary to avoid a crash in pytype.
 from tensorflow.dtypes import *
-from tensorflow.dtypes import DType as DType
 from tensorflow.experimental.dtensor import Layout
 from tensorflow.keras import losses as losses
 from tensorflow.linalg import eye as eye
diff --git a/stubs/tensorflow/tensorflow/_aliases.pyi b/stubs/tensorflow/tensorflow/_aliases.pyi
index 5566b8069..e26c80880 100644
--- a/stubs/tensorflow/tensorflow/_aliases.pyi
+++ b/stubs/tensorflow/tensorflow/_aliases.pyi
@@ -5,9 +5,11 @@ from collections.abc import Iterable, Mapping, Sequence
 from typing import Any, Protocol, TypeVar
 from typing_extensions import TypeAlias
 
+import numpy  # pytype needs the unaliased import to resolve DTypeLike
 import numpy as np
 import numpy.typing as npt
 import tensorflow as tf
+from tensorflow.dtypes import DType
 from tensorflow.keras.layers import InputSpec
 
 _T = TypeVar("_T")
@@ -52,7 +54,7 @@ SparseTensorCompatible: TypeAlias = TensorCompatible | tf.SparseTensor
 TensorOrArray: TypeAlias = tf.Tensor | AnyArray
 
 ShapeLike: TypeAlias = tf.TensorShape | Iterable[ScalarTensorCompatible | None] | int | tf.Tensor
-DTypeLike: TypeAlias = tf.DType | str | np.dtype[Any] | int
+DTypeLike: TypeAlias = DType | str | numpy.dtype[Any] | int
 
 ContainerTensors: TypeAlias = ContainerGeneric[tf.Tensor]
 ContainerTensorsLike: TypeAlias = ContainerGeneric[TensorLike]

Right now, pytype handles circular dependencies among pyi files in a rather ugly way, by just resolving names multiple times until everything is sorted. So tweaking the dependency structure can help with name resolution issues.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra merged commit 6c52322 into python:main Mar 13, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants