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

Move Defaults to to end of arg docstring and standardise values #17748

Closed
wants to merge 4 commits into from

Conversation

SamuelMarks
Copy link
Contributor

I've written a Python compiler that can do a bunch of thing. Relevant to Keras I am turning TensorFlow into SQL tables by way of SQLalchemy:

# TensorFlow 2.12.0
$ python3 -m cdd exmod --module 'tensorflow.keras.optimizers' \
                       --emit 'sqlalchemy_table' \
                       -o '/tmp/tf_optimizers'

After deduplicating and running black here are the contents of sgd.py:

"""
Generated from keras.optimizers.sgd.
"""

SGD = Table(
    "SGD",
    metadata,
    Column(
        "learning_rate",
        Float,
        comment="A `Tensor`, floating point value, or a schedule that is a `tf.keras.optimizers.schedules.LearningRateSchedule`, or a callable that takes no arguments and returns the actual value to use. The learning rate",
        default=0.001,
        nullable=False,
    ),
    Column(
        "momentum",
        String,
        comment="float hyperparameter >= 0 that accelerates gradient descent in the relevant direction and dampens oscillations.e., vanilla gradient descent",
        default="0, i",
        nullable=False,
    ),
    Column(
        "nesterov",
        Boolean,
        comment="boolean. Whether to apply Nesterov momentum",
        default=False,
        nullable=False,
    ),
    Column("clipnorm", default=None, nullable=True),
    Column("clipvalue", default=None, nullable=True),
    Column("use_ema", Boolean, default=False, nullable=False),
    Column("ema_overwrite_frequency", default=None, nullable=True),
    Column("weight_decay", default=None, nullable=True),
    Column("global_clipnorm", default=None, nullable=True),
    Column("ema_momentum", Float, default=0.99, nullable=False),
    Column("jit_compile", Boolean, default=True, nullable=False),
    Column("name", String, default="SGD", nullable=False),
    Column("id", Integer, primary_key=True, server_default=Identity()),
)

__all__ = ["SGD"]

As you can see, momentum has an incorrect value because of how you've written your docstrings:
https://github.com/keras-team/keras/blob/0f8e81f/keras/optimizers/sgd.py#L62

Some commentary on the Defaults to syntax as understood by Sphinx:


So this PR rewrite your docstrings to put the Defaults to at the end and limit it to one value or a boolean condition.

PS: I also noticed inconsistent use of backticks. Did you want them everywhere? - This would disambiguate floats from integers I guess

@gbaned gbaned requested a review from qlzh727 March 31, 2023 18:37
@google-ml-butler google-ml-butler bot added the keras-team-review-pending Pending review by a Keras team member. label Mar 31, 2023
Copy link
Member

@qlzh727 qlzh727 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. Since this is quite large at the moment, is it possible to split this into smaller ones (one PR per package), so that we can speed up the review (sending to multiple team member.)?

@SamuelMarks
Copy link
Contributor Author

SamuelMarks commented Apr 4, 2023

@qlzh727 I can, in the past I had a script to turn one PR into one per file and that got through all the hoops until they reached TensorFlow master.

So you want me to do that here? - Happy to. - Maybe I'll link back here in case others want to chime in about repository-wide standards? - Also if any dis/agreements of this approach were to be made, maybe make them on this issue #17748

EDIT: Here's my script, was hitting GraphQL rate limits so added a sleep:

# Setup branch, commit, push up branch
while read -r name; do (
    branch="${name//\//.}"; branch="${branch::-3}-defaults-to";
    git checkout master && \
    git checkout -b "${branch}" && \
    git checkout defaults-to "$name" && \
    git commit -S -m '['"${name}"'] Standardise docstring usage of "Defaults to"' && \
    git push --set-upstream mine "$branch"; 
) done< <(git diff --name-only)

# Send PRs after `git diff --name-only > /tmp/names` on soft-reset `defaults-to` branch
while read -r name; do
    branch="${name//\//.}"; branch="${branch::-3}-defaults-to";
    git checkout "$branch" && \
    gh pr create \
        --title '['"${name}"'] Standardise docstring usage of "Default to"' \
        --body 'This is one of many PRs. Discussion + request to split into multiple PRs @ #17748' ; 
    sleep 5m ; # was doing `sleep 1h` before
done< <(cat /tmp/names)

This was referenced Apr 4, 2023
copybara-service bot pushed a commit that referenced this pull request May 5, 2023
…fault to"

Imported from GitHub PR #17973

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
b43b215 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/mixed_precision/loss_scale_optimizer.py] Standardise docstring usage of "Default to"

--
19dcdeb by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/mixed_precision/loss_scale_optimizer.py] Docstring minor improvements

Merging this change closes #17973

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17973 from SamuelMarks:keras.mixed_precision-defaults-to 19dcdeb
PiperOrigin-RevId: 529706785
copybara-service bot pushed a commit that referenced this pull request May 5, 2023
Imported from GitHub PR #17974

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
b96f2bc by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/models/cloning.py,keras/models/sharpness_aware_minimization.py] Standardise docstring usage of "Default to"

--
5719f7b by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/models/sharpness_aware_minimization.py] Use backticks for defaults in docstrings

Merging this change closes #17974

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17974 from SamuelMarks:keras.models-defaults-to 5719f7b
PiperOrigin-RevId: 529706730
copybara-service bot pushed a commit that referenced this pull request May 9, 2023
…ult to"

Imported from GitHub PR #17979

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
5c24834 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/testing_infra/test_combinations.py,keras/testing_infra/test_utils.py] Standardise docstring usage of "Default to"

Merging this change closes #17979

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17979 from SamuelMarks:keras.testing_infra-defaults-to 5c24834
PiperOrigin-RevId: 530509950
copybara-service bot pushed a commit that referenced this pull request May 9, 2023
…ault to"

Imported from GitHub PR #17958

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
c7157c0 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/feature_column/dense_features.py,keras/feature_column/sequence_feature_column.py] Standardise docstring usage of "Default to"

Merging this change closes #17958

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17958 from SamuelMarks:keras.feature_column-defaults-to c7157c0
PiperOrigin-RevId: 530510337
copybara-service bot pushed a commit that referenced this pull request May 9, 2023
Imported from GitHub PR #17956

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
9ad7371 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/engine/base_layer.py,keras/engine/base_layer_utils.py,keras/engine/base_layer_v1.py,keras/engine/base_preprocessing_layer.py,keras/engine/data_adapter.py,keras/engine/functional.py,keras/engine/input_layer.py,keras/engine/training.py,keras/engine/training_v1.py] Standardise docstring usage of "Default to"

Merging this change closes #17956

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17956 from SamuelMarks:keras.engine-defaults-to 9ad7371
PiperOrigin-RevId: 530510437
copybara-service bot pushed a commit that referenced this pull request May 9, 2023
Imported from GitHub PR #17956

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
9ad7371 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/engine/base_layer.py,keras/engine/base_layer_utils.py,keras/engine/base_layer_v1.py,keras/engine/base_preprocessing_layer.py,keras/engine/data_adapter.py,keras/engine/functional.py,keras/engine/input_layer.py,keras/engine/training.py,keras/engine/training_v1.py] Standardise docstring usage of "Default to"

Merging this change closes #17956

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17956 from SamuelMarks:keras.engine-defaults-to 9ad7371
PiperOrigin-RevId: 530510437
copybara-service bot pushed a commit that referenced this pull request May 16, 2023
…ult to"

Imported from GitHub PR #17979

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
5c24834 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/testing_infra/test_combinations.py,keras/testing_infra/test_utils.py] Standardise docstring usage of "Default to"

Merging this change closes #17979

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17979 from SamuelMarks:keras.testing_infra-defaults-to 5c24834
PiperOrigin-RevId: 530509950
copybara-service bot pushed a commit that referenced this pull request May 16, 2023
…ult to"

Imported from GitHub PR #17977

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
d7486bf by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/preprocessing/image.py,keras/preprocessing/text.py] Standardise docstring usage of "Default to"

--
af5d96a by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/preprocessing/text.py] Docstring grammar improvements

--
44c0b83 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/preprocessing/image.py] Consistent indent of 2 for `flow_from_directory` docstring ; `flow_from_directory` defaults-to in docstring

Merging this change closes #17977

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17977 from SamuelMarks:keras.preprocessing-defaults-to 44c0b83
PiperOrigin-RevId: 530503462
copybara-service bot pushed a commit that referenced this pull request May 25, 2023
…lt to"

Imported from GitHub PR #17954

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
f4445f1 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/applications/convnext.py,keras/applications/efficientnet.py,keras/applications/efficientnet_v2.py,keras/applications/imagenet_utils.py,keras/applications/inception_v3.py,keras/applications/mobilenet.py,keras/applications/mobilenet_v3.py,keras/applications/regnet.py,keras/applications/resnet_rs.py] Standardise docstring usage of "Default to"

--
5879670 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/applications] Remove  as these docstrings aren't interpolated

Merging this change closes #17954

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17954 from SamuelMarks:keras.applications-defaults-to 5879670
PiperOrigin-RevId: 535286449
copybara-service bot pushed a commit that referenced this pull request May 26, 2023
Imported from GitHub PR #17957

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
037fc4a by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/estimator/__init__.py] Standardise docstring usage of "Default to"

Merging this change closes #17957

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17957 from SamuelMarks:keras.estimator-defaults-to 037fc4a
PiperOrigin-RevId: 535275444
copybara-service bot pushed a commit that referenced this pull request May 26, 2023
Imported from GitHub PR #17953

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
6893bd5 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/utils/audio_dataset.py,keras/utils/conv_utils.py,keras/utils/data_utils.py,keras/utils/dataset_utils.py,keras/utils/feature_space.py,keras/utils/generic_utils.py,keras/utils/image_dataset.py,keras/utils/image_utils.py,keras/utils/layer_utils.py,keras/utils/losses_utils.py,keras/utils/metrics_utils.py,keras/utils/text_dataset.py] Standardise docstring usage of "Default to"

Merging this change closes #17953

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17953 from SamuelMarks:keras.utils-defaults-to 4cbcdf8
PiperOrigin-RevId: 535277920
copybara-service bot pushed a commit that referenced this pull request May 31, 2023
…ault to"

Imported from GitHub PR #17966

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
60043ea by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/layers/pooling/average_pooling2d.py,keras/layers/pooling/average_pooling3d.py,keras/layers/pooling/global_average_pooling2d.py,keras/layers/pooling/global_average_pooling3d.py,keras/layers/pooling/global_max_pooling2d.py,keras/layers/pooling/global_max_pooling3d.py,keras/layers/pooling/max_pooling2d.py,keras/layers/pooling/max_pooling3d.py] Standardise docstring usage of "Default to"

Merging this change closes #17966

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17966 from SamuelMarks:keras.layers.pooling-defaults-to 60043ea
PiperOrigin-RevId: 536547948
copybara-service bot pushed a commit that referenced this pull request May 31, 2023
…efault to"

Imported from GitHub PR #17971

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
5331dac by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/legacy_tf_layers/base.py,keras/legacy_tf_layers/migration_utils.py,keras/legacy_tf_layers/variable_scope_shim.py] Standardise docstring usage of "Default to"

--
1801651 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/legacy_tf_layers/migration_utils.py] Move docstring from method to class and document `seed`

Merging this change closes #17971

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17971 from SamuelMarks:keras.legacy_tf_layers-defaults-to 1801651
PiperOrigin-RevId: 535274600
copybara-service bot pushed a commit that referenced this pull request May 31, 2023
…f "Default to"

Imported from GitHub PR #17963

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
c7bcf63 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/layers/convolutional/base_depthwise_conv.py,keras/layers/convolutional/conv1d_transpose.py,keras/layers/convolutional/conv2d.py,keras/layers/convolutional/conv2d_transpose.py,keras/layers/convolutional/conv3d.py,keras/layers/convolutional/conv3d_transpose.py,keras/layers/convolutional/depthwise_conv1d.py,keras/layers/convolutional/depthwise_conv2d.py,keras/layers/convolutional/separable_conv2d.py] Standardise docstring usage of "Default to"

Merging this change closes #17963

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17963 from SamuelMarks:keras.layers.convolutional-defaults-to c7bcf63
PiperOrigin-RevId: 535275433
copybara-service bot pushed a commit that referenced this pull request May 31, 2023
Imported from GitHub PR #17956

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
9ad7371 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/engine/base_layer.py,keras/engine/base_layer_utils.py,keras/engine/base_layer_v1.py,keras/engine/base_preprocessing_layer.py,keras/engine/data_adapter.py,keras/engine/functional.py,keras/engine/input_layer.py,keras/engine/training.py,keras/engine/training_v1.py] Standardise docstring usage of "Default to"

Merging this change closes #17956

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17956 from SamuelMarks:keras.engine-defaults-to 9ad7371
PiperOrigin-RevId: 536547635
copybara-service bot pushed a commit that referenced this pull request May 31, 2023
…ault to"

Imported from GitHub PR #17958

This is one of many PRs. Discussion + request to split into multiple PRs @ #17748
Copybara import of the project:

--
c7157c0 by Samuel Marks <807580+SamuelMarks@users.noreply.github.com>:

[keras/feature_column/dense_features.py,keras/feature_column/sequence_feature_column.py] Standardise docstring usage of "Default to"

Merging this change closes #17958

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17958 from SamuelMarks:keras.feature_column-defaults-to c7157c0
PiperOrigin-RevId: 535275621
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keras-team-review-pending Pending review by a Keras team member. size:M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants