Skip to content
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

Merged
merged 32 commits into from
Jun 27, 2017
Merged
Changes from 3 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
bcac91a
scope design doc
jacquesqiao Jun 21, 2017
002a6c9
Rearrange docs
reyoung Jun 21, 2017
674b1d3
Update code
reyoung Jun 21, 2017
7a48507
fix code style
jacquesqiao Jun 21, 2017
3e09978
Add scope doc
reyoung Jun 22, 2017
acf0b75
Merge branch 'scope' of https://github.com/jacquesqiao/Paddle into fe…
reyoung Jun 22, 2017
04ad9b6
Add Scope Parent & Local section
reyoung Jun 22, 2017
581e4c1
Parent & local scope done
reyoung Jun 22, 2017
0b70361
Refining english
reyoung Jun 22, 2017
76e2a3c
Refine English
reyoung Jun 22, 2017
8282138
some properties of scope
jacquesqiao Jun 22, 2017
4413726
Merge branch 'scope' of https://github.com/jacquesqiao/Paddle into scope
jacquesqiao Jun 22, 2017
d7aca77
Update API
reyoung Jun 22, 2017
2d5507f
Add interfaces
reyoung Jun 22, 2017
64a1cdf
Merge branch 'scope' of https://github.com/jacquesqiao/Paddle into fe…
reyoung Jun 22, 2017
73b1c5b
add overview for scope design doc
Jun 22, 2017
1f0056b
Update interface
reyoung Jun 22, 2017
0b07583
Merge branch 'scope' of https://github.com/jacquesqiao/Paddle into scope
Jun 22, 2017
17eed33
Update key attributes
reyoung Jun 22, 2017
37fd48b
some detailed explaination of the Scope properties
jacquesqiao Jun 22, 2017
c3a4b8b
refine style of markdown
jacquesqiao Jun 22, 2017
db96c0e
Use unique_ptr instead of shared_ptr/weak_ptr.
reyoung Jun 22, 2017
f104ce2
fix a mistake share by nets -> share by scopes
jacquesqiao Jun 22, 2017
32fe097
Merge branch 'scope' of https://github.com/jacquesqiao/Paddle into fe…
reyoung Jun 22, 2017
eab0e52
To google code style
reyoung Jun 22, 2017
63a56b4
Change typo
reyoung Jun 22, 2017
921fa13
Remove delete
reyoung Jun 22, 2017
5d88249
Typo
reyoung Jun 22, 2017
f8a209c
Rearrange description.
reyoung Jun 22, 2017
c5ad89a
Change title
reyoung Jun 22, 2017
237efc2
Fix markdown
reyoung Jun 22, 2017
3bac2d0
Typo
reyoung Jun 22, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions doc/design/scope.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# Scope

### Define

Scope is a context to manage Variables. It mainly contains a map from Variable name to Variable. Net will get and update variable throw scope.

```cpp
class Variable;
using VariablePtr = std::shared_ptr<Variable>;
Copy link
Collaborator

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_;

Copy link
Collaborator

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>;
}

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.


class Scope final {
public:
Copy link
Collaborator

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:.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Scope();
Scope(const std::shared_ptr<Scope>& parent);

//! Get Variable in this scope.
//! @return nullptr if no such variable.
const VariablePtr& getVar(const std::string& name) const;
Copy link
Collaborator

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.


//! Create or get a variable in this scope.
VariablePtr& createOrGetVar(const std::string& name);
Copy link
Collaborator

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done


private:
/// variable name -> variable
std::unordered_map<std::string, VariablePtr> vars_;
Copy link
Collaborator

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;

Copy link
Contributor

@dzhwinter dzhwinter Jun 21, 2017

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.

std::shared_ptr<Scope> parent_{nullptr};
};
```

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.
Copy link
Collaborator

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_;


```cpp
Scope global;
auto x = newVar("X"); // x is created in scope global, implicitly.
Copy link
Collaborator

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

Copy link
Member Author

Choose a reason for hiding this comment

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

done

auto y = newVar("Y");
Net net1;
net1.addOp("add", {x, y}, {x}); // x = x + y;
net1.run();

for (size_t i=0; i<10; ++i) {
Scope local;
auto tmp = newVar("tmp"); // tmp is created in scope local.
Net net2;
net2.addOp("add", {x, y}, {tmp});
net2.run(); // tmp = x + y;
}

Net net3;
net3.addOp("add", {x, y}, {"tmp"}); // error! cannot found "tmp" in global scope.

```

### Chain structure

Scope has a pointer point to it's parent scope, this is mainly used in RNN when it need to create many stepNet.


### Scope Guard