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

Initial TF session management #120

Merged
merged 18 commits into from
Sep 9, 2019
Merged

Initial TF session management #120

merged 18 commits into from
Sep 9, 2019

Conversation

jklaise
Copy link
Member

@jklaise jklaise commented Jul 29, 2019

We want to move the TF session management internally to the explainers that use TF, this is a first attempt taking CounterFactual as an example.

The main difficulty is that with tf.keras this works out of the box (tests passing etc.), but using keras is throwing FailedPreconditionError, so there is some incompatibility with initialization between tf.keras and keras.

@codecov-io
Copy link

codecov-io commented Jul 29, 2019

Codecov Report

Merging #120 into master will increase coverage by 0.67%.
The diff coverage is 95.2%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
+ Coverage   85.42%   86.09%   +0.67%     
==========================================
  Files          30       32       +2     
  Lines        2580     2647      +67     
==========================================
+ Hits         2204     2279      +75     
+ Misses        376      368       -8
Impacted Files Coverage Δ
alibi/utils/tf.py 100% <100%> (ø)
alibi/utils/tests/test_utils.py 100% <100%> (ø)
alibi/explainers/tests/test_cem.py 96% <66.66%> (-0.67%) ⬇️
alibi/explainers/counterfactual.py 80.95% <83.33%> (+0.51%) ⬆️
alibi/explainers/cem.py 79.45% <90.9%> (ø) ⬆️
alibi/explainers/cfproto.py 78.36% <91.66%> (+0.69%) ⬆️
alibi/explainers/tests/test_cfproto.py 97.67% <93.1%> (-2.33%) ⬇️
alibi/explainers/tests/test_counterfactual.py 99.02% <95%> (+0.95%) ⬆️
alibi/datasets.py 86.82% <0%> (-2.33%) ⬇️
alibi/explainers/anchor_base.py 89.92% <0%> (ø) ⬆️
... and 5 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 68dd474...a28de36. Read the comment docs.

@jklaise
Copy link
Member Author

jklaise commented Jul 29, 2019

This may be solved by conditionally importing either tensorflow.keras.backend or keras.backend depending on the model passed in to get the model's session.

@jklaise
Copy link
Member Author

jklaise commented Jul 31, 2019

Update:

  • CounterFactual works with both tf.keras and keras models now
  • CounterFactualProto is more complex as it can have multiple models passed in (model itself, autoencoder and encoder). Currently works when all components are made with either tf.keras or keras, but no mix-and-match. Left to investigate whether this can be supported automatically. Worst case, we can have sess as an optional argument if people want to pass in mixed components, so the burden then would be on the user to match up the sessions.

@jklaise
Copy link
Member Author

jklaise commented Aug 9, 2019

Remaining things to land this: CEM and CFProto example notebooks and documentation needs updating @arnaudvl

  • Currently the CEM notebook appears very sensitive to the model for producing reliable PNs and PPs with the same set of hyperparameters
  • The CFProto notebook can't be executed end-to-end because we switch from a TF model to a black-box model midway through, so there is still and underlying old graph which is causing issues. Looks like it's currently broken for black-box functions that happen to have a graph attached.

@jklaise
Copy link
Member Author

jklaise commented Aug 14, 2019

After chatting to @arnaudvl it makes sense that a black-box function which is a keras model in disguise (i.e. via predict=model.predict) would not work, as it already has an attached session which we discard when initializing tf.Session for black-box functions. So we need a way to get the default session regardless if the passed object is a model or a black-box function. This gets a bit complicated as we still need to somehow check that what is passed in is derived from tf.keras or keras.

@jklaise
Copy link
Member Author

jklaise commented Aug 27, 2019

The only thing left to do is to produce a good example for the CEM MNIST notebook - this proves to be quite tricky as the algorithm is quite sensitive to the trained model and hyperparameters.

Other than that, the following (across CEM, CounterFactual and CounterFactualProto) are supported:

  • tf.keras models
  • keras models
  • black-box models (e.g. sklearn models via passing predict_proba as a function
  • black-box models that are actually tf.keras models (e.g. via passing model.predict)
  • any other tf or keras model via passing in a tf session via the optional sess keyword parameter

@jklaise jklaise changed the title WIP: Initial TF session management Initial TF session management Aug 27, 2019
@arnaudvl arnaudvl merged commit 4a87657 into SeldonIO:master Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants