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

Bugfix/normalization invert #17501

Merged

Conversation

jerabaul29
Copy link
Contributor

Fix bug keras-team/tf-keras#281 , by:

  • making sure the invert attribute is tracked when saving / loading
  • adding extra test for this case.

Notes:

  • I am not too familiar with the keras tests setup, so the test I added may need editing
  • there are several tests around my new additional test, it is possible that one may want to add extra "invert" tests corresponding to each of these.

@google-cla
Copy link

google-cla bot commented Jan 30, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@gbaned gbaned requested a review from qlzh727 January 30, 2023 16:37
@google-ml-butler google-ml-butler bot added the keras-team-review-pending Pending review by a Keras team member. label Jan 30, 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 fix, overall LGTM, please fix the format.

@qlzh727
Copy link
Member

qlzh727 commented Jan 30, 2023

Thanks for the fix, overall LGTM, please fix the format.

See https://github.com/keras-team/keras/actions/runs/4044365513/jobs/6958299676

@qlzh727
Copy link
Member

qlzh727 commented Jan 31, 2023

The format is still broken. Please use the format script in the repo https://github.com/keras-team/keras/blob/master/shell/format.sh

@jerabaul29
Copy link
Contributor Author

FYI: I first tried running this script on the full repository. The isort command failed:

~/Desktop/Git/keras [bugfix/normalization_invert|✔]> isort -sl keras
WARNING: Unable to parse file keras due to [Errno 21] Is a directory: '/home/jrmet/Desktop/Git/keras/keras'
~/Desktop/Git/keras/keras [bugfix/normalization_invert|✔]> isort -sl keras
WARNING: Unable to parse file keras due to [Errno 2] No such file or directory: '/home/jrmet/Desktop/Git/keras/keras/keras'

Then the black command found many hits that "needed" refactor, far outside what I was writing / modifying:

~/Desktop/Git/keras [bugfix/normalization_invert|✔]> black --line-length 80 keras
reformatted /home/jrmet/Desktop/Git/keras/keras/applications/mobilenet_v2.py
reformatted /home/jrmet/Desktop/Git/keras/keras/applications/mobilenet_v3.py
reformatted /home/jrmet/Desktop/Git/keras/keras/benchmarks/metrics_memory_benchmark_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/benchmarks/eager_microbenchmarks_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/benchmarks/keras_examples_benchmarks/mnist_conv_custom_training_benchmark_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/benchmarks/layer_benchmarks/layer_benchmarks_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/callbacks_v1.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/custom_training_loop_optimizer_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/distributed_training_utils.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/dataset_creator_model_fit_test_base.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/custom_training_loop_models_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/keras_embedding_model_correctness_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/distribute_coordinator_utils.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/mirrored_variable_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/saved_model_test_base.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/multi_worker_callback_tf2_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/dtensor/integration_test_utils.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/distributed_training_utils_v1.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/keras_utils_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/sharded_variable_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/dtensor/utils_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/engine/keras_tensor.py
reformatted /home/jrmet/Desktop/Git/keras/keras/engine/data_adapter.py
reformatted /home/jrmet/Desktop/Git/keras/keras/distribute/distribute_strategy_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/engine/training_arrays_v1.py
reformatted /home/jrmet/Desktop/Git/keras/keras/engine/base_layer.py
reformatted /home/jrmet/Desktop/Git/keras/keras/callbacks_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/engine/base_layer_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/engine/training_generator_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/feature_column/sequence_feature_column_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/feature_column/dense_features_v2_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/engine/training_utils_v1.py
reformatted /home/jrmet/Desktop/Git/keras/keras/feature_column/dense_features_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/integration_test/parameter_server_custom_training_loop_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/engine/functional_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/integration_test/parameter_server_keras_preprocessing_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/integration_test/multi_worker_tutorial_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/integration_test/saved_model_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/engine/training_v1.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/convolutional/base_conv.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/attention/multi_head_attention.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/pooling/max_pooling1d.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/preprocessing/discretization_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/preprocessing/text_vectorization.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/preprocessing/preprocessing_stage_functional_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/preprocessing/normalization_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/reshaping/cropping2d.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/reshaping/cropping3d.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/rnn/legacy_cell_wrappers.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/rnn/gru_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/rnn/legacy_cells.py
reformatted /home/jrmet/Desktop/Git/keras/keras/engine/training_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/rnn/lstm_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/layers/rnn/base_rnn_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/mixed_precision/autocast_variable.py
reformatted /home/jrmet/Desktop/Git/keras/keras/mixed_precision/layer_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/mixed_precision/mixed_precision_graph_rewrite_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/metrics/base_metric_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/metrics/metrics.py
reformatted /home/jrmet/Desktop/Git/keras/keras/models/cloning_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/optimizers/legacy/nadam_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/mixed_precision/loss_scale_optimizer_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/optimizers/utils.py
reformatted /home/jrmet/Desktop/Git/keras/keras/optimizers/legacy/optimizer_v2.py
reformatted /home/jrmet/Desktop/Git/keras/keras/preprocessing/sequence.py
reformatted /home/jrmet/Desktop/Git/keras/keras/saving/legacy/saved_model/layer_serialization.py
reformatted /home/jrmet/Desktop/Git/keras/keras/optimizers/optimizer_v1.py
reformatted /home/jrmet/Desktop/Git/keras/keras/saving/object_registration_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/saving/legacy/saving_utils.py
reformatted /home/jrmet/Desktop/Git/keras/keras/metrics/metrics_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/saving/legacy/saved_model/save_impl.py
reformatted /home/jrmet/Desktop/Git/keras/keras/saving/saving_lib.py
reformatted /home/jrmet/Desktop/Git/keras/keras/testing_infra/test_combinations.py
reformatted /home/jrmet/Desktop/Git/keras/keras/saving/legacy/saved_model/load.py
reformatted /home/jrmet/Desktop/Git/keras/keras/tests/custom_training_loop_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/saving/legacy/save_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/saving/legacy/saved_model/saved_model_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/tests/model_subclassing_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/utils/generic_utils.py
reformatted /home/jrmet/Desktop/Git/keras/keras/tests/tracking_util_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/utils/dataset_utils_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/utils/object_identity.py
reformatted /home/jrmet/Desktop/Git/keras/keras/utils/losses_utils.py
reformatted /home/jrmet/Desktop/Git/keras/keras/utils/sidecar_evaluator_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/utils/tf_utils_test.py
reformatted /home/jrmet/Desktop/Git/keras/keras/utils/metrics_utils.py

All done! ✨ 🍰 ✨
86 files reformatted, 629 files left unchanged.

So I ended up running black on just the files I modified, which is what is committed now.

Not sure how you want modified where, if this is still not what you want, can you apply these changes yourself? Thanks :) .

@google-ml-butler google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Feb 1, 2023
@rchao rchao removed the keras-team-review-pending Pending review by a Keras team member. label Feb 2, 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.

Btw, it seems that the unit test is failed for the newly added test. Please fix. Thanks.

@qlzh727
Copy link
Member

qlzh727 commented Feb 2, 2023

Btw, it seems that the unit test is failed for the newly added test. Please fix. Thanks.

See https://source.cloud.google.com/results/invocations/c5bfb743-1844-4093-8802-30cf7171cb07/log for more details.

@google-ml-butler google-ml-butler bot removed the ready to pull Ready to be merged into the codebase label Feb 3, 2023
@jerabaul29
Copy link
Contributor Author

Sorry, I was confused on how to set invert=True in the tests. Should be fixed now. I was probably running the test in a wrong way on my end which is why I did not catch it.

A small note: is there a way for me to see the output of the CI/CD before you validate every step? :) I think not seeing the CI/CD output before you allow it to run is making it harder for me to catch this kind of stupid mistakes that the CI/CD would have reported to me at once otherwise :) . (I suppose CI/CD does not run automatically when I push so that you cannot get ddos:ed or something like this?)

@jerabaul29
Copy link
Contributor Author

Any update on this? It is a quite 'impactful' bug (as models using inverse normalisation layer get destroyed with a save and load cycle as of today), would be great to push :) .

@qlzh727 qlzh727 added the ready to pull Ready to be merged into the codebase label Feb 9, 2023
copybara-service bot pushed a commit that referenced this pull request Feb 14, 2023
Imported from GitHub PR #17501

Fix bug #17486 , by:

- making sure the ```invert``` attribute is tracked when saving / loading
- adding extra test for this case.

Notes:

- I am not too familiar with the keras tests setup, so the test I added may need editing
- there are several tests around my new additional test, it is possible that one may want to add extra "invert" tests corresponding to each of these.
Copybara import of the project:

--
f5452d5 by JRT <jean.rblt@gmail.com>:

Add invert attribute to get_config for Normalization

--
a2cf8c9 by JRT <jean.rblt@gmail.com>:

Add test for invert normalization load

--
04e4017 by JRT <jean.rblt@gmail.com>:

Fix formatting

--
b1660f0 by JRT <jean.rblt@gmail.com>:

Run black to format

--
ba1207c by JRT <jean.rblt@gmail.com>:

Fix setting invert property

Merging this change closes #17501

FUTURE_COPYBARA_INTEGRATE_REVIEW=#17501 from jerabaul29:bugfix/normalization_invert ba1207c
PiperOrigin-RevId: 509576868
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.

None yet

5 participants