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

📝 Add typing to some callback classes #16692

Conversation

gabrieldemarmiesse
Copy link
Contributor

Provides an exemple to keras-team/tf-keras#555

@gbaned gbaned requested a review from fchollet June 17, 2022 10:21
@google-ml-butler google-ml-butler bot added the keras-team-review-pending Pending review by a Keras team member. label Jun 17, 2022
Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! First, we'll discuss with the team to determine whether we need/want explicit types throughout the codebase.

save_freq: Union[int, str] = "epoch",
options: Union[
tf.train.CheckpointOptions, tf.saved_model.SaveOptions, None
] = None,
Copy link
Member

Choose a reason for hiding this comment

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

In general we should avoid explicit types when the expect type is a custom class, like in the above case. It creates significant tech debt and maintenance burden going forward.

@fchollet
Copy link
Member

After extensive discussions, this is what we will do going forward:

We accept type annotations for simple Python types (not unions, and not custom types), but they're optional when adding new code. We will merge contributor PRs that add such annotations going forward. We will not maintain custom types.

Please update the PR accordingly and format the code using black. Thanks!

@gabrieldemarmiesse
Copy link
Contributor Author

gabrieldemarmiesse commented Jun 30, 2022

Could we make an exception for the public api? I get that it's a lot of work to use custom types everywhere in the codebase and to maintain them, but for the public API, it has a direct benefit to the user through Mypy and IDEs, so it might outweight the cost of maintenance

Furthermore, those public API type hints could be in theory be read and displayed in the documentation, giving us even more incentive to do it

This move would be in line with the first of the keras-team values: We have empathy for our users

@fchollet
Copy link
Member

fchollet commented Jul 1, 2022

This is our policy at this time. We may revise it in the future.

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

@google-ml-butler google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Jul 1, 2022
@fchollet fchollet removed the keras-team-review-pending Pending review by a Keras team member. label Jul 2, 2022
@copybara-service copybara-service bot merged commit 43416e6 into keras-team:master Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to pull Ready to be merged into the codebase size:S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants