Skip to content

Fix broken TF DistributedOptimizer with Keras 2.11+ #3822

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
chongxiaoc merged 13 commits into from
Jan 30, 2023
Merged

Fix broken TF DistributedOptimizer with Keras 2.11+ #3822

chongxiaoc merged 13 commits into from
Jan 30, 2023

Conversation

@nvcastet
Copy link
Collaborator

@nvcastet nvcastet commented Jan 23, 2023 •
edited
Loading

Signed-off-by: Nicolas Castet ncastet@nvidia.com

Checklist before submitting

  • Did you read the contributor guide?
  • Did you update the docs?
  • Did you write any tests to validate this change?
  • Did you update the CHANGELOG, if this change affects users?

Description

Horovod was only checking for tf.keras.optimizers.Optimizer in DistributedOptimizer but supported optimizers are under tf.keras.optimizers.legacy.Optimizer for Keras 2.11+.

@nvcastet nvcastet requested a review from chongxiaoc January 23, 2023 16:54
@chongxiaoc
Copy link
Collaborator

chongxiaoc commented Jan 23, 2023

There are some tests failing. For example:

=================================== FAILURES ===================================
_______________ Tf2KerasTests.test_partial_distributed_optimizer _______________

self = <test_tensorflow2_keras.Tf2KerasTests testMethod=test_partial_distributed_optimizer>

@chongxiaoc
Copy link
Collaborator

chongxiaoc commented Jan 23, 2023

But I like this fix. Should land as soon as all tests pass.

@nvcastet
Copy link
Collaborator Author

nvcastet commented Jan 23, 2023

Thanks @chongxiaoc Yes I need to update the different tests/examples. Since with Keras >= 2.11 we were not always using the legacy optimizers.

@github-actions
Copy link

github-actions bot commented Jan 23, 2023 •
edited
Loading

Unit Test Results

0 files   -      977  0 suites   - 977   0s ⏱️ - 11h 47m 43s
0 tests  -      865  0 ✔️  -      754  0 💤  -    111  0 ❌ ±0 
0 runs   - 21 131  0 ✔️  - 14 810  0 💤  - 6 321  0 ❌ ±0 

Results for commit 7230379. ± Comparison against base commit 5266a29.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jan 23, 2023 •
edited
Loading

Unit Test Results (with flaky tests)

0 files   -   1 164  0 suites   - 1 164   0s ⏱️ - 13h 10m 40s
0 tests  -      865  0 ✔️  -      753  0 💤  -    111  0 ❌  - 1 
0 runs   - 25 179  0 ✔️  - 17 397  0 💤  - 7 779  0 ❌  - 3 

Results for commit 7230379. ± Comparison against base commit 5266a29.

♻️ This comment has been updated with latest results.

@chongxiaoc
Copy link
Collaborator

chongxiaoc commented Jan 24, 2023

some missing examples and tests:

tensorflow2_mnist_data_service_train_fn_compute_side_dispatcher.py
test/parallel/test_tensorflow2_keras.py
/horovod/examples/tensorflow2/tensorflow2_keras_mnist.py

@pjannaty
Copy link

pjannaty commented Jan 25, 2023

Thank you both, @chongxiaoc and @nvcastet. This is an important fix for TensorFlow!

nvcastet added 13 commits January 30, 2023 12:57
@nvcastet
Fix broken TF DistributedOptimizer with Keras 2.11+
86d6671 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Move type check to _keras.create_distributed_optimizer
70d49a1 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Update changelog
614382a 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Remove extra import
81304b9 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Fix tests
08e8a23 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Fix imports in tests
80ad6ca 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Fix tests well this time
7128ab2 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Fix test imports
4d06115 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Fix 2 more tests
634376a 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Fix when importing 'keras' instead of 'tf.keras'
f372438 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Fix circular import
457673f 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Fix optimizer check when version < 2.11
263e32f 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Fix error for tf-nightly
7230379 
Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@nvcastet
Copy link
Collaborator Author

nvcastet commented Jan 30, 2023

@chongxiaoc Finally, we should be good.

chongxiaoc
chongxiaoc approved these changes Jan 30, 2023
View reviewed changes
Copy link
Collaborator

@chongxiaoc chongxiaoc left a comment

Choose a reason for hiding this comment

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

Thx for fixing it.

@chongxiaoc chongxiaoc merged commit 305280d into horovod:master Jan 30, 2023
@nvcastet nvcastet deleted the fix_new_keras_opts branch January 30, 2023 22:13
nvcastet added a commit to nvcastet/horovod that referenced this pull request Mar 10, 2023
@nvcastet
Revert "Fix broken TF DistributedOptimizer with Keras 2.11+ (horovod#…
7c36a49 
…3822)"

This reverts commit 305280d.

Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
@tejaschumbalkar tejaschumbalkar mentioned this pull request Mar 13, 2023
Closed
4 tasks
nvcastet added a commit to nvcastet/horovod that referenced this pull request Mar 24, 2023
@nvcastet
Revert "Fix broken TF DistributedOptimizer with Keras 2.11+ (horovod#…
ec79003 
…3822)"

This reverts commit 305280d.

Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
nvcastet added a commit to nvcastet/horovod that referenced this pull request Apr 4, 2023
@nvcastet
Revert "Fix broken TF DistributedOptimizer with Keras 2.11+ (horovod#…
e672c97 
…3822)"

This reverts commit 305280d.

Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
nvcastet added a commit to nvcastet/horovod that referenced this pull request Apr 17, 2023
@nvcastet
Revert "Fix broken TF DistributedOptimizer with Keras 2.11+ (horovod#…
582dadb 
…3822)"

This reverts commit 305280d.

Signed-off-by: Nicolas Castet <ncastet@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chongxiaoc chongxiaoc chongxiaoc approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nvcastet @chongxiaoc @pjannaty