Conversation
Added code for skipping the assert op if the env variable NGRAPH_TF_SKIP_ASSERT is set i.e basically get rid of the control edge Dump unmarked graphs with assert skipped Added python test for checking the assert is actually skipped
kanvi-nervana
requested review from
avijit-nervana,
sayantan-nervana and
shresthamalik
sayantan-nervana
left a comment
sayantan-nervana
left a comment
There was a problem hiding this comment.
Requesting changes in:
- python test assert
- prints in SkipAssert
test/python/test_skip_assert.py
Outdated
| z: test_input | ||
| }) | ||
|
|
||
| self.with_ngraph(run_test) |
There was a problem hiding this comment.
maybe we want to do this with and without ngraph?
assert (self.with_ngraph(run_test) == self.without_ngraph(run_test)).all()
There was a problem hiding this comment.
We can write a test that fails with tf and works with nGraph (as we got rid of control edges). But would that be ideal?
There was a problem hiding this comment.
I think as a test, it is a good idea. But in a real life network, its not ideal.
There was a problem hiding this comment.
What would be the purpose for such a test?
src/ngraph_rewrite_pass.cc
Outdated
| if (std::getenv("NGRAPH_TF_SKIP_ASSERT") != nullptr) { | ||
| TF_RETURN_IF_ERROR(SkipAssert(options.graph->get())); | ||
| // If requested, dump unmarked graphs without asserts | ||
| if (DumpCapturedGraphs()) { |
There was a problem hiding this comment.
Should we use a new check DumpSkipAssertGraphs() (We can check for DumpAllGraphs() and NGRAPH_TF_SKIP_ASSERT, May not want another flag) like the other ones?
There was a problem hiding this comment.
Another concern is that skip assert is part of VariableCapturePass, it should not be there if we go by the name of the pass (should only be capturing variables). We kept it there as we wanted to test at which stage the edges need to be removed and before TF Rewrite for Execution seems to work for us.
So should we introduce/register a new pass or rename the Variable Capture Pass or the current implementation is fine.
On these lines, encapsulate pass also has a function (rewrite for tracking) that does not really align with encapsulation.
There was a problem hiding this comment.
@shresthamalik good feedback. We need to refactor the code to take care of these. Please create a JIRA ticket and we will address these.
There was a problem hiding this comment.
Do we need to do anything in this regards for this PR?
src/ngraph_rewrite_pass.cc
Outdated
| if (std::getenv("NGRAPH_TF_SKIP_ASSERT") != nullptr) { | ||
| TF_RETURN_IF_ERROR(SkipAssert(options.graph->get())); | ||
| // If requested, dump unmarked graphs without asserts | ||
| if (DumpCapturedGraphs()) { |
There was a problem hiding this comment.
@shresthamalik good feedback. We need to refactor the code to take care of these. Please create a JIRA ticket and we will address these.
There was a problem hiding this comment.
Are we doing the ngraph-will-pass-but-tf-will-fail test?
the python test could be arranged as:
try:
run_tf
assert False #the above line should fail, so we should not hit this assert
catch:
run_ngtf #once run-tf fails, we land up here
LGTM otherwise
shresthamalik
left a comment
shresthamalik
left a comment
There was a problem hiding this comment.
Other than the test. LGTM
| }) | ||
|
|
||
| assert ( | ||
| self.with_ngraph(run_test) == self.without_ngraph(run_test)).all() |
There was a problem hiding this comment.
This test wont run with the env variable in the CI, right?
shresthamalik
left a comment
shresthamalik
left a comment
There was a problem hiding this comment.
Need to revisit the test.
4 participants
No description provided.