basic scope implementation #2629

jacquesqiao · 2017-06-27T15:41:11Z

Basic implementation of Scope.

… scope-impl

wangkuiyi · 2017-06-28T00:37:22Z

paddle/framework/scope.h

@@ -0,0 +1,50 @@
+#pragma once


Need copyright information

wangkuiyi · 2017-06-28T00:37:31Z

paddle/framework/scope.cc

@@ -0,0 +1,55 @@
+#include "paddle/framework/scope.h"


Need copyright information.

wangkuiyi · 2017-06-28T00:43:30Z

paddle/framework/scope.h

+
+  // Create Variable in this Scope. Return error if Variable already been
+  // created.
+  Error __must_check CreateVariable(const std::string& name);


I checked the definition of __must_check. It means "must use the returned value", and has nothing about "check".

Also, according to Google C++ style, a macro should be all captital, e.g., MUST_USED_RETVALUE. It breaks the style.

Should we remove paddle/platform/must_check.h?

ok, I will remove __must_check and return Variable directly

wangkuiyi · 2017-06-28T00:47:36Z

paddle/framework/scope.h

+
+  // Create Variable in this Scope. Return error if Variable already been
+  // created.
+  Error __must_check CreateVariable(const std::string& name);


I checked Caffe2's source code of the similar functionality at here:

Blob* Workspace::CreateBlob(const string& name) { if (HasBlob(name)) { VLOG(1) << "Blob " << name << " already exists. Skipping."; } else { VLOG(1) << "Creating blob " << name; blob_map_[name] = unique_ptr<Blob>(new Blob()); } return GetBlob(name); }

I am impressed that Caffe2's authors tried to handle the error as much as they could inside the function definition. Whereas, we simply return an error and use __must_check to require users of our functions to handle the error. The best place to handle the error is at where it happens, where we have the most sufficient information to recover, to report, or to fail.

I'd suggest that we use:

Variable* CreateVar(const std::string& name);

To give an accurate semantics, I prefer to provide two interfaces:

CreateVariable will check and fail if there already have a variable with the same name in the scope.

GetOrCreateVariable is used to get a variable anyway.
Currently, I think the user should know exactly what they want.

Good argue!

Let us think about this following the first principle.

What users need are

Create

Get

Create could err if there has been a variable with the same name there. We can do either of the following things with this error:

we do nothing

return the error

A basic design principle is that we handle the error as much as we could at where it happens, and try not to return to the users, because the users knows nothing about the detail and cannot really do much with this error. Also, if the variable is already there, users' intention (create the variable) is fulfilled, it doesn't worthy of returning an error. So I prefer Create takes no-op with the duplication error.

Similarly, Get could err if it cannot find the variable, and we can take the following possible actions:

no-op

return error

create the variable

When the user Gets a variable, it assumes that the variable should have been there. If it has not, it implies that the creation process failed or we were creating a wrong network description. Anyway, there do have an error so we cannot take no-op. Similarly, if we create the variable, we hide the error; this is not right. So we must report error. Fortunately, reporting the error is simple -- just return nullptr would be enough.

Summarize above derivation, I think we should have

CreateVar, which does nothing if there has been the variable, and

Get, which returns nullptr if the variable isn't there.

if the variable is already there, users' intention (create the variable) is fulfilled, there is really nothing worth of returning.

This line convinced me, thanks! I will try to learn more about the first principle.

wangkuiyi · 2017-06-28T00:52:01Z

paddle/framework/scope.h

+  Variable* GetVariable(const std::string& name) const;
+
+  // find and return Variables in the scope it self.
+  Variable* GetVarLocally(const std::string& name) const;


In what situation we need GetVarLocally instead of GetVaraible?

wangkuiyi · 2017-06-28T00:52:29Z

paddle/framework/scope.h

+
+  // Get a Variable from Scope, if the Variable is not exist then create it.
+  // User should call this function most of time.
+  Variable* GetOrCreateVariable(const std::string& name);


How about merge CreateVariable and GetOrCreateVariable into one -- CreateVar?

To give an accurate semantics, I prefer to provide two interfaces:

CreateVariable will check and fail if there already have a variable with the same name in the scope.

GetOrCreateVariable is used to get a variable anyway.
Currently, I think the user should know exactly what they want.

wangkuiyi · 2017-06-28T00:53:25Z

paddle/framework/scope.h

+  // User should call this function most of time.
+  Variable* GetOrCreateVariable(const std::string& name);
+
+  bool HaveVariable(const std::string& name);


Have => Has, because the implied subject is "a workspace", not "workspaces" or something like that.

wangkuiyi · 2017-06-28T00:59:29Z

paddle/framework/scope.h

+#include <unordered_map>
+#include <vector>
+#include "paddle/framework/variable.h"
+#include "paddle/utils/Error.h"


I don't see that this header file needs a special Error type. I think if we follow below comments, we can remove this inclusion.

reyoung · 2017-06-28T06:15:13Z

paddle/framework/scope.cc

+namespace framework {
+
+Variable* Scope::CreateVariable(const std::string& name) {
+  if (!HasVariable(name)) {


If HasVariable, just ASSERT_FALSE is better.

And local scope could create a variable that uses the same name in the parent. It means the local variable can hide parent variable.

void CreateVariable(const std::string& name) { auto it = vars_.find(name); ASSERT_EQ(it, vars_.end()); vars_[name] = std::unique_ptr<Variable>(new Variable()); }

In our design, CreateVariable is equal to GetOrCreateVariable

Maybe we should implement Get, Create by it means, and implement another method name GetOrCreateVariable().

I prefer a concise interface. It takes the users only two lines to implement GetOrCreateVariable:

const Variable* v = ws.GetVar("something"); if (v == nullptr) ws.CreateVar("something"); // do something with v.

doesn't differ much from

const Variable* v = ws.GetOrCreateVar("something"); // do something with v.

reyoung · 2017-06-28T06:16:01Z

paddle/framework/scope.cc

+}
+
+Variable* Scope::GetVarLocally(const std::string& name) const {
+  if (vars_.count(name)) {


vars_.count is not good.

auto it = vars_.find(name); if (it != vars_.end()) { return it->second.get(); } return nullptr;

ok, It's better to use find than count. Thanks for reminding!

reyoung · 2017-06-28T06:18:50Z

paddle/framework/scope.cc

+  }
+}
+
+bool Scope::HasVariable(const std::string &name) {


HasVariable() const

reyoung

I think we could implement Scope in the header only. Not split them into cpp files.

Because function in the header could be inlined, but in cpp, the function cannot be inlined.

The scope function is simple and should be inlined.

reyoung · 2017-06-28T06:24:48Z

paddle/framework/scope.h

+ */
+class Scope {
+ public:
+  Scope() {}


Scope() = default;

reyoung · 2017-06-28T06:25:08Z

paddle/framework/scope.h

+
+  explicit Scope(const std::shared_ptr<Scope>& scope) : parent_(scope) {}
+
+  ~Scope() {}


Destructor is not needed.

reyoung · 2017-06-28T06:25:39Z

paddle/framework/scope.h

+ public:
+  Scope() {}
+
+  explicit Scope(const std::shared_ptr<Scope>& scope) : parent_(scope) {}


Maybe a single ctor is enough.

And ctor of Scope should be private. There should be another method create std::shared_ptr<Scope> like:

private: Scope(const std::shared_ptr<Scope>& parent): parent_(parent) {} public: static std::shared_ptr<Scope> Create(const std::shared_ptr<Scope>& parent=nullptr) { return std::make_shared<Scope>(parent); }

not quite understand the reason to always create a shared_prt?

Why not the simplest way:

class Scope { public: Scope(Scope* parent) : parent_(parent) {} private: std::shared_ptr<Scope> parent_; };

Because we want user to use Scope as shared_ptr(std::shared_ptr parent_), a create function will do this for user.

Done， remove default argument

reyoung · 2017-06-28T06:28:08Z

paddle/framework/scope.h

+
+  ~Scope() {}
+
+  // Create Variable in this Scope. Return error if Variable already been


Use '///' or '/**', see doxygen style

reyoung · 2017-06-28T13:11:47Z

paddle/framework/scope.h

+ public:
+  static std::shared_ptr<Scope> Create(
+      const std::shared_ptr<Scope>& parent = nullptr) {
+    return std::make_shared<Scope>(Scope(parent));


std::make_shared<Scope>(parent); should enough.

According to https://google.github.io/styleguide/cppguide.html#Default_Arguments, we shouldn't use default argument here.

My proposal in #2629 (comment) seems OK.

it will fail if the default consturctor is private:

In file included from /Users/qiaolongfei/project/paddle/paddle/framework/scope_test.cc:15: In file included from /Users/qiaolongfei/project/paddle/paddle/framework/scope.h:17: In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/string:439: In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/algorithm:628: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2194:15: error: field of type 'paddle::framework::Scope' has private default constructor __second_(_VSTD::forward<_Args2>(_VSTD::get<_I2>(__second_args))...) ^ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2457:15: note: in instantiation of function template specialization 'std::__1::__libcpp_compressed_pair_imp<std::__1::allocator<paddle::framework::Scope>, paddle::framework::Scope, 1>::__libcpp_compressed_pair_imp<std::__1::allocator<paddle::framework::Scope> &, const std::__1::shared_ptr<paddle::framework::Scope> &, 0, 0>' requested here : base(__pc, _VSTD::move(__first_args), _VSTD::move(__second_args), ^ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:3800:16: note: in instantiation of function template specialization 'std::__1::__compressed_pair<std::__1::allocator<paddle::framework::Scope>, paddle::framework::Scope>::__compressed_pair<std::__1::allocator<paddle::framework::Scope> &, const std::__1::shared_ptr<paddle::framework::Scope> &>' requested here : __data_(piecewise_construct, _VSTD::forward_as_tuple(__a), ^ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:4411:26: note: in instantiation of function template specialization 'std::__1::__shared_ptr_emplace<paddle::framework::Scope, std::__1::allocator<paddle::framework::Scope> >::__shared_ptr_emplace<const std::__1::shared_ptr<paddle::framework::Scope> &>' requested here ::new(__hold2.get()) _CntrlBlk(__a2, _VSTD::forward<_Args>(__args)...); ^ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:4775:29: note: in instantiation of function template specialization 'std::__1::shared_ptr<paddle::framework::Scope>::make_shared<const std::__1::shared_ptr<paddle::framework::Scope> &>' requested here return shared_ptr<_Tp>::make_shared(_VSTD::forward<_Args>(__args)...); ^ /Users/qiaolongfei/project/paddle/paddle/framework/scope.h:41:17: note: in instantiation of function template specialization 'std::__1::make_shared<paddle::framework::Scope, const std::__1::shared_ptr<paddle::framework::Scope> &>' requested here return std::make_shared<Scope>(parent); ^ /Users/qiaolongfei/project/paddle/paddle/framework/scope.h:35:12: note: declared private here explicit Scope(const std::shared_ptr<Scope>& parent = nullptr) ^ 1 error generated. make[2]: *** [paddle/framework/CMakeFiles/scope_test.dir/scope_test.cc.o] Error 1 make[1]: *** [paddle/framework/CMakeFiles/scope_test.dir/all] Error 2

default argument removed

reyoung · 2017-06-28T13:13:39Z

paddle/framework/scope.h

+  /// Create Variable in this Scope. Failed if Variable already been
+  /// created.
+  Variable* CreateVariable(const std::string& name) {
+    PADDLE_ASSERT(!HasVariable(name));


Should local scope hide parent scope variable?

If so, then here should be

PADDLE_ASSERT(vars_.find(name) == vars_.end());

I am not sure why we need to hide parent scope variable

wangkuiyi · 2017-06-29T03:15:52Z

I approved because my questions were addressed. Please feel free to continue the discussion.

… scope-impl

* update readme en, test=document_fix * update doc

scope-impl

2f47562

jacquesqiao requested review from reyoung, wangkuiyi and kuke June 27, 2017 15:41

jacquesqiao changed the title ~~scope-impl~~ basic scope implementation Jun 27, 2017

jacquesqiao added 3 commits June 27, 2017 23:53

refine GetOrCreateVariable

5e8d8e0

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

fa4f00d

… scope-impl

refine code of scope with style check

7d13859

wangkuiyi requested changes Jun 28, 2017

View reviewed changes

This was referenced Jun 28, 2017

Adding platform/must_check.h #2600

Merged

Remove paddle/platform/must_check.h #2632

Closed

wangkuiyi reviewed Jun 28, 2017

View reviewed changes

wangkuiyi mentioned this pull request Jun 28, 2017

Feature/add proto attrs #2604

Closed

refine code of operator

6ad1d21

reyoung reviewed Jun 28, 2017

View reviewed changes

clang format

3e16f5f

reyoung reviewed Jun 28, 2017

View reviewed changes

paddle/framework/scope.cc Outdated

}

}

bool Scope::HasVariable(const std::string &name) {

Copy link

Collaborator

reyoung Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HasVariable() const

reyoung requested changes Jun 28, 2017

View reviewed changes

reyoung reviewed Jun 28, 2017

View reviewed changes

jacquesqiao added 2 commits June 28, 2017 15:57

move code to scope.h, remove scope.cc

b8ffa8b

add Create for scope

1678ad7

reyoung reviewed Jun 28, 2017

View reviewed changes

wangkuiyi approved these changes Jun 29, 2017

View reviewed changes

merge CreateVar and GetOrCreateVar

aabe171

jacquesqiao added 3 commits June 29, 2017 13:38

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

cfdfa89

… scope-impl

do not use default argument in Scope

9af54c9

update design doc

3c925fe

reyoung approved these changes Jun 29, 2017

View reviewed changes

jacquesqiao merged commit 590795d into PaddlePaddle:develop Jun 29, 2017

jacquesqiao added this to Done in PaddlePaddle Refactoring: Phase 1 Aug 2, 2017

heavengate pushed a commit to heavengate/Paddle that referenced this pull request Aug 16, 2021

update readme en, test=document_fix (PaddlePaddle#2629)

6e7e13f

* update readme en, test=document_fix * update doc


		explicit Scope(const std::shared_ptr<Scope>& scope) : parent_(scope) {}

		~Scope() {}


		~Scope() {}

		// Create Variable in this Scope. Return error if Variable already been

basic scope implementation #2629

basic scope implementation #2629

Conversation

jacquesqiao commented Jun 27, 2017 • edited Loading

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

wangkuiyi Jun 29, 2017 • edited Loading

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

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

reyoung 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

reyoung Jun 28, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkuiyi commented Jun 29, 2017

jacquesqiao commented Jun 27, 2017 •

edited

Loading

wangkuiyi Jun 29, 2017 •

edited

Loading

reyoung Jun 28, 2017 •

edited

Loading