add executor.prepare #9022

jacquesqiao · 2018-03-13T08:25:30Z

No description provided.

reyoung · 2018-03-13T09:39:26Z

paddle/fluid/pybind/pybind.cc

+            return static_cast<void *>(Executor::Prepare(pdesc, block_id));
+          },
+          py::return_value_policy::reference)
+      .def_static("delete_prepared_ctx",


This method is not needed.

We can just return ExecutorPrepareContext in prepare method, and make Python delete this object.

@reyoung Ok, I use copy to return the void * now:

/** Create a new copy of the returned object, which will be owned by Python. This policy is comparably safe because the lifetimes of the two instances are decoupled. */ copy,

I am not sure how will Python manage this returned pointer, can you give some information?

The default policy is OK

… use-executor-prepare

reyoung · 2018-03-16T06:36:05Z

paddle/fluid/pybind/pybind.cc

           });

+  py::class_<ExecutorPrepareContext>(m, "ExecutorPrepareContext")
+      .def("__init__", [](ExecutorPrepareContext &instance, ProgramDesc &desc,


It seems that __init__ method is not needed

reyoung · 2018-03-16T06:36:16Z

paddle/fluid/pybind/pybind.cc

+                     int block_id) -> std::unique_ptr<ExecutorPrepareContext> {
+                    return Executor::Prepare(pdesc, block_id);
+                  },
+                  py::return_value_policy::automatic)


Default policy is OK

panyx0718

high level question: The plan is to keep prepare as private method called within run, right?

panyx0718 · 2018-03-19T00:35:54Z

paddle/fluid/pybind/pybind.cc

+
  py::class_<framework::Executor>(m, "Executor")
      .def(py::init<const platform::Place &>())
+      .def_static("prepare",


I think the visibility can be controlled on the Python side.

panyx0718 · 2018-03-19T00:36:56Z

paddle/fluid/pybind/pybind.cc

+                     int block_id) -> std::unique_ptr<ExecutorPrepareContext> {
+                    return Executor::Prepare(pdesc, block_id);
+                  })
+      .def("run_prepared_ctx",


_run_prepared_ctx

same as above

panyx0718 · 2018-03-19T00:37:25Z

python/paddle/fluid/executor.py

+        self.program = program
+        self.fetch_list = fetch_list
+        self.feed_var_name = feed_var_name
+        self.fetch_var_name = fetch_var_name


Are all of them public members?

Remove this class if it's no longer used?

panyx0718 · 2018-03-19T00:38:37Z

python/paddle/fluid/executor.py


+    def _get_program_cache(self, feed, fetch_list):
+        program_cache_key = get_program_cache_key(feed, fetch_list)
+        program_cache = self.program_caches.get(program_cache_key, None)


nit: return at this line?

panyx0718 · 2018-03-19T00:41:47Z

python/paddle/fluid/executor.py

+
+        return tmp_program
+
+    def feed_data(self, program, feed, feed_var_name, scope):


private member?

panyx0718 · 2018-03-19T00:42:50Z

python/paddle/fluid/executor.py

+        ]
+        return outs
+
+    def prepare(self,


private member?

panyx0718 · 2018-03-19T00:43:35Z

python/paddle/fluid/executor.py

+        return PreparedContext(handle, program, fetch_list, feed_var_name,
+                               fetch_var_name)
+
+    def run_prepared_ctx(self, ctx, feed=None, scope=None, return_numpy=True):


panyx0718 · 2018-03-19T00:44:48Z

python/paddle/fluid/tests/unittests/test_executor_and_mul.py

+        exe = Executor(place)
+        feed = {'a': a_np, 'b': b_np, 'c': c_np}
+
+        prepared_ctx = exe.prepare(feed=feed, fetch_list=[out])


do you plan to expose prepare as a public member?

yes, prepare should be public, because a user should use it directly.

panyx0718 · 2018-03-19T05:22:03Z

python/paddle/fluid/executor.py

-            core.get_fetch_variable(scope, fetch_var_name, i)
-            for i in xrange(len(fetch_list))
-        ]
+            program = self._add_feed_fetch_ops(


why is it no longer poping the cached program here?

done, add back.

panyx0718 · 2018-03-19T05:25:51Z

python/paddle/fluid/executor.py

+        ]
+        return outs
+
+    def _prepare(self,


Is _prepare still called in python? If not, we can remove it

For now, it can be used for the unit test, we can delete it future when we implement the right program version and cache in the CPP side.

I think we can test it without using _prepare? if we don't use_program_cache, the run will give us up-to-date result, if we do use program_cache, the executor will give us stale result?

If not necessary, I would prefer to avoid exposing _prepare to python.

…version is added

panyx0718 · 2018-03-20T07:55:35Z

python/paddle/fluid/executor.py

+        self.program = program
+        self.fetch_list = fetch_list
+        self.feed_var_name = feed_var_name
+        self.fetch_var_name = fetch_var_name


Remove this class if it's no longer used?

… use-executor-prepare

jacquesqiao added 3 commits March 13, 2018 16:24

add executor.prepare

3980011

change class to struct

9b4bd6b

add delete_prepared_ctx and optimize code

3d90b07

jacquesqiao requested a review from reyoung March 13, 2018 09:22

reyoung reviewed Mar 13, 2018

View reviewed changes

jacquesqiao added 6 commits March 14, 2018 10:39

rm delete_prepared_ctx, lete Python manage the object

efac71b

return pointer with type ExecutorPrepareContext

c54d5f8

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

82ec018

… use-executor-prepare

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

e7eab76

… use-executor-prepare

delete ununsed private field

7241a52

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

582659a

… use-executor-prepare

reyoung reviewed Mar 16, 2018

View reviewed changes

jacquesqiao added 2 commits March 16, 2018 14:44

rm unused code

7590242

rm unused code

c682ed2

jacquesqiao requested a review from panyx0718 March 16, 2018 08:54

panyx0718 reviewed Mar 19, 2018

View reviewed changes

jacquesqiao added 2 commits March 19, 2018 09:51

optimize interface of executor.py

56d5c61

make prepare private

8190ff3

panyx0718 reviewed Mar 19, 2018

View reviewed changes

jacquesqiao added 2 commits March 19, 2018 14:03

add program_caches.pop

04518af

rm the expose of executor.prepare, should use cpp cache when program.…

b873486

…version is added

panyx0718 previously approved these changes Mar 20, 2018

View reviewed changes

PreparedContext

31aca56

jacquesqiao dismissed panyx0718’s stale review via 31aca56 March 20, 2018 10:02

jacquesqiao added 2 commits March 20, 2018 19:15

revert code

9fb33bf

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

b938180

… use-executor-prepare

panyx0718 approved these changes Mar 20, 2018

View reviewed changes

jacquesqiao merged commit 37a272e into PaddlePaddle:develop Mar 20, 2018


		return tmp_program

		def feed_data(self, program, feed, feed_var_name, scope):

add executor.prepare #9022

add executor.prepare #9022

Uh oh!

Conversation

jacquesqiao commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacquesqiao Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panyx0718 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panyx0718 Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jacquesqiao Mar 14, 2018 •

edited

Loading

panyx0718 Mar 20, 2018 •

edited

Loading