Alternative to run_all_in_graph_and_eager_mode #1288
Conversation
gabrieldemarmiesse
changed the title
gabrieldemarmiesse
changed the title
gabrieldemarmiesse
changed the title
There was a problem hiding this comment.
Thanks this definitely simplifies the testing for new contributors! I think this should work since the only support for graph mode in 2.x is tf.function. It's possible some people will want to use Addons using tf.compat.v1 but to me that's outside of our scope.
Couple of clarifying questions, and then I also wish we had a better understanding of the keras black magic. Checking the metrics module it says it'll run as graph but quite difficult for me to see where that is happening:
https://github.com/tensorflow/tensorflow/blob/v2.1.0/tensorflow/python/keras/metrics.py#L218
It's possible that this isn't being controlled with tf.function in which case I think the tests should fail with or without the experimental config so probably a non-issue.
tensorflow_addons/conftest.py
Outdated
| @pytest.fixture(scope="function", params=["eager_mode", "tf_function"]) | ||
| def maybe_run_functions_eagerly(request): | ||
| if request.param == "eager_mode": | ||
| tf.config.experimental_run_functions_eagerly(True) |
There was a problem hiding this comment.
So I agree this is preferable to a private API, but just want to note it is still experimental and could change on us:
tensorflow/community#218
There was a problem hiding this comment.
Could we communicate to the TF team that this flag is really needed on our side? So that it gets out of experimental mode? It's a much needed feature for debugging in general. CF #13 (comment)
|
* Alternative to run_all_in_graph_and_eager_mode. * Better names when printing stuff. * Give another example. * Get finalizer out. * Moved the maybe_run_functions_eagerly to test_utils.
So, replacing this
run_all_in_graph_and_eager_modeis quite difficult but it's worth it for two reasons:callmethod, for the metrics/losses, it's theupdate_state...). It allows us to enjoy the eager mode outside those selected functions and we don't have to doself.sessionself.evaluate(tf.compat.v1.initialize_global_variables(...)). We can just do.numpy()to get results. This has the benefit to make testing much easier and bring new contributors who might be put off by this testing which looks like coding in tensorflow 1.x.tf.functionin eager mode. So it's not actually helping us at all. I think it's just a TF 1.x relict and is useful only to force graph mode when tf.function is not present. See [WIP] Should break in eager mode. #1289So how to do that, in a clean way? Well that's more or less difficult.
From the discussion in #13 , all public functions should have the
@tf.functionsdecorator. #807 shows that it's a problem when using python variables, but in this case, we should just use theinput_signatureto convert automatically scalars to tensorflow tensors. See https://www.tensorflow.org/api_docs/python/tf/function . We can even add tests to ensure we don't draw the graph multiple times with the methodget_concrete_function.We already use tf.function many time in the codebase, and even when we don't use it, keras enforce it with black magic when subclassing
Metricor evenLayer.Let's then assume that all public functions have the
tf.functiondecorator.Here is my proposal. It uses pytest features and it won't work when subclassing tf.test.TestCase. But I believe we don't need subclassing anyway for most of the tests present in tf.addons. Worst case scenario we can code it again for
tf.test.TestCasewith the absl parametrize decorator.conftest.pymust be present when running the tests which means that we need to make bazel aware of it. This is why I linked it totensorflow_addons/utilswhich is present all the time in the tests.Some reference:
https://docs.pytest.org/en/latest/fixture.html