Skip to content

tensorflow: Add (and rename) aliases #11324

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 31, 2024
Merged

tensorflow: Add (and rename) aliases #11324

merged 7 commits into from
Jan 31, 2024

Conversation

hoel-bagard
Copy link
Contributor

@hoel-bagard hoel-bagard commented Jan 27, 2024 •
edited
Loading

This PR adds aliases (taken from here) so that they can be used in the future. It also renames aliases that do not have a runtime equivalent (by adding an _).

@hmc-cs-mdrissi The previous PR keeps getting bigger because tf.keras.Model relies on many other types, so I will make smaller PRs adding those types before adding tf.keras.Model.

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_aliases branch 3 times, most recently from a9395ed to deb8ec7 Compare January 27, 2024 09:02
@hoel-bagard hoel-bagard marked this pull request as draft January 27, 2024 09:03
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@hoel-bagard hoel-bagard marked this pull request as ready for review January 27, 2024 09:53
@hoel-bagard
Copy link
Contributor Author

@hmc-cs-mdrissi Should the type aliases declared in _aliases.pyi not start with an underscore since they are already in a private module ? It made more sense to me to have all the type aliases that do not have a runtime equivalent start with an _, but it seems that flake8 does not agree 🤔

@hoel-bagard hoel-bagard changed the title Add (and rename) TF2 aliases tensorflow: Add (and rename) aliases Jan 27, 2024
@hmc-cs-mdrissi
Copy link
Contributor

Yup. The convention is anything in _aliases.py is type checking only and has no equivalent at runtime. Partly motivated by that flake8 rule so no need to have underscore start in alias name.

hmc-cs-mdrissi
hmc-cs-mdrissi reviewed Jan 27, 2024
View reviewed changes
stubs/tensorflow/tensorflow/_aliases.pyi Outdated

class _KerasSerializable1(Protocol):
def get_config(self) -> dict[str, Any]: ...

class _KerasSerializable2(Protocol):
__name__: str

KerasSerializable: TypeAlias = _KerasSerializable1 | _KerasSerializable2
_KerasSerializable: TypeAlias = _KerasSerializable1 | _KerasSerializable2
Copy link
Contributor

Choose a reason for hiding this comment

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

I think since you've unified aliases to all live here consistently they can all lose underscore prefix.

Except for that the PR looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c7d65fd

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_aliases branch 3 times, most recently from ce8b30e to f22350f Compare January 27, 2024 11:34
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@JelleZijlstra
Copy link
Member

This looks good but I suspect it conflicts with some of the other PRs I just merged, so I updated the branch and might push some changes to the branch to get it to pass.

@JelleZijlstra JelleZijlstra merged commit 67df2df into python:main Jan 31, 2024
@github-actions GitHub Actions
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmc-cs-mdrissi hmc-cs-mdrissi hmc-cs-mdrissi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hoel-bagard @hmc-cs-mdrissi @JelleZijlstra