Remove use-grappler-optimizer compile option #799
Conversation
adk9
commented
Feb 1, 2021
•
adk9
commented
- Unconditionally enable "ngraph-optimizer" grappler pass
- Remove is_grappler_enabled() API call
- Run tests using the grappler pass
* Unconditionally enable grappler path * Remove is_grappler_enabled() API call * Run tests using the grappler pass
The grappler optimizer, now used by default for tests, does not fail on optimizer error unless explicitly specified in the rewriter config. Invert the logic in test_disable_op_2 to not expect a session failure when trying to disable invalid ops.
| @@ -41,6 +41,7 @@ namespace testing { | |||
| // etc.,etc. | |||
|
|
|||
| TEST(GrapplerConfig, RConfig1) { | |||
| ActivateNGraph(); | |||
There was a problem hiding this comment.
Why is the explicit enabling/disabling of nGraph required?
There was a problem hiding this comment.
It's to disable de-assigning of clusters.
There was a problem hiding this comment.
You can use NGRAPH_TF_DISABLE_DEASSIGN_CLUSTERS ?
There was a problem hiding this comment.
Where? To run the entire unit test suite? I'm not sure why this isn't being done already.
| patch_file_name = "test/python/tensorflow/tf_unittest_ngraph" + ( | ||
| "_with_grappler" | ||
| if ngraph_bridge.is_grappler_enabled() else "") + ".patch" | ||
| patch_file_name = "test/python/tensorflow/tf_unittest_ngraph.patch" |
There was a problem hiding this comment.
The TF py tests will also run only with the graph pass?
There was a problem hiding this comment.
Yes, bridge C++ unit tests run with the optimizer, bridge Python and TF Python unit tests run without the optimizer. It seems a little excessive to test all the combinations here since the optimizer and non-optimizer paths are shared. Which configurations would we prefer testing?
There was a problem hiding this comment.
Which one is going to be the default/promoted more than the other, we should make sure all tests pass with that. If there isn't any then probably the above configuration is fine.
There was a problem hiding this comment.
The current default is non-optimizer. We could use this for all three but that'd leave the optimizer path untested.
