A simple solution to work on the copy of the graph #15

sharanry · 2018-07-25T06:12:11Z

No description provided.

ferrine · 2018-07-25T06:26:03Z

Tests seem to be broken, do they pass localy? Solution looks nice, are there any corner cases? What's happening when we use nested models?

ferrine · 2018-07-25T06:28:17Z

Also what if self._cfg contains tf variables?

sharanry · 2018-07-25T06:39:12Z

@ferrine, some problems with using the the right graph with Session(), that is why test is failing. Fixing it.

self._cfg contains the model configuration passed either during model initialization or using model.configure()

ColCarroll · 2018-07-25T07:31:22Z

Looks nice - once tests are fixed, it might be worth wrapping this in its own decorator or method.

ColCarroll · 2018-07-25T07:31:49Z

pymc4/model/base.py

-            with ed.interception(interceptor):
+            temp_graph = tf.Graph()
+            tf.contrib.graph_editor.copy(self.graph, temp_graph)
+            with temp_graph.as_default(), ed.interception(interceptor):
                self._f(self._cfg)


not related to this PR, but this line should probably be self._f(self.cfg), right?

IMO it shouldnt make a difference. self.cfg returns self._cfg. Although I am not sure why we are saving it as _cfg

sharanry · 2018-07-25T09:06:52Z

The CI is failing because they removed tf-nightly==1.8.0.dev20180331. Need to update the requirements.

coveralls · 2018-07-25T14:19:02Z

Pull Request Test Coverage Report for Build 53

18 of 18 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 85.0%

Totals
Change from base Build 42:	0.4%
Covered Lines:	204
Relevant Lines:	240

💛 - Coveralls

ColCarroll

sorry for late review!

ColCarroll · 2018-07-29T09:19:16Z

pymc4/model/base.py

@@ -38,6 +38,7 @@ def __init__(self, name=None, graph=None, session=None, **config):
            session = tf.Session(graph=graph)
        self.session = session
        self.observe(**config)
+        self.temp_graph = tf.Graph()


I would use this pattern for creating and destroying the temporary graph. I'm using a pymc3 model here because I think the pattern is similar, but let me know if it isn't! Note that you will only need to implement the equivalent of the temp_model method below.

import pymc3 as pm def check_in_model_context(): try: m = pm.modelcontext(None) return f'In model context! {len(m.vars)} vars!' except TypeError: return 'Not in model context!' class Model(object): def __init__(self): self._temp_model = None @contextmanager def temp_model(self): self._temp_model = pm.Model() try: with self._temp_model: yield finally: self._temp_model = None def calculation(self): print(check_in_model_context()) with self.temp_model(): pm.Normal('x', 0, 1) print(check_in_model_context()) print(check_in_model_context()) with self.temp_model(): print(check_in_model_context()) print(check_in_model_context())

decorator for inclass function is causing problems.. I will look into

ColCarroll · 2018-07-29T09:19:45Z

pymc4/util/interceptors.py

@@ -12,7 +12,7 @@
 VariableDescription = collections.namedtuple('VariableDescription', 'Dist,shape,rv')


-class Interceptor(object):
+class Interceptor():


good practice to inherit from object, even if you don't need to!

Why? I thought in py3 we don't need that anymore.

Yes, I am facing a linting error when I import from object

ferrine · 2018-07-29T20:39:59Z

Explicit is better than implicit вс, 29 июл. 2018 г., 15:03 Thomas Wiecki <notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In pymc4/util/interceptors.py <#15 (comment)>: > @@ -12,7 +12,7 @@ VariableDescription = collections.namedtuple('VariableDescription', 'Dist,shape,rv') -class Interceptor(object): +class Interceptor(): Why? I thought in py3 we don't need that anymore. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALKb7jmrRGmUrnjPtKeGaI4wIWyTBUlkks5uLaSNgaJpZM4Vffeo> .

twiecki · 2018-07-30T08:15:24Z

Explicit is better than implicit

There is nothing explicit about this, it's a python 2 hack to get super working (or something like that).

ColCarroll · 2018-07-30T08:27:13Z

Sort of a jokey discussion, but some nuggets of wisdom in there, notably that there aren't strong feelings either way:
https://forum.dabeaz.com/t/inheriting-from-object/161

I'm fine leaving it out.

ferrine

The proposed approach has important corner cases not supported

ferrine · 2018-08-02T07:34:11Z

pymc4/model/base.py

@@ -54,6 +55,7 @@ def _init_variables(self):
        info_collector = interceptors.CollectVariablesInfo()
        with self.graph.as_default(), ed.interception(info_collector):
            self._f(self.cfg)
+        tf.contrib.graph_editor.copy(self.graph, self.temp_graph)


So 1) You create a temp graph for inner manipulations

ferrine · 2018-08-02T07:42:30Z

pymc4/model/base.py

-        with self.graph.as_default(), ed.interception(interceptors.Chain(*chain)):
+        tf.contrib.graph_editor.copy(self.graph, self.temp_graph)
+        # pylint: disable=not-context-manager
+        with self.temp_graph.as_default(), ed.interception(interceptors.Chain(*chain)):


You reuse tmp graph created on step 1) suppose cfg has any tensor from original graph and you call your model with temp_graph.as_default(). That creates all variables for temp_graph. This looks fine unless the case I'm talking about. If you attempt to do an operation on two variables from different graphs, you will get an exception

I do not know a solution for this case yet. I've seen some kind of working and relevant example in tensorflow codebase
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/framework/function.py#L347

twiecki · 2019-01-05T13:52:00Z

Closing as the main prototype is from the london branch.

keep a graph copy solution

b218d43

sharanry requested review from ColCarroll and ferrine July 25, 2018 06:12

sharanry changed the base branch from master to functional July 25, 2018 06:12

clear temp_graph

88fc3c6

sharanry changed the title ~~A simple solution to work on the copy of the graph~~ [WIP] A simple solution to work on the copy of the graph Jul 25, 2018

ColCarroll reviewed Jul 25, 2018

View reviewed changes

fix graph

8136428

sharanry added 4 commits July 25, 2018 14:44

update requirements.txt

3fd3acc

clean up

0032173

fix pylint errors

f3b6715

fix pycodestyle errors

72cb7ac

sharanry changed the title ~~[WIP] A simple solution to work on the copy of the graph~~ A simple solution to work on the copy of the graph Jul 26, 2018

ColCarroll reviewed Jul 29, 2018

View reviewed changes

ferrine reviewed Aug 2, 2018

View reviewed changes

twiecki closed this Jan 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A simple solution to work on the copy of the graph #15

A simple solution to work on the copy of the graph #15

sharanry commented Jul 25, 2018

ferrine commented Jul 25, 2018

ferrine commented Jul 25, 2018

sharanry commented Jul 25, 2018

ColCarroll commented Jul 25, 2018

ColCarroll Jul 25, 2018

sharanry Jul 25, 2018

sharanry commented Jul 25, 2018

coveralls commented Jul 25, 2018

ColCarroll left a comment

ColCarroll Jul 29, 2018

sharanry Aug 1, 2018

ColCarroll Jul 29, 2018

twiecki Jul 29, 2018

sharanry Jul 30, 2018

ferrine commented Jul 29, 2018 via email

twiecki commented Jul 30, 2018

ColCarroll commented Jul 30, 2018

ferrine left a comment

ferrine Aug 2, 2018

ferrine Aug 2, 2018 •

edited

Loading

ferrine Aug 2, 2018

twiecki commented Jan 5, 2019

A simple solution to work on the copy of the graph #15

A simple solution to work on the copy of the graph #15

Conversation

sharanry commented Jul 25, 2018

ferrine commented Jul 25, 2018

ferrine commented Jul 25, 2018

sharanry commented Jul 25, 2018

ColCarroll commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharanry commented Jul 25, 2018

coveralls commented Jul 25, 2018

Pull Request Test Coverage Report for Build 53

💛 - Coveralls

ColCarroll left a 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

ferrine commented Jul 29, 2018 via email

twiecki commented Jul 30, 2018

ColCarroll commented Jul 30, 2018

ferrine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferrine Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twiecki commented Jan 5, 2019

ferrine Aug 2, 2018 •

edited

Loading