New destroy handler #6000

ReyhaneAskari · 2017-06-01T19:40:09Z

Issue #5976. Supervisor class is kept but we added a has_destroyers attribute to fgraph, but some tests regarding dumping the pickle failed because a new attribute was added to the fgraph. I changed it like this :

fgraph.destroyers = [get_destroyers_of, has_destroyers]

@nouiz what do you suggest?

fix #6230

lamblin · 2017-06-01T21:00:24Z

Now that #5794 has been merged, you will probably need to rebase on the master. That will make the diff easier to check.

nouiz · 2017-06-01T21:10:24Z

theano/gof/destroyhandler.py

+            for item in l:
+                droot, _, root_destroyer = self.refresh_droot_impact()
+                try:
+                    [root_destroyer[droot[item]]]


I think you wanted this line to be:

if root_destroyer[droot[item]]: return True

yeah! thanks.

nouiz · 2017-06-01T21:12:04Z

theano/gof/destroyhandler.py

+                try:
+                    [root_destroyer[droot[item]]]
+                    return True
+                except Exception:


I know you copied the Exception from above, but this is a bad practice. We should try to catch a much more precise exception.

Anyway, this isn't the optimized algorythm anyway. Both otherwise, you seem to have done the first part well. If with my modif in the other comment and tests pass, the structural change would be done. It would only miss the optimized implementation here.

You are right! Thanks I changed it to KeyError

ReyhaneAskari · 2017-06-02T14:58:05Z

theano/compile/function_module.py

-        for r in self.protected + list(fgraph.outputs):
-            if fgraph.destroyers(r):
+        if config.cycle_detection == 'fast':
+            if fgraph.destroyers[1](self.protected + list(fgraph.outputs)):


@nouiz , do we need to pass fgraph.outputs here ?

I think so.

nouiz · 2017-06-02T17:51:56Z

theano/compile/function_module.py

+            if fgraph.fast_destroyers_check(self.protected):
+                raise gof.InconsistencyError("Trying to destroy a protected"
+                                             "Variable.")
+


else: return True

nouiz · 2017-06-02T17:57:45Z

theano/gof/destroyhandler.py

+        def recursive_destroys_finder(clients_list):
+            for client in clients_list:
+                # client is a tuple (I don't know if its size is always one)
+                for item in client:


I think it would be more like:

def recursive_destroys_finder(clients_list): for (app, idx) in clients_list: if app == 'output': continue if idx in flatten(getattr(app.op, 'destroy_map', {}).values()): return True for var in app.outputs[getattr(app.op, 'view_map', {}).keys()]: if recursive_destroys_finder(var.clients): return True return False

nouiz · 2017-06-02T19:37:35Z

theano/gof/destroyhandler.py

            for protected_var in protected_list:
-                if recursive_destroys_finder(protected_var.clients):
+                clients_set.update(protected_var.clients)


the clients list of tuple (app, idx) is already a set.

So mostly, that commit don't help. There is some saving possible if one node reuse the same variable multiple time as input, but it happen so infrequently that I don't think the extra complexity is useful.

ReyhaneAskari · 2017-06-05T17:25:29Z

I removed that commit. Now we need to check the failing tests and make sure they are the same tests that are failing in the master with THEANO_FLAGS=cycle_detetion='fast'. @lamblin suggested that I mark the failing tests in master as nosetests' known failures.

nouiz · 2017-06-05T17:57:26Z

Just to be sure, make them as known failure only when cycle='fast'

…

On Mon, Jun 5, 2017 at 1:25 PM Reyhane Askari ***@***.***> wrote: I removed that commit. Now we need to check the failing tests and make sure they are the same tests that are failing in the master with THEANO_FLAGS=cycle_detetion='fast'. @lamblin <https://github.com/lamblin> suggested that I mark the failing tests in master as nosetests' known failures. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6000 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALC--UcopxR5lhHFzJjFo5wzs1_ggz3ks5sBDoLgaJpZM4Ntcs-> .

nouiz · 2017-06-05T17:58:24Z

theano/gof/destroyhandler.py

+        def has_destroyers(protected_list):
+            visited_app_set = set()
+
+            def recursive_destroys_finder(clients_list):


I would change the interface to have this function take a variable as input. Then it would loop over var.clients itself. I think it is a nicer interface and will help the conversion to the recursion free version.

Sure. Thanks.

nouiz · 2017-06-05T18:03:22Z

theano/gof/destroyhandler.py

+                    destroy_maps = getattr(app.op, 'destroy_map', {}).values()
+                    if idx in [dmap for sublist in destroy_maps for dmap in sublist]:
+                        return True
+                    for var in getattr(app.op, 'view_map', {}).keys():


Here, this is too much restrictif. We are asking that all view of app are not destroyed. But we only care the outputs that are a view of app.inputs[idx].

I see. Thanks. Is this check good?

for var in getattr(app.op, 'view_map', {}).keys(): if idx in app.op.view_map[var] and recursive_destroys_finder(app.outputs[var]): return True

that seem good. To help read it, I would rename var to var_idx, to tell it is an index, not a variable.

Sure. Thanks.

ReyhaneAskari · 2017-06-05T21:03:34Z

@lamblin, @nouiz I could only find unittest.expectedFailure as a decorator. To make sure it only adds when cycle_detection='fast', I wrote another decorator like this because there were no builtin options:

import unittest
from theano import config

def expectedFailure_fast():
    return unittest.expectedFailure if config.cycle_detection == 'fast' else lambda x: x

@expectedFailure_fast()
def test_usage_loop_through_views_2():

Do you have any suggestions on where to add this code snippet? I was thinking maybe theano.gof.utils?

nouiz · 2017-06-06T14:54:20Z

theano.gof.utils seem the right place for that.

nouiz · 2017-06-06T14:55:02Z

theano/gof/destroyhandler.py

-                        if recursive_destroys_finder(app.outputs[var].clients):
-                            return True
+                        if idx in app.op.view_map[var] and recursive_destroys_finder(app.outputs[var]):
+                                return True


you added an extra indentation that isn't useful.

nouiz · 2017-06-06T14:56:07Z

theano.tests.unittests_tools would be better then theano.gof.utils I think.

lamblin · 2017-06-07T21:17:33Z

Most issues seem to be:

 Error Details

OutputGuard.0 is a view/destroyed version of more then one inputs. Currently, we only support the case where an output is a view or a destroyed version of one input.

 Stack Trace

Traceback (most recent call last):
  File "/miniconda/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/jenkins/workspace/Theano_PR/theano/typed_list/tests/test_basic.py", line 257, in test_inplace
    accept_inplace=True)
  File "/home/jenkins/workspace/Theano_PR/theano/compile/function.py", line 326, in function
    output_keys=output_keys)
  File "/home/jenkins/workspace/Theano_PR/theano/compile/pfunc.py", line 486, in pfunc
    output_keys=output_keys)
  File "/home/jenkins/workspace/Theano_PR/theano/compile/function_module.py", line 1814, in orig_function
    output_keys=output_keys)
  File "/home/jenkins/workspace/Theano_PR/theano/compile/function_module.py", line 1491, in __init__
    insert_deepcopy(fgraph, inputs, outputs + additional_outputs)
  File "/home/jenkins/workspace/Theano_PR/theano/compile/function_module.py", line 1099, in insert_deepcopy
    view_tree_set(alias_root(fgraph.outputs[i]), views_of_output_i)
  File "/home/jenkins/workspace/Theano_PR/theano/compile/function_module.py", line 57, in alias_root
    str(v) + " is a view/destroyed version of more then one inputs. "
NotImplementedError: OutputGuard.0 is a view/destroyed version of more then one inputs. Currently, we only support the case where an output is a view or a destroyed version of one input.

nouiz · 2017-06-08T15:31:52Z

theano/tensor/tests/test_sharedvar.py

@@ -27,6 +27,7 @@ def makeSharedTester(shared_constructor_,
                     theano_fct_,
                     ref_fct_,
                     cast_value_=np.asarray,
+                     need_decorator=True,


Don't forget @lamblin comment in gh-6014.

Yes, I'm on it.

ReyhaneAskari · 2017-06-08T17:52:27Z

There was a small conflict here with jenkins and travis wouldn't run with out resolving it. I resolved the conflict online and committed it. The name was automatically generated as Merge branch 'master' into new_destroy_handler. I will have to rebase in the end and I will clean this up.

ReyhaneAskari · 2017-06-08T19:49:52Z

@lamblin @nouiz
There were so many tests that were failing due to the insertion of OutputGurad. I checked several tests and I figured that we should raise an error when AdddestoryHandler tries to insert the OutputGurad . So we need to check the graph.outputs as well as the protected_var. I also changed the has_destroyer such that in the case when the protected_var is an outputGuard, we do the check.

I was not sure if there are cases where app == 'output' but the protected_var is not an OutputGuard. There are still some failing tests that I'm looking into.

ReyhaneAskari · 2017-06-12T18:07:05Z

There were many failures in the tests because we were inplace in some cases where we shouldn't have been. I checked these tests and found out that we are not raising the error in the Supervisor class in some cases that the inplace was wrong. The problem lied in the case where we had a graph in which one node was cached in the visited_app_set and never checked again.

The mechanism of visited_app_set is wrong. Consider the following case when an apply node is the client of serval variable nodes and has a destroy map of {0 : [1]}. In the first visit to this node since the destroy map is on the second input, we do not raise any error and we mark the apply node as visited while it should be checked for the second variable node as well where it is destroying one of its inputs.

I guess it will be too complicated to support these cases. For now I removed the visited_app_set to see if all the tests pass. Then in the benchmarking we can figure where is the bottleneck.

lamblin · 2017-06-12T19:59:30Z

Conclusion of IRL discussion:

remove view_map from OutputGuard
In AddDestroyHandler, do not try to introduce OutputGuard when fast cycle detection is on
undo the new check (full=True).

ReyhaneAskari · 2017-06-13T14:51:54Z

We won't remove the view_map for the output Guard for now since it's causing many errors. We will handle that in another PR where we'll completely remove the output Guard.

ReyhaneAskari · 2017-06-13T18:17:08Z

Is there a way to add the flag to the examples inside the docs? Maybe the easier way to change the cycle_detection flag was to change the default value to fast for the testing purposes and revert it back in the end.

nouiz · 2017-06-13T18:19:01Z

Doing your suggestion would have done it.

…

On Tue, Jun 13, 2017 at 2:17 PM Reyhane Askari ***@***.***> wrote: Is there a way to add the flag to the examples inside the docs? Maybe the easier way to change the cycle_detection flag was to change the default value to fast for the testing purposes and revert it back in the end. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6000 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALC-68x2ky6NgOogQxIMwQgvETlSxufks5sDtIngaJpZM4Ntcs-> .

…fast

…ure_fast

…raph.

…regression.

nouiz reviewed Jun 1, 2017

View reviewed changes

ReyhaneAskari force-pushed the new_destroy_handler branch from 5728dbf to e9597e1 Compare June 2, 2017 14:44

ReyhaneAskari commented Jun 2, 2017

View reviewed changes

ReyhaneAskari force-pushed the new_destroy_handler branch from e9597e1 to 590f2f6 Compare June 2, 2017 17:49

nouiz requested changes Jun 2, 2017

View reviewed changes

nouiz reviewed Jun 2, 2017

View reviewed changes

ReyhaneAskari force-pushed the new_destroy_handler branch from 95d9429 to 6090615 Compare June 5, 2017 17:21

nouiz requested changes Jun 5, 2017

View reviewed changes

nouiz reviewed Jun 6, 2017

View reviewed changes

ReyhaneAskari force-pushed the new_destroy_handler branch from dd15fb3 to ef610c1 Compare June 6, 2017 15:29

ReyhaneAskari mentioned this pull request Jun 6, 2017

fast cycle detection flag added #6014

Closed

nouiz requested changes Jun 8, 2017

View reviewed changes

ReyhaneAskari force-pushed the new_destroy_handler branch 2 times, most recently from 1cc937e to 5c79a2f Compare June 8, 2017 19:40

ReyhaneAskari force-pushed the new_destroy_handler branch 2 times, most recently from f072886 to 7e4a569 Compare June 9, 2017 21:53

ReyhaneAskari added 15 commits August 1, 2017 17:02

replaced get_destroyers and fgraph.destroyers with has_destroyers

9ad8e92

updated docstring

0a652bb

change cycle_detection to fast

c26cf06

minor fix

9e4514e

expectedFailure_fast added to 5 tests

ecaa310

minor fix

dccc8e5

removed check for output guard in debug mode when cycle_detection is …

5471309

…fast

changed expectedFailure to assertFailure

83a8b1f

minor fix

ad231af

dummy commit

9426922

added assertFailure_fast to some tests and minor change in assertFail…

5dbdadf

…ure_fast

reverted assertFailure_fast change

b8951ba

flake8

d9d64a4

fix for assertFailure_fast

2ff1c2d

reverted the cycle_detection to regular

216beeb

ReyhaneAskari force-pushed the new_destroy_handler branch from b8cfafb to 216beeb Compare August 1, 2017 21:13

ReyhaneAskari and others added 4 commits August 2, 2017 11:29

removed checkfor outputguard for regular cycle detection

e747bbf

Use the big structure when it is there to prevent investigating the g…

5e0cc2b

…raph.

updated doc for fast cycle detection

2f3f2e5

Fix doc warning

d9d7c01

nouiz mentioned this pull request Aug 8, 2017

updated doc for fast cycle detection #6265

Closed

Catch the good exception.

66c0c86

abergeron approved these changes Aug 8, 2017

View reviewed changes

nouiz approved these changes Aug 9, 2017

View reviewed changes

nouiz added 2 commits August 9, 2017 15:12

Try to fix travis speed regression in the default case.

27dcba5

Try to fix Travis slow down. This also make sure to don't have speed …

f03092a

…regression.

abergeron merged commit e082176 into Theano:master Aug 14, 2017

This was referenced Aug 15, 2017

Merge Supervisor Feature inside the DestroyHandler #5976

Closed

fast destroy follow up #6310

Open

nouiz mentioned this pull request Aug 24, 2017

By default, do validation during elemwise_inplace_optimizer approx 10… #5124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New destroy handler #6000

New destroy handler #6000

ReyhaneAskari commented Jun 1, 2017 •

edited by nouiz

lamblin commented Jun 1, 2017

nouiz Jun 1, 2017

ReyhaneAskari Jun 2, 2017

nouiz Jun 1, 2017

ReyhaneAskari Jun 2, 2017

ReyhaneAskari Jun 2, 2017

nouiz Jun 2, 2017

nouiz Jun 2, 2017

nouiz Jun 2, 2017

nouiz Jun 2, 2017

nouiz Jun 2, 2017

ReyhaneAskari commented Jun 5, 2017

nouiz commented Jun 5, 2017 via email

nouiz Jun 5, 2017

ReyhaneAskari Jun 5, 2017

nouiz Jun 5, 2017

ReyhaneAskari Jun 5, 2017

nouiz Jun 6, 2017

ReyhaneAskari Jun 6, 2017

ReyhaneAskari commented Jun 5, 2017

nouiz commented Jun 6, 2017

nouiz Jun 6, 2017

nouiz commented Jun 6, 2017

lamblin commented Jun 7, 2017

nouiz Jun 8, 2017

ReyhaneAskari Jun 8, 2017

ReyhaneAskari commented Jun 8, 2017 •

edited

ReyhaneAskari commented Jun 8, 2017

ReyhaneAskari commented Jun 12, 2017 •

edited

lamblin commented Jun 12, 2017

ReyhaneAskari commented Jun 13, 2017

ReyhaneAskari commented Jun 13, 2017

nouiz commented Jun 13, 2017 via email

New destroy handler #6000

New destroy handler #6000

Conversation

ReyhaneAskari commented Jun 1, 2017 • edited by nouiz

lamblin commented Jun 1, 2017

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

Choose a reason for hiding this comment

ReyhaneAskari commented Jun 5, 2017

nouiz commented Jun 5, 2017 via email

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

ReyhaneAskari commented Jun 5, 2017

nouiz commented Jun 6, 2017

Choose a reason for hiding this comment

nouiz commented Jun 6, 2017

lamblin commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReyhaneAskari commented Jun 8, 2017 • edited

ReyhaneAskari commented Jun 8, 2017

ReyhaneAskari commented Jun 12, 2017 • edited

lamblin commented Jun 12, 2017

ReyhaneAskari commented Jun 13, 2017

ReyhaneAskari commented Jun 13, 2017

nouiz commented Jun 13, 2017 via email

ReyhaneAskari commented Jun 1, 2017 •

edited by nouiz

ReyhaneAskari commented Jun 8, 2017 •

edited

ReyhaneAskari commented Jun 12, 2017 •

edited