Skip to content

tensorflow: Add members from tensorflow.keras.metrics#11329

Merged
JelleZijlstra merged 15 commits intopython:mainfrom
hoel-bagard:hoel/add_tf_keras_metrics
Feb 17, 2024
Merged

tensorflow: Add members from tensorflow.keras.metrics#11329
JelleZijlstra merged 15 commits intopython:mainfrom
hoel-bagard:hoel/add_tf_keras_metrics

Conversation

@hoel-bagard
Copy link
Copy Markdown
Contributor

@github-actions

This comment has been minimized.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_metrics branch 3 times, most recently from 66f37c0 to a8d82cc Compare January 27, 2024 13:55
@github-actions

This comment has been minimized.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_metrics branch 2 times, most recently from 61faa9e to efbb44a Compare January 27, 2024 14:47
@github-actions

This comment has been minimized.

2 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_metrics branch 3 times, most recently from 7788199 to 4ef4051 Compare January 27, 2024 16:00
@github-actions

This comment has been minimized.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_metrics branch 3 times, most recently from 08ec9a5 to 155f910 Compare January 27, 2024 16:13
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@hoel-bagard hoel-bagard marked this pull request as ready for review January 28, 2024 03:52
@hoel-bagard
Copy link
Copy Markdown
Contributor Author

@hmc-cs-mdrissi If you have time to review it.

Taken from: https://github.com/hmc-cs-mdrissi/tensorflow_stubs/blob/main/stubs/tensorflow/keras/metrics.pyi

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

fix CI errors

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

fix CI errors

TensorCompatible -> _TensorCompatible

fix CI errors

fix CI errors

fix CI errors

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

fix CI errors

fix CI errors

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

fix CI errors

[pre-commit.ci] auto fixes from pre-commit.com hooks
[pre-commit.ci] auto fixes from pre-commit.com hooks
@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_metrics branch from a7c6586 to 1c66747 Compare January 28, 2024 03:53
@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Copy Markdown
Member

FYI this has a merge conflict now.

@github-actions

This comment has been minimized.

@hoel-bagard
Copy link
Copy Markdown
Contributor Author

@JelleZijlstra The CI was passing before merging with main, but now there is an error with the imports that I do not understand. Would you have an idea of what is causing it ?

@JelleZijlstra
Copy link
Copy Markdown
Member

There was a failing stubtest job with no output; I reran it.

There is also a pytype crash:

  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/site-packages/pytype/pytd/visitors.py", line 512, in VisitNamedType
    raise KeyError(f"No {name} in module {module_name}") from e
KeyError: 'No ABCMeta in module importlib.resources.abc'

The above exception was the direct cause of the following exception:

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.7/x64/lib/python3.11/site-packages/pytype/load_pytd.py", line 632, in finish_and_verify_ast
    self._modules[k].ast = self._resolve_external_types(
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/site-packages/pytype/load_pytd.py", line 526, in _resolve_external_types
    mod_ast = self._resolver.resolve_external_types(
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/site-packages/pytype/load_pytd.py", line 283, in resolve_external_types
    raise BadDependencyError(key, name) from e
pytype.load_pytd.BadDependencyError: No ABCMeta in module importlib.resources.abc, referenced from 'tensorflow.keras.metrics'

Which looks more mysterious, since I don't see how this PR relates to importlib.resources. cc @rchen152

Comment thread stubs/tensorflow/tensorflow/keras/metrics.pyi Outdated
Comment thread stubs/tensorflow/tensorflow/keras/metrics.pyi Outdated
Comment thread stubs/tensorflow/tensorflow/keras/metrics.pyi Outdated
def from_config(cls, config: dict[str, Any]) -> Self: ...
def get_config(self) -> dict[str, Any]: ...
@override
def add_weight(
Copy link
Copy Markdown
Contributor

@hmc-cs-mdrissi hmc-cs-mdrissi Jan 31, 2024

Choose a reason for hiding this comment

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

This also looks like an inherited method so metrics class shouldn't have it defined. Add it to Layer class if it's not already there.

Copy link
Copy Markdown
Contributor Author

@hoel-bagard hoel-bagard Feb 1, 2024

Choose a reason for hiding this comment

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

Done in e9246aa. It was already inherited from Layer indeed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@hmc-cs-mdrissi The add_weight from keras.metrics.Metric is different from the one from keras.layers.Layer, so I need to redefine it. However I'm facing the same error I had in another PR with MultiHeadAttention. Even if I add the method to the stubtest_allowlist, mypy still reports an error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

# type: ignore works for that. stubtest allowlist is for runtime vs stub checks. Before that happens there's also a mypy pass. And you can use normal # type: ignore to handle that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally get mypy passing first. No need to consider allowlist and then after mypy is green work on stubtest errors, and use allowlist when appropriate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks.

Comment thread stubs/tensorflow/tensorflow/keras/metrics.pyi Outdated
@github-actions

This comment has been minimized.

@rchen152
Copy link
Copy Markdown
Collaborator

rchen152 commented Feb 1, 2024

There was a failing stubtest job with no output; I reran it.

There is also a pytype crash:

  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/site-packages/pytype/pytd/visitors.py", line 512, in VisitNamedType
    raise KeyError(f"No {name} in module {module_name}") from e
KeyError: 'No ABCMeta in module importlib.resources.abc'

The above exception was the direct cause of the following exception:

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.7/x64/lib/python3.11/site-packages/pytype/load_pytd.py", line 632, in finish_and_verify_ast
    self._modules[k].ast = self._resolve_external_types(
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/site-packages/pytype/load_pytd.py", line 526, in _resolve_external_types
    mod_ast = self._resolver.resolve_external_types(
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/site-packages/pytype/load_pytd.py", line 283, in resolve_external_types
    raise BadDependencyError(key, name) from e
pytype.load_pytd.BadDependencyError: No ABCMeta in module importlib.resources.abc, referenced from 'tensorflow.keras.metrics'

Which looks more mysterious, since I don't see how this PR relates to importlib.resources. cc @rchen152

Yeah, this was a weird one! Turns out it was an issue with how pytype resolves module aliases. I have a fix out for review, but I might not be able to release a new version of pytype until sometime next week, as we have a few big changes in the pipeline.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_metrics branch from f53ff92 to 5e28825 Compare February 1, 2024 03:02
@github-actions

This comment has been minimized.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_keras_metrics branch from 5e28825 to 8db4a5d Compare February 1, 2024 03:29
@github-actions

This comment has been minimized.

rchen152 added a commit to google/pytype that referenced this pull request Feb 1, 2024
See python/typeshed#11329 for a case in which the old
approach failed.

Hypothetically, this new approach could get quite expensive, with
LookupExternalTypes called over and over, but I ran typeshed's pytype_test a
few times with and without the change and noticed no difference in runtime.

PiperOrigin-RevId: 603435155
@github-actions

This comment has been minimized.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 8, 2024

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

@rchen152
Copy link
Copy Markdown
Collaborator

Yeah, this was a weird one! Turns out it was an issue with how pytype resolves module aliases. I have a fix out for review, but I might not be able to release a new version of pytype until sometime next week, as we have a few big changes in the pipeline.

Sorry, this took longer than expected, but pytype-2024.02.09 is out with this fix.

@hoel-bagard
Copy link
Copy Markdown
Contributor Author

Thank you!

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