-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
scope design doc #2548
scope design doc #2548
Conversation
doc/design/scope.md
Outdated
|
||
private: | ||
/// variable name -> variable | ||
std::unordered_map<std::string, VariablePtr> vars_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why unordered_map
other than map
? I am not sure which is better -- I'd always believed the former is a hash map and the latter is an RD-tree.
Anyway, I noticed that Caffe2 is using map
in its Workspace:
private:
BlobMap blob_map_;
where BlobMap
was defined as a CaffeMap
at here:
typedef CaffeMap<string, unique_ptr<Blob> > BlobMap;
and CaffeMap
is defined here
using CaffeMap = std::map<Key, Value>;
// using CaffeMap = std::unordered_map;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yangqing has metioned why he prefer map
instead unordered_map
here
Note(Yangqing): NVCC does not play well with unordered_map on some platforms,
// forcing us to use std::map instead of unordered_map.
doc/design/scope.md
Outdated
using VariablePtr = std::shared_ptr<Variable>; | ||
|
||
class Scope final { | ||
public: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder -- I've put .clang-format files that strictly follows Google style in new source directories paddle/platform
, paddle/framework
. So this line would trigger clang-format error because Google style requires a single space before public:
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
doc/design/scope.md
Outdated
|
||
```cpp | ||
class Variable; | ||
using VariablePtr = std::shared_ptr<Variable>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this using alias because in a different scenario we might use different smart pointers. I'd suggest that we write everything in the definition of vars_
like:
using string = std::string; // So we can switch to other implementation of strings later.
std::map<string, unique_ptr<Variable> > vars_;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have a global header called TypeDefs.h
. In this header, we specify all default types for Map
, String
, Set
, etc. Like
namespace paddle {
template <typename KEY, typename VAL>
using Map = std::unordered_map<KEY, VAL>;
using String = std::string;
template <typename T>
using Set = std::unordered_set<T>;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us do such kind of summarization later -- after we make sure that some types are common to various situations.
doc/design/scope.md
Outdated
|
||
```cpp | ||
class Variable; | ||
using VariablePtr = std::shared_ptr<Variable>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, Caffe2 is using unique_ptr
in its Workspace definition:
typedef CaffeMap<string, unique_ptr<Blob> > BlobMap;
Why do we use shared_ptr
here?
To my understand, unique_ptr
is the right choice here because it transfers ownership of the pointed object when copy, whereas shared_ptr
increment the reference count when copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using unique_ptr
and return a raw pointer, users must not hold this pointer inside private member or any global variable. Because when scope
is destroyed, all pointers get before are invalid.
I think to use shared_ptr
inside a Scope
, but return a weak_ptr
when GetVariable
can resolve this issue. But this code is a little bit complicated.
Maybe we should have an agreement. We cannot store any variable pointer in our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us don't save pointers from unique_ptr as a class data member. I don't think gemm
need to hold such a pointer when we write an operator.
doc/design/scope.md
Outdated
|
||
```cpp | ||
Scope global; | ||
auto x = newVar("X"); // x is created in scope global, implicitly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a remind -- the following functions/methods do not following Google naming style:
newVar ==> NewVar
addOp ==> AddOp
run ==> Run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
doc/design/scope.md
Outdated
}; | ||
``` | ||
|
||
You need to specify a scope to run a Net. One net can run in different scopes and update different variable in the scope. If you did not specify one, It will run in a default scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!
I second removing the requirement that a net belongs to a workspace and am happy to see that our Scope doesn't have a line like the one in caffe2::Workspace
:
class Workspace {
...
NetMap net_map_;
doc/design/scope.md
Outdated
|
||
//! Get Variable in this scope. | ||
//! @return nullptr if no such variable. | ||
const VariablePtr& getVar(const std::string& name) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the return type must be Variable*
if we use unique_ptr
other than shared_ptr
in vars_
.
Also, following Google code style, this method should be named GetVar
.
doc/design/scope.md
Outdated
const VariablePtr& getVar(const std::string& name) const; | ||
|
||
//! Create or get a variable in this scope. | ||
VariablePtr& createOrGetVar(const std::string& name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename it into GetOrCreateVar
? After all, the primary purpose of this method is to "get", other than "create" -- it creates only if it cannot get.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…ature/scope_design
…ature/scope_design
But user can not hold this pointers.
doc/design/scope.md
Outdated
1. Scope should destruct all Variables within it when itself is destructed. | ||
|
||
Because Variable can only be got from Scope, when destroying Scope, we also need to destroy all the Vars in it. | ||
Variable can not belong to many scopes. If you want to use variables from parent scope, you can use `parent scope`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use multi
instead of many
is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe multiple
?
|
||
1. We can create local variables in a local scope. When that local scope are destroyed, all local variables should also be destroyed. | ||
2. Variables in a parent scope can be retrieved from local scopes of that parent scope, i.e., when user get a variable from a scope, it will try to search this variable in current scope. If there is no such variable in the local scope, `scope` will keep searching from its parent, until the variable is found or there is no parent. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
在编程语言里面,scope是可以多层嵌套的。这里scope可以嵌套多层吗?比如如果local没有,就先找parent,然后再找parent的parent,直到找到为止。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
读到文档末尾看到了,是可以嵌套的
class Scope { | ||
public: | ||
Scope(const std::shared_ptr<Scope>& scope): parent_(scope) {} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里传递的参数是指针的引用,是说parent_这个指针还会发生变化吗?也就是说一个Scope的parent scope是可以自己修改的?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
对于复杂类型的传参,应该传递const 引用。而parent_调用了shared_ptr的复制方法,复制了一份scope的指针。
如果这里改成传递std::shared_ptr<Scope>
,会在传参的时候创建一个临时变量。shared_ptr的开销在于每次创建变量的时候,要对这个变量加一个全局的Mutex,再增加一下计数器。所以开销也不算特别小。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
一个Scope的parent scope自己不可以修改。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us don't over use smart pointers. I believe the following would be enough for this case.
explicit Scope(const Scope& parent) : parent_(parent) {}
FYI, Caffe2 has the following:
explicit Workspace(Workspace* const shared) : shared_(shared) {}
doc/design/scope.md
Outdated
|
||
private: | ||
std::shared_ptr<Scope> parent_; | ||
std::unordered_map<std::string, std::unique_ptr<Attribute>> vars_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里似乎是笔误吧,放置了一个Attribute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
doc/design/scope.md
Outdated
if (var != nullptr) { | ||
return var; | ||
} else if (parent_ != nullptr) { | ||
return parent_->Get(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
既然parent也是scope,在这里调用的Get方法,那么Scope里面需要定义一个Get接口吧?还是说这里笔误了,应该是return parent_->GetVariable(name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
``` | ||
## Only scope can create a variable | ||
|
||
To ensure `only scope can create a variable`, we should mark `Variable`'s constructor as a private member function, and Scope is a friend class of Variable. And then only `CreateVariable` can construct `Variable`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I have a variable_test.cc
, this test file must contain scope.h
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我在想我们是在写代码的时候就要限制死用户不能在其他地方创建Variable
,还是这个只是个『约定』。
感觉如果写代码的时候限制死,用户在其他地方创建Variable
的时候直接报编译错,似乎更科学。
如果要这样的话,variable_test.cc如果需要写单测就要include scope.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second @hedaoyuan . I don't think we need the restriction that Variables can only be created by Scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember this afternoon we discussed the third point in this comments(#2545 (comment)).
The grad of the value is recorded by the Scope. But I did not seem to see the design.
如何根据一个参数的名字,来找到他的梯度。这件事情并不是Scope设计中的事情。Scope只存储了名字到变量的映射。 如何根据一个参数的名字找到他的梯度,目前来看可以有两种方法:
不过,这两种方法究竟应该选择哪个,如何做,应该是另一个issue和PR确定的事情。 |
|
所以Variable只能由Scope创建。 |
I don't think Operators are "stateless". an RNNOp would even create a sub-scope as it runs. Actually, I used to think that Net and Scope can be decoupled, but above seems an objective example. What do you think about this? |
RNN Op will create its sub-scopes actually. But it should return sub-scopes as Operator's output. The sub-scopes should not store locally in the operator class. In general, if an Op holds a GPU memory by private data member, we cannot switch If an Op needs some temporary memory while computing, we can just use If an Op needs some temporary memory sharing between multiple ops, it should be a variable in scope by our definition. |
在caffe2中,除了workspace中的blobMap保存了所有blob的信息,python端还提供了一个ModleHelper的类,抽象出了一个Model的概念,用来索引所有参数相关的blob;在dynet中,也存在一个Model的抽象,用来索引所有的参数。 那么我们可能的做法有以下两个:
|
还是在C++这边实现这些吧。。不要和Python强绑定为好。 |
我也不认为OP只能是stateless的,另外,是否stateless和有无临时变量也没关系。卷积OP里面有临时变量并不妨碍什么。 |
我们肯定期待使用高效的alloc算法会让临时变量的申请和释放不耗时。但是如果 即: class OpBase {
private:
vector<string> outputs_;
};
class SomeOpUseHugeMemory : public OpBase {
public:
SomeOpUseHugeMemory() {
outputs_.push_back("HugeTempVariableName");
}
}; |
看了下ModelHelper貌似是对Cpp中的net和parameters的一层封装,net就是core.Net(), parameter是BlobReference,按照现在的设计,在实现Python部分的时候,应该也需要这样一层抽象 |
Just like [scope](https://en.wikipedia.org/wiki/Scope_(computer_science)) in programming languages, `Scope` in the neural network can also be a local scope. There are two attributes about local scope. | ||
|
||
1. We can create local variables in a local scope. When that local scope are destroyed, all local variables should also be destroyed. | ||
2. Variables in a parent scope can be retrieved from local scopes of that parent scope, i.e., when user get a variable from a scope, it will try to search this variable in current scope. If there is no such variable in the local scope, `scope` will keep searching from its parent, until the variable is found or there is no parent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a local scope can have multiple parent scopes.
If a local scope Ls
has two parent scopes PsA and PsB; PsA and PsB have two variables called a, and b, respectively.
Ls want to use PsA.a and PsB.b how to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, user cannot access PsA.a
and PsB.b
in one local scope.
The scope is a linked-list. It will get local variable firstly, and local variable will hide parent variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just not at present, or never? Whether to consider later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this situation is not quite useful right now.
The original discuess in * PaddlePaddle#2548 (comment) * PaddlePaddle#2579 (comment) This commit is just a proposal, let's do such kind of summarization in this PR.
@reyoung @kuke and @jacquesqiao will coedit this doc on the design of
Scope
Markdown link is here
Fixes #2546