matthew-z commented 5 years ago

update requirements.txt: keras=2.3.0 and tensorflow >= 2.0.0
upgrade pip in .travis.yml (tf 2.0 requires pip >= 19)
make raking losses inherit keras.losses.Loss to support sample_weight keyword param
replace some keras.backend.tf with tf (K.tf does not exist anymore in 2.3.0 as keras is going to be synced with tf.keras and drop multi-backend)
add clear_session before prepare in model tests to prevent OOM during CI test

fix #789

matthew-z commented 5 years ago

It seems that the DecayingDropoutLayer is broken in tf 2.0: replacing it with normal dropout could fix the error.

A minimal code gist to reproduce this error: https://gist.github.com/matthew-z/c6e0067e860d2e46648b2f1b3d891beb

Somehow K.in_train_phase cannot be used with self.add_update([K.update_add... in TF 2.0, so I replaced it with tf.control_dependencies and tf.assign_add to implement the same logic

uduse commented 5 years ago

Nice work! I just added you as one of the maintainers, so once this PR is finished and approved by at least 2 reviewers, you may go ahead and merge this.

bwanglzu commented 5 years ago

I was wondering does it still work for the ppl using tensorflow 1.x.x?

bwanglzu commented 5 years ago

@matthew-z also do not forget to add dependencies in setup.py

matthew-z commented 5 years ago

OK I have changed dependencies in setup.py as well.

It works with Keras 2.3 + tf 1.14, but not with Keras 2.2 + tf 1.14. I have not tested older tf versions.

Also, I think we should consider fully using tf.keras in tf 2.0.

Going forward, we recommend that users consider switching their Keras code to tf.keras in TensorFlow 2.0. It implements the same Keras 2.3.0 API (so switching should be as easy as changing the Keras import statements), but it has many advantages for TensorFlow users, such as support for eager execution, distribution, TPU training, and generally far better integration between low-level TensorFlow and high-level concepts like Layer and Model. It is also better maintained. https://github.com/keras-team/keras/releases

bwanglzu commented 5 years ago

@ZizhenWang did some job in #643 , can you review @ZizhenWang ?

@matthew-z checkout description in #642

codecov-io commented 5 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (2.2-dev@60c0bb6). Click here to learn what that means. The diff coverage is 92.89%.

@@           Coverage Diff            @@
##             2.2-dev   #793   +/-   ##
========================================
  Coverage           ?    92%           
========================================
  Files              ?    102           
  Lines              ?   3704           
  Branches           ?      0           
========================================
  Hits               ?   3408           
  Misses             ?    296           
  Partials           ?      0

Impacted Files	Coverage Δ
matchzoo/contrib/layers/matching_tensor_layer.py	`100% <100%> (ø)`
matchzoo/losses/rank_cross_entropy_loss.py	`100% <100%> (ø)`
...atchzoo/contrib/layers/semantic_composite_layer.py	`100% <100%> (ø)`
matchzoo/contrib/models/esim.py	`100% <100%> (ø)`
matchzoo/models/mvlstm.py	`100% <100%> (ø)`
matchzoo/contrib/models/diin.py	`98% <100%> (ø)`
matchzoo/models/knrm.py	`100% <100%> (ø)`
matchzoo/contrib/layers/spatial_gru.py	`100% <100%> (ø)`
matchzoo/models/drmm.py	`100% <100%> (ø)`
matchzoo/losses/rank_hinge_loss.py	`91.3% <100%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 60c0bb6...4ca0ddc. Read the comment docs.

matthew-z commented 5 years ago

As keras 2.3 removed K.tf.*, so my initial thought is simply to fix K.tf in the affected files. Some ops have alias in K.*, but some of them don't. Do you think we should replace all K.* with tf.*?

Some K.* have no equivalent tf.*, such as K.batch_dot, which means we may still need to keep both tf.keras.backend.* and tf.* in some cases.

matthew-z commented 5 years ago

@ZizhenWang did some job in #643 , can you review @ZizhenWang ?

@matthew-z checkout description in #642

I was wondering why the PR was closed without merging?

bwanglzu commented 5 years ago

@matthew-z I have no clue.. why they closed the previous pr.

matthew-z commented 5 years ago

Thank you for the review. I guess I need one more reviewer to approve before merging it?

bwanglzu commented 5 years ago

@matthew-z Yes! I'll send them a message.

ZizhenWang commented 5 years ago

Sorry for the late response, I will review the code ASAP.

ZizhenWang commented 5 years ago

@matthew-z Nice work! You can merge PR after doing the little changes as I comment.

I notice that you use tensorflow explicitly rather than as one of the keras backends. In that way, tf seems parallel to keras in this repo, just as @bwanglzu mentioned.

At #643 I use tf.keras instead of keras, it makes matchzoo dependent entirely on tensorflow, but that PR is delayed by other things. If you have interest on tf.keras, we can open another PR and remove keras.

Thanks for your contribution again!

matthew-z commented 5 years ago

Thank you for the review. I am interested in switching to tf.keras, and I will have a try soon

uduse commented 5 years ago

Good job! Time for matchzoo version 2.2!

matthew-z commented 5 years ago

BTW, CI failed because the certificate of nlp.stanford.edu expired several hours ago....

NTMC-Community / MatchZoo

support keras 2.3 and tensorflow 2.0 #793

Codecov Report