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

add net_design doc #2547

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
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
65 changes: 65 additions & 0 deletions doc/design/refactor_discuess/net_design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# Network Design
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest that we move this design doc into paddle/framework/net.md, so we could keep the code (implementation) together with the design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


`Network` is the container and controller of a set of operators in a network, users can use `Network.addOp` to add operators into a network,
and use `Network.runOps` to run all the operators in the network.
Copy link
Contributor

Choose a reason for hiding this comment

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

To conform to list API in Net class, we should use Run instead of RunOps here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


The `Network` will

- manage all the operators contained in the network.
- not own any `Variable`.

# API

To make the `Network` extendibe, a base class is defined like this

```c++
// The minimum a network should be implemented.
class BaseNetwork {
Copy link
Contributor

Choose a reason for hiding this comment

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

Named NetworkBase or Net is better ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Net seems wired without a prefix like "SimpleNet" or "DAGNet" and I will name it "NetworkBase"

public:
BaseNetwork(const NetDef& def, Scope *scope);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Following the purpose of decoupling Scope and Net, scope should be passed to Net::Run other than to Net::Net.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


// run all the operators and return success(true) or not.
virtual bool Run() = 0;

protected:
// the input variables feed into the network.
std::vector<Variable*> external_inputs_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As there is not something named internal_inputs_, here we should just name it inputs_? And analogously, rename external_outputs_ into outputs_?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that a net could read and/or write a variable, so we might have read-only and writable variables, other than input and output variables, associated with it?

I am not sure if we should distinguish them, or just have vars_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think inputs and outputs should be distinguished because both of them are important properties of a network(a directed graph).

As a "base network", we may inherit NetworkBase and implement a DAG in the future, and inputs and outputs will directly determine a sub-graph of a DAG.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because a Scope is passed in to Run other than the constructor, we don't have a chance to get Variable* here. Instead, we should have:

std::vector<string> vars_;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// the corresponding output variables the network will write.
std::vector<Variable*> external_outputs_;
// scope which contains all the global variables visiable to this network.
Scope *scope_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we have scope_, we are not decoupling Net from Workspace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

};
```

A simple implemention is as followed:

```c++
class Network : public BaseNetwork {
public:

// Create an empty network.
Network(const std::string& name, Scope *scope);

// NetDef is the definition of a network, in some occasion, operators are created
// dynamically by user one by one; but in some other occasion such as LSTM, all
// the operators in the networks should be created during the construction
// of the network. So a `NetDef` is provided to make the `Network` create a
// network with all the operators described in `def`.
Network(const std::string& name, const NetDef& def, Scope *scope);

// add a operator which is identified as `type` and has attributes described
// in `attr`.
bool AddOp(const std::string &type, const OprAttr& attr);

// run all operators in oprs_ sequentially.
virtual bool Run() override;

protected:
// to make the network's structure more human-readable, each network will
// has a `name`
std::string name_;
Copy link
Contributor

Choose a reason for hiding this comment

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

it is useless at present. we can remove it from the design doc and add when it is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// the operations are owned by `Network`.
std::vector<std::shared_ptr<Operator>> oprs_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should use unique_ptr here other than shared_ptr. The reason is in this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

The word "operator" usually shortened as "op", not "opr".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

};
```