Kanvi/skip assert #265

kanvi-nervana · 2018-10-17T21:03:33Z

No description provided.

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

sayantan-nervana

Requesting changes in:

python test assert
prints in SkipAssert

sayantan-nervana · 2018-10-17T21:25:48Z

test/python/test_skip_assert.py

+                    z: test_input
+                })
+
+        self.with_ngraph(run_test)


maybe we want to do this with and without ngraph?

assert (self.with_ngraph(run_test) == self.without_ngraph(run_test)).all()

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?

I think as a test, it is a good idea. But in a real life network, its not ideal.

What would be the purpose for such a test?

src/ngraph_skip_assert.cc

shresthamalik · 2018-10-18T01:18:51Z

src/ngraph_rewrite_pass.cc

+    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()) {


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?

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.

@shresthamalik good feedback. We need to refactor the code to take care of these. Please create a JIRA ticket and we will address these.

Do we need to do anything in this regards for this PR?

@kanvi-nervana No

src/ngraph_rewrite_pass.cc

avijit-nervana · 2018-10-18T18:16:43Z

src/ngraph_rewrite_pass.cc

+    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()) {


@shresthamalik good feedback. We need to refactor the code to take care of these. Please create a JIRA ticket and we will address these.

src/ngraph_skip_assert.cc

test/python/test_skip_assert.py

…/ngraph-tf into kanvi/skip-assert

avijit-nervana

Looks good.

sayantan-nervana

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

Other than the test. LGTM

shresthamalik · 2018-10-18T20:55:59Z

test/python/test_disable_assert.py

+                })
+
+        assert (
+            self.with_ngraph(run_test) == self.without_ngraph(run_test)).all()


This test wont run with the env variable in the CI, right?

shresthamalik

Need to revisit the test.

Add more checks

…/ngraph-tf into kanvi/skip-assert

ci changes

kanvi.khanna added 4 commits October 16, 2018 16:52

Changes for skipping the Assert op

3f96d19

Merge branch 'master' into kanvi/skip-assert

92aa2ba

Disable deadness check

d79a4e6

Skip Assert

31e055c

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 shresthamalik, avijit-nervana and sayantan-nervana October 17, 2018 21:03

Fix format

cbd15d7

sayantan-nervana suggested changes Oct 17, 2018

View reviewed changes

shresthamalik reviewed Oct 18, 2018

View reviewed changes

Merge branch 'master' into kanvi/skip-assert

59b3e57

avijit-nervana suggested changes Oct 18, 2018

View reviewed changes

kanvi.khanna added 4 commits October 18, 2018 12:01

Incorporate comments

98abc87

Merge branch 'master' into kanvi/skip-assert

75493ae

Merge branch 'kanvi/skip-assert' of https://github.com/NervanaSystems…

d7696d4

…/ngraph-tf into kanvi/skip-assert

Corrected spelling mistakes

9acd86e

avijit-nervana approved these changes Oct 18, 2018

View reviewed changes

sayantan-nervana approved these changes Oct 18, 2018

View reviewed changes

shresthamalik approved these changes Oct 18, 2018

View reviewed changes

shresthamalik reviewed Oct 18, 2018

View reviewed changes

avijit-nervana added the Fully Reviewed All reviewers have approved label Oct 19, 2018

Merge branch 'master' into kanvi/skip-assert

2363177

sayantan-nervana removed Fully Reviewed All reviewers have approved labels Oct 19, 2018

shresthamalik suggested changes Oct 19, 2018

View reviewed changes

kanvi.khanna and others added 5 commits October 19, 2018 11:33

Fix segmentation fault

67bda54

Add more checks

Merge branch 'kanvi/skip-assert' of https://github.com/NervanaSystems…

c5746df

…/ngraph-tf into kanvi/skip-assert

Add another test

a540b22

ci changes

Minor changes

47e065b

Merge branch 'master' into kanvi/skip-assert

0e3bafe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kanvi/skip assert #265

Kanvi/skip assert #265

kanvi-nervana commented Oct 17, 2018

sayantan-nervana left a comment

sayantan-nervana Oct 17, 2018

shresthamalik Oct 18, 2018 •

edited

sayantan-nervana Oct 18, 2018

kanvi-nervana Oct 18, 2018

shresthamalik Oct 18, 2018

shresthamalik Oct 18, 2018

avijit-nervana Oct 18, 2018

kanvi-nervana Oct 18, 2018

avijit-nervana Oct 18, 2018

avijit-nervana Oct 18, 2018

avijit-nervana left a comment

sayantan-nervana left a comment •

edited

shresthamalik left a comment

shresthamalik Oct 18, 2018

shresthamalik left a comment

Kanvi/skip assert #265

Are you sure you want to change the base?

Kanvi/skip assert #265

Conversation

kanvi-nervana commented Oct 17, 2018

sayantan-nervana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shresthamalik Oct 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avijit-nervana left a comment

Choose a reason for hiding this comment

sayantan-nervana left a comment • edited

Choose a reason for hiding this comment

shresthamalik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shresthamalik left a comment

Choose a reason for hiding this comment

shresthamalik Oct 18, 2018 •

edited

sayantan-nervana left a comment •

edited