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

Fix apply() API change in TF 2.9 for unit tests #3427

Closed
wants to merge 1 commit into from

Conversation

chongxiaoc
Copy link
Collaborator

TF 2.9 removed apply() API, use call() instead.

Signed-off-by: Chongxiao Cao chongxiaoc@uber.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

Fixes #3422 .

Review process to land

  1. All tests and other checks must succeed.
  2. At least one member of the technical steering committee must review and approve.
  3. If any member of the technical steering committee requests changes, they must be addressed.

TF 2.9 removed apply() API, use __call__() instead.

Signed-off-by: Chongxiao Cao <chongxiaoc@uber.com>
@github-actions
Copy link

Unit Test Results

     773 files  ±0       773 suites  ±0   9h 14m 7s ⏱️ - 21m 7s
     722 tests ±0       675 ✔️ ±0       47 💤 ±0  0 ±0 
16 693 runs  ±0  11 867 ✔️ +2  4 826 💤  - 2  0 ±0 

Results for commit 72572ce. ± Comparison against base commit 642a6b3.

@github-actions
Copy link

Unit Test Results (with flaky tests)

     877 files  +  10       877 suites  +10   9h 41m 8s ⏱️ - 17m 0s
     722 tests ±    0       671 ✔️  -     3       47 💤 ±  0  4 +3 
18 987 runs  +151  13 373 ✔️ +129  5 610 💤 +19  4 +3 

For more details on these failures, see this check.

Results for commit 72572ce. ± Comparison against base commit 642a6b3.

@EnricoMi EnricoMi changed the title test: fix Issue#3422 Fix apply() API change in TF 2.9 for unit tests Feb 28, 2022
Copy link
Collaborator

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

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

The code could be simplified. I think this improves readability.

Comment on lines +4087 to +4090
bn_func = bn.apply(x, training=True) if LooseVersion(
tf.__version__) < LooseVersion('2.9.0') else bn(x, training=True)
sync_bn_func = sync_bn.apply(tf.expand_dims(x[hvd.rank()], 0), training=True) if LooseVersion(
tf.__version__) < LooseVersion('2.9.0') else sync_bn(tf.expand_dims(x[hvd.rank()], 0), training=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code could be simplified as follows:

Suggested change
bn_func = bn.apply(x, training=True) if LooseVersion(
tf.__version__) < LooseVersion('2.9.0') else bn(x, training=True)
sync_bn_func = sync_bn.apply(tf.expand_dims(x[hvd.rank()], 0), training=True) if LooseVersion(
tf.__version__) < LooseVersion('2.9.0') else sync_bn(tf.expand_dims(x[hvd.rank()], 0), training=True)
if LooseVersion(tf.__version__) < LooseVersion('2.9.0'):
bn = bn.apply
sync_bn = sync_bn.apply
bn_func = bn(x, training=True)
sync_bn_func = sync_bn(tf.expand_dims(x[hvd.rank()], 0), training=True)

sync_bn_func = sync_bn.apply(tf.expand_dims(x[hvd.rank()], 0), training=True)
# apply() API is removed in TF 2.9.0
bn_func = bn.apply(x, training=True) if LooseVersion(
tf.__version__) < LooseVersion('2.9.0') else bn(x, training=True)
Copy link
Collaborator

@Tixxx Tixxx Feb 28, 2022

Choose a reason for hiding this comment

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

I don't think you need the version check since __ call __ is by default always there in keras.layers.

@Tixxx
Copy link
Collaborator

Tixxx commented Feb 28, 2022

Didn't realize you were working on the same failure. I also opened one to fix all head failures. But feel free to merge this in after addressing the comments.

@EnricoMi
Copy link
Collaborator

@chongxiaoc in that case, I would prefer the call approach for any tensorflow version as in #3431. You can do this here and we merge #3431 afterwards.

@chongxiaoc
Copy link
Collaborator Author

close due to duplication with #3431

@chongxiaoc chongxiaoc closed this Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

TF Nightly breaks unit tests test_horovod_syncbn_gpu() and test_horovod_syncbn_cpu()
3 participants