-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Implement basic Load()
and modify example based on updated inference design
#7690
Conversation
paddle/inference/inference.h
Outdated
|
||
void LoadInferenceModel(framework::Executor& executor, | ||
framework::Scope& scope, | ||
const std::string& dirname); |
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.
The definition of InferenceEngine
will be deleted (or you can delete it in this PR). I suggest to implement all load and save functions as a global function in a separate file named io.cc
, just like io.py
in Python API.
About the function's name, I suggest we use Load(...)
directly.
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.
Yes, I just kept this to have a minimal InferenceEngine as of now to get the load() working. Just to clarify, this calls the LoadModelAndParam
which is a new namespace infer
(in inference.cc). So as you suggested, in the future, i will move this to a separate file. Do you think the name LoadModelAndParam
is not okay?
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.
Do you think the name LoadModelAndParam is not okay?
I think we can Load(...)
for simple.
in the future, i will move this to a separate file
I suggest doing this now to fix the interface for users as soon as possible.
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.
Okay, will fix naming and file structure. Thanks.
paddle/framework/program_desc.h
Outdated
@@ -45,10 +45,19 @@ class ProgramDesc { | |||
|
|||
proto::ProgramDesc *Proto(); | |||
|
|||
// 4 utility functions for inference (TODO: Better comment) | |||
const std::vector<std::string> &GetFeedVarNames() const; | |||
const std::vector<std::string> &GetFetchVarNames() 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.
I think the two functions are not special for inference. When there are feed_op
in ProgramDesc
, we can always call this function to get feed var names. Same to GetFetchVarNames()
.
I am thinking another implementation, not keeping a member of feed_var_names_
and traversing all the operators to find the inputs of feed_op
. Like:
Paddle/paddle/inference/inference.cc
Lines 43 to 52 in 9fea1d4
framework::BlockDesc* global_block = program_->MutableBlock(0); | |
feed_var_names_.clear(); | |
fetch_var_names_.clear(); | |
for (auto* op : global_block->AllOps()) { | |
if (op->Type() == "feed") { | |
feed_var_names_.insert(feed_var_names_.begin(), op->Output("Out")[0]); | |
} else if (op->Type() == "fetch") { | |
fetch_var_names_.push_back(op->Input("X")[0]); | |
} | |
} |
Which way looks 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.
Thanks for the nice suggestion. But if go with option 2 (your suggestion), then we will need some placeholder class for keeping these 2 lists right? (For example, ProgramBuilder
in your initial design). Otherwise, these 2 lists will have to be floating around in the program.
Or maybe i misunderstood?
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.
we will need some placeholder class for keeping these 2 lists right?
There is no need to keep the 2 lists. When we want to get the 2 lists, we can always traverse all the operators of the given ProgramDesc
to get them. It is not time-consuming. We can make sure that the feed_var_names
and fetch_var_names
got from the two interfaces are consistent with the user ProgramDesc
.
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.
Sorry, i don't understand this:
We can make sure that the feed_var_names and fetch_var_names got from the two interfaces are consistent
If we don't keep feed_var_names and fetch_var_names as private members of the ProgramDesc
class, then we won't need the 4 functions (GetFetch, GetFeed, InsertFeed, InsertFetch) right?
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.
@sidgoyal78 I prefer @Xreki's suggestion because we don't need to add extra members to ProgramDesc
class.
In your code, it is possible that the feed_var_names
data member is inconsistent with the actual info in ProgramDesc
because you construct the feed_var_names
using InsertFetchVarName
.
Following @Xreki's suggestion:
We don't change any code in the ProgramDesc
class at all, thus we don't need these four functions.
Instead, we provide the general (not a member function of ProgramDesc
class) :
std::vector<std::string>& GetFeedVarNames(const ProgramDesc&);
std::vector<std::string>& GetFetchVarNames(const ProgramDesc&);
which obtain the vector of names on the fly by traversing the global block. This way we also make sure the obtained names are consistent with program desc.
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 in your suggestion InsertFetch and InsertFeed are not taken into account. Maybe we can go away without those inserts in this PR (assuming that the user is not provided with the functionality of inserts).
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.
Yes. I don't think InsertFetch and InsertFeed are good utility function because they are error prone. We can provide SetFetch SetFeed function instead to overwrite the feed/fetch info contained in ProgramDesc if user wants to.
paddle/inference/inference.h
Outdated
@@ -17,35 +17,31 @@ limitations under the License. */ | |||
#include "paddle/framework/block_desc.h" | |||
#include "paddle/framework/lod_tensor.h" | |||
#include "paddle/framework/program_desc.h" | |||
#include "paddle/framework/executor.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.
Please follow alphabetical order when including files, i.e., put executor.h
under block_desc.h
I think the example runs successfully now:
The model was generated with the following script (which seem to be deleted as of now): # Copyright (c) 2018 PaddlePaddle Authors. All Rights Reserve.
#
#Licensed under the Apache License, Version 2.0 (the "License");
#you may not use this file except in compliance with the License.
#You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
#Unless required by applicable law or agreed to in writing, software
#distributed under the License is distributed on an "AS IS" BASIS,
#WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
#See the License for the specific language governing permissions and
#limitations under the License.
from __future__ import print_function
import numpy as np
import paddle.v2 as paddle
import paddle.v2.fluid as fluid
BATCH_SIZE = 128
image = fluid.layers.data(name='x', shape=[784], dtype='float32')
regularizer = fluid.regularizer.L2Decay(0.0005 * BATCH_SIZE)
hidden1 = fluid.layers.fc(input=image,
size=128,
act='relu',
param_attr=fluid.ParamAttr(
regularizer=regularizer))
hidden2 = fluid.layers.fc(input=hidden1,
size=64,
act='relu',
param_attr=regularizer)
predict = fluid.layers.fc(input=hidden2,
size=10,
act='softmax',
param_attr=regularizer)
label = fluid.layers.data(name='y', shape=[1], dtype='int64')
cost = fluid.layers.cross_entropy(input=predict, label=label)
avg_cost = fluid.layers.mean(x=cost)
optimizer = fluid.optimizer.Momentum(learning_rate=0.001, momentum=0.9)
opts = optimizer.minimize(avg_cost)
accuracy = fluid.evaluator.Accuracy(input=predict, label=label)
inference_program = fluid.default_main_program().clone()
with fluid.program_guard(inference_program):
test_accuracy = fluid.evaluator.Accuracy(input=predict, label=label)
test_target = [avg_cost] + test_accuracy.metrics + test_accuracy.states
inference_program = fluid.io.get_inference_program(test_target)
train_reader = paddle.batch(
paddle.reader.shuffle(
paddle.dataset.mnist.train(), buf_size=8192),
batch_size=BATCH_SIZE)
test_reader = paddle.batch(paddle.dataset.mnist.test(), batch_size=128)
place = fluid.CPUPlace()
exe = fluid.Executor(place)
feeder = fluid.DataFeeder(feed_list=[image, label], place=place)
exe.run(fluid.default_startup_program())
PASS_NUM = 100
break_out = False
for pass_id in range(PASS_NUM):
accuracy.reset(exe)
for data in train_reader():
out, acc = exe.run(fluid.default_main_program(),
feed=feeder.feed(data),
fetch_list=[avg_cost] + accuracy.metrics)
pass_acc = accuracy.eval(exe)
test_accuracy.reset(exe)
for data in test_reader():
out, acc = exe.run(inference_program,
feed=feeder.feed(data),
fetch_list=[avg_cost] + test_accuracy.metrics)
test_pass_acc = test_accuracy.eval(exe)
print("pass_id=" + str(pass_id) + " train_cost=" + str(
out) + " train_acc=" + str(acc) + " train_pass_acc=" + str(pass_acc)
+ " test_acc=" + str(test_pass_acc))
if test_pass_acc > 0.7:
fluid.io.save_inference_model(
"./recognize_digits_mlp.inference.model/", ["x"], [predict],
exe)
break_out = True
break
if break_out == True:
break
# Use load_inference_model to obtain the inference program desc,
# the feed_target_names (the names of variables that will be feeded
# data using feed operators), and the fetch_targets (variables that
# we want to obtain data from using fetch operators).
[infer_prog, feed_target_names, fetch_targets] = fluid.io.load_inference_model(
"./recognize_digits_mlp.inference.model/", exe)
tensor_x = np.random.rand(1, 784).astype("float32")
# Construct feed as a dictionary of {feed_target_name: feed_target_data}
# and results will contain a list of data corresponding to fetch_targets.
results = exe.run(infer_prog,
feed={feed_target_names[0]: tensor_x},
fetch_list=fetch_targets)
print(results[0])
exit(0) |
paddle/inference/example.cc
Outdated
@@ -28,12 +30,29 @@ int main(int argc, char** argv) { | |||
exit(1); | |||
} | |||
|
|||
// 1. Define place, executor, scope | |||
auto* place = new paddle::platform::CPUPlace(); |
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.
Change this line to
auto place = paddle::platform::CPUPlace();
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.
Thanks for the review. Quick question: why do we want to use auto
over auto *
or vice-versa?
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 the question is more about when do you want to create a instance of a class or new a instance of class. auto
and auto*
will follow naturally. Below is just my personal opinion.
For this place example, because place is only used as a variable to separate CPU/GPU, we don't need to modify its state or use its member function. To simplify the code, we prefer to directly construct paddle::platform::CPUPlace()
.
For other examples, where you may want to modify the state of a instance or use its member functions, it may be better to use auto* = new xxx
.
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
paddle/inference/example.cc
Outdated
auto* place = new paddle::platform::CPUPlace(); | ||
paddle::framework::InitDevices(); | ||
paddle::framework::Executor* executor = | ||
new paddle::framework::Executor(*place); |
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.
Change to
auto* executor = new paddle::framework::Executor(place);
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
paddle/inference/example.cc
Outdated
@@ -62,6 +99,9 @@ int main(int argc, char** argv) { | |||
std::cout << std::endl; | |||
} | |||
|
|||
delete engine; | |||
delete place; |
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.
delete this line
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
paddle/inference/io.h
Outdated
framework::Scope& scope, | ||
const std::string& dirname); | ||
|
||
std::vector<std::string> GetFeedVarNames(framework::ProgramDesc* main_program); |
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 we prefer to keep this function as a public member of the ProgramDesc class.
So that we can use the following code:
std::vector<std::string>& feed_var_names = main_program->GetFeedVarNames();
We still get the vector on the fly, meaning we don't add private data members to program desc.
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.
Okay, i have a related question. Do you think GetFeedVarNames() will be useful elsewhere apart from the current use case for inference?
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 also think GetFeedVarNames
and GetFetchVarNames
are not io interfaces. @sidgoyal78 @kexinzhao I'm thinking about whether we can simplify the new Executor.Run(...)
using this two functions? I am not sure, just thinking about...
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.
Okay.. I think as of now, based on yours and @kexinzhao 's suggestion, I will put the GetFeed
and GetFetch
methods in the ProgramDesc
. We can then think about simplifying.
paddle/inference/io.h
Outdated
const std::string& dirname); | ||
|
||
std::vector<std::string> GetFeedVarNames(framework::ProgramDesc* main_program); | ||
std::vector<std::string> GetFetchVarNames(framework::ProgramDesc* main_program); |
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.
Same here.
paddle/inference/example.cc
Outdated
paddle::framework::InitDevices(); | ||
paddle::framework::Executor* executor = | ||
new paddle::framework::Executor(*place); | ||
paddle::framework::Scope* scope = new paddle::framework::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.
paddle::framework::Scope* -> auto*
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
paddle/inference/example.cc
Outdated
// 3. Optional: perform optimization on the inference_program | ||
|
||
// 4. Get the feed_var_names and fetch_var_names | ||
std::vector<std::string> feed_var_names = |
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 change feed/fetch_var_names to feed/fetch_target_names to make them consistent with python side 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.
Sure, will modify. Thanks.
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 it gets a bit confusing with variables feed_targets
and fetch_targets
. Maybe I will leave this for now.
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.
There are feed_var_names and feed_var_name in python implementation. But there are not the same things: feed_var_names
is the output's names of feed_op
and feed_var_name
is the input's names of feed_op
. So we should distinguish this. Now we use feed_targets
and feed_holder
respectively now. Do you have any other suggestion?
paddle/inference/example.cc
Outdated
@@ -15,7 +15,9 @@ limitations under the License. */ | |||
#include <time.h> | |||
#include <iostream> | |||
#include "gflags/gflags.h" | |||
#include "paddle/inference/inference.h" | |||
#include "io.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.
io.h
-> paddle/inference/io.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.
Done
paddle/inference/io.cc
Outdated
See the License for the specific language governing permissions and | ||
limitations under the License. */ | ||
|
||
#include "io.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.
io.h
-> paddle/inference/io.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.
Done
paddle/inference/io.cc
Outdated
|
||
namespace paddle { | ||
|
||
namespace io { |
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.
namespace io
-> namespace inference
, the namespace
's name should be the same as the directory tree.
Remove blank line 19
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
paddle/inference/io.cc
Outdated
namespace io { | ||
|
||
bool IsParameter(const framework::VarDesc* var, | ||
const framework::ProgramDesc* main_program) { |
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 remind here that this function is introduced to make the current inference ProgramDesc
run. Normally, there should not be any unreferenced variables. We should also check the existence of all parameters and give prompt information if there are some parameters absent. This can be done in next PR.
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.
Noted.
op->CheckAttrs(); | ||
} | ||
} | ||
executor.Run(*load_program, &scope, 0, true, true); |
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.
Delete load_program here.
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
paddle/inference/io.cc
Outdated
return feed_var_names; | ||
} | ||
|
||
std::vector<std::string> GetFetchVarNames( |
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.
std::vector<std::string>
-> const std::vector<std::string>&
paddle/inference/io.cc
Outdated
std::vector<std::string> fetch_var_names; | ||
|
||
for (auto* op : global_block->AllOps()) { | ||
if (op->Type() == "fetch") { |
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.
fetch
-> kFetchOpType
paddle/inference/io.h
Outdated
#include <vector> | ||
#include "paddle/framework/block_desc.h" | ||
#include "paddle/framework/executor.h" | ||
#include "paddle/framework/init.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.
Line 21
can be removed.
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
paddle/inference/io.h
Outdated
framework::Scope& scope, | ||
const std::string& dirname); | ||
|
||
std::vector<std::string> GetFeedVarNames(framework::ProgramDesc* main_program); |
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 also think GetFeedVarNames
and GetFetchVarNames
are not io interfaces. @sidgoyal78 @kexinzhao I'm thinking about whether we can simplify the new Executor.Run(...)
using this two functions? I am not sure, just thinking about...
paddle/inference/io.h
Outdated
|
||
namespace paddle { | ||
|
||
namespace io { |
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.
Remove blank line 27
Rename namespace to inference
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
… initial_inference
… initial_inference
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.
There are some minor aspects to be fixed. But we can merge this PR first to avoid blocking other PRs. Will fix those minor aspects in next PR.
BlockDesc *global_block = blocks_[0].get(); | ||
std::vector<std::string> feed_var_names; | ||
for (auto *op : global_block->AllOps()) { | ||
if (op->Type() == "feed") { |
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.
feed -> kFeedOpType
and let's rename feed_var_names
to feed_target_names
if there is no better candidate.
BlockDesc *global_block = blocks_[0].get(); | ||
std::vector<std::string> fetch_var_names; | ||
for (auto *op : global_block->AllOps()) { | ||
if (op->Type() == "fetch") { |
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.
fetch -> kFetchOpType
and let's rename fetch_var_names
to fetch_target_names
if there is no better candidate.
@@ -45,6 +45,10 @@ class ProgramDesc { | |||
|
|||
proto::ProgramDesc *Proto(); | |||
|
|||
const std::vector<std::string> GetFeedVarNames(); | |||
|
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.
remove this blank line.
inputfs.close(); | ||
|
||
framework::ProgramDesc* main_program = | ||
new framework::ProgramDesc(program_desc_str); |
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.
We need to use std::unique_ptr
here and not leave the delete task to users.
This PR basically modifies the current
example.cc
andinference.*
files (in addition to changes inprogram_desc.*
) to implement a basicLoad()
function which follows the current design doc by @Xreki (PR #7315). The example code is modified accordingly.The example code runs successfully,
but there is another TODO item.TODO: The new getters and setters for feed and fetch vars inprogram_desc
should have corresponding implementations inprotobuf.cc
.