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

Better version of PR #7985 (Modify load() for inference) #8024

Merged
merged 21 commits into from
Feb 8, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
74 changes: 58 additions & 16 deletions paddle/inference/io.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,18 @@ limitations under the License. */
namespace paddle {
namespace inference {

void ReadProgramDescFromFile(const std::string& model_filename,
std::string& program_desc_str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function is a common function to read binary to string. If there is no other use for this reading function, how about change it to

std::unique_ptr<framework::ProgramDesc> ReadProgram(const std::string& filename)

But we can change this in next PR.

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 this function will also be useful for "loading-from-buffer" case. I have changed the name to ReadBinaryFile.

VLOG(3) << "loading model from " << model_filename;
std::ifstream inputfs(model_filename, std::ios::in | std::ios::binary);
inputfs.seekg(0, std::ios::end);
program_desc_str.resize(inputfs.tellg());
inputfs.seekg(0, std::ios::beg);
VLOG(3) << "program_desc_str's size: " << program_desc_str.size();
inputfs.read(&program_desc_str[0], program_desc_str.size());
inputfs.close();
}

bool IsParameter(const framework::VarDesc* var,
const framework::ProgramDesc& main_program) {
if (var->Persistable()) {
Expand All @@ -44,12 +56,15 @@ bool IsParameter(const framework::VarDesc* var,

void LoadPersistables(framework::Executor& executor,
framework::Scope& scope,
const framework::ProgramDesc& main_program,
const std::string& dirname,
const framework::ProgramDesc& main_program) {
const std::string& param_filename) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two LoadPersistables. Can merge them into one?

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

const framework::BlockDesc& global_block = main_program.Block(0);

framework::ProgramDesc* load_program = new framework::ProgramDesc();
framework::BlockDesc* load_block = load_program->MutableBlock(0);
std::vector<std::string> paramlist;

for (auto* var : global_block.AllVars()) {
if (IsParameter(var, main_program)) {
VLOG(3) << "parameter's name: " << var->Name();
Expand All @@ -61,36 +76,63 @@ void LoadPersistables(framework::Executor& executor,
new_var->SetLoDLevel(var->GetLoDLevel());
new_var->SetPersistable(true);

// append_op
framework::OpDesc* op = load_block->AppendOp();
op->SetType("load");
op->SetOutput("Out", {new_var->Name()});
op->SetAttr("file_path", {dirname + "/" + new_var->Name()});
op->CheckAttrs();
if (!param_filename.empty()) {
paramlist.push_back(new_var->Name());
} else {
// append_op
framework::OpDesc* op = load_block->AppendOp();
op->SetType("load");
op->SetOutput("Out", {new_var->Name()});
op->SetAttr("file_path", {dirname + "/" + new_var->Name()});
op->CheckAttrs();
}
}
}

if (!param_filename.empty()) {
// sort paramlist to have consistent ordering
std::sort(paramlist.begin(), paramlist.end());
Copy link
Contributor

Choose a reason for hiding this comment

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

Tensor and LoDTensor have no name. If there is something wrong (for example, user coincidentally gives a wrong param_filename), there is no warning information at all. It will be difficult for users to debug. Can we add another mechanism to check or ensure the correctness? We can refer to other frameworks. Also, this can be done in next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will think about it and as you mentioned will do later.

// append just the load_combine op
framework::OpDesc* op = load_block->AppendOp();
op->SetType("load_combine");
op->SetOutput("Out", paramlist);
op->SetAttr("file_path", {param_filename});
op->CheckAttrs();
}

executor.Run(*load_program, &scope, 0, true, true);

VLOG(3) << "Ran loading successfully";
delete load_program;
}

std::unique_ptr<framework::ProgramDesc> Load(framework::Executor& executor,
framework::Scope& scope,
const std::string& dirname) {
std::string model_filename = dirname + "/__model__";
LOG(INFO) << "loading model from " << model_filename;
std::ifstream inputfs(model_filename, std::ios::in | std::ios::binary);
std::string program_desc_str;
inputfs.seekg(0, std::ios::end);
program_desc_str.resize(inputfs.tellg());
inputfs.seekg(0, std::ios::beg);
LOG(INFO) << "program_desc_str's size: " << program_desc_str.size();
inputfs.read(&program_desc_str[0], program_desc_str.size());
inputfs.close();
ReadProgramDescFromFile(model_filename, program_desc_str);

std::unique_ptr<framework::ProgramDesc> main_program(
new framework::ProgramDesc(program_desc_str));

LoadPersistables(executor, scope, *main_program, dirname, "");
return main_program;
}

std::unique_ptr<framework::ProgramDesc> Load(
framework::Executor& executor,
framework::Scope& scope,
const std::string& prog_filename,
const std::string& param_filename) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to keep two Load() interfaces as:

std::unique_ptr<framework::ProgramDesc> Load(
     framework::Executor& executor,
     framework::Scope& scope,
     const std::string& dirname);

std::unique_ptr<framework::ProgramDesc> Load(
     framework::Executor& executor,
     framework::Scope& scope,
     const std::string& prog_filename,
     const std::string& params_filename);

Users are free to rename the file which saving the program. The argument list of save_inference_model may be changed in the future. #7995 is merged, we need to test the new Load. Please use the new Load in the unittest instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

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

Choose a reason for hiding this comment

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

Sorry for not describing my suggest clearly. I mean to define the interface to:

std::unique_ptr<framework::ProgramDesc> Load(
    framework::Executor& executor,
    framework::Scope& scope,
    const std::string& prog_filepath,
    const std::string& param_filepath);

No need of dirname any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I think you described your suggestion clearly: #8024 (comment)

But, I chose to keep another argument of dirname to make stuff consistent with the python side. More specifically, in save_vars() we have: https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/v2/fluid/io.py#L124, and in load_vars() we have: https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/v2/fluid/io.py#L202

Hence, I put that extra argument. But it seems that you would prefer an implementation without it, so I will modify it. Thanks.

std::string model_filename = prog_filename;
std::string program_desc_str;
ReadProgramDescFromFile(model_filename, program_desc_str);

std::unique_ptr<framework::ProgramDesc> main_program(
new framework::ProgramDesc(program_desc_str));

LoadPersistables(executor, scope, dirname, *main_program);
LoadPersistables(executor, scope, *main_program, "", param_filename);
return main_program;
}

Expand Down
8 changes: 7 additions & 1 deletion paddle/inference/io.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,18 @@ namespace inference {

void LoadPersistables(framework::Executor& executor,
framework::Scope& scope,
const framework::ProgramDesc& main_program,
const std::string& dirname,
const framework::ProgramDesc& main_program);
const std::string& param_filename);

std::unique_ptr<framework::ProgramDesc> Load(framework::Executor& executor,
framework::Scope& scope,
const std::string& dirname);

std::unique_ptr<framework::ProgramDesc> Load(framework::Executor& executor,
framework::Scope& scope,
const std::string& prog_filename,
const std::string& param_filename);

} // namespace inference
} // namespace paddle
62 changes: 58 additions & 4 deletions paddle/inference/tests/book/test_inference_recognize_digits.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,29 @@ limitations under the License. */

DEFINE_string(dirname, "", "Directory of the inference model.");

template <typename Place, typename T>
template <typename Place, typename T, bool IsCombined>
void TestInference(const std::string& dirname,
const std::vector<paddle::framework::LoDTensor*>& cpu_feeds,
std::vector<paddle::framework::LoDTensor*>& cpu_fetchs) {
// 1. Define place, executor and scope
auto place = Place();
auto executor = paddle::framework::Executor(place);
auto* scope = new paddle::framework::Scope();
std::unique_ptr<paddle::framework::ProgramDesc> inference_program;

LOG(INFO) << "Going to call load()" << std::endl;
// 2. Initialize the inference_program and load all parameters from file
auto inference_program = paddle::inference::Load(executor, *scope, dirname);
if (IsCombined) {
// Hard-coding the names for combined params case
std::string prog_filename = "__model_combined__";
std::string param_filename = "__params_combined__";
inference_program = paddle::inference::Load(executor,
*scope,
dirname + "/" + prog_filename,
dirname + "/" + param_filename);
} else {
inference_program = paddle::inference::Load(executor, *scope, dirname);
}

// 3. Get the feed_target_names and fetch_target_names
const std::vector<std::string>& feed_target_names =
Expand Down Expand Up @@ -123,7 +135,49 @@ TEST(inference, recognize_digits) {
cpu_fetchs1.push_back(&output1);

// Run inference on CPU
TestInference<paddle::platform::CPUPlace, float>(
TestInference<paddle::platform::CPUPlace, float, false>(
dirname, cpu_feeds, cpu_fetchs1);
LOG(INFO) << output1.dims();

#ifdef PADDLE_WITH_CUDA
paddle::framework::LoDTensor output2;
std::vector<paddle::framework::LoDTensor*> cpu_fetchs2;
cpu_fetchs2.push_back(&output2);

// Run inference on CUDA GPU
TestInference<paddle::platform::CUDAPlace, float, false>(
dirname, cpu_feeds, cpu_fetchs2);
LOG(INFO) << output2.dims();

CheckError<float>(output1, output2);
#endif
}

TEST(inference, recognize_digits_combine) {
if (FLAGS_dirname.empty()) {
LOG(FATAL) << "Usage: ./example --dirname=path/to/your/model";
}

LOG(INFO) << "FLAGS_dirname: " << FLAGS_dirname << std::endl;
std::string dirname = FLAGS_dirname;

// 0. Call `paddle::framework::InitDevices()` initialize all the devices
// In unittests, this is done in paddle/testing/paddle_gtest_main.cc

paddle::framework::LoDTensor input;
// Use normilized image pixels as input data,
// which should be in the range [-1.0, 1.0].
SetupTensor<float>(
input, {1, 28, 28}, static_cast<float>(-1), static_cast<float>(1));
std::vector<paddle::framework::LoDTensor*> cpu_feeds;
cpu_feeds.push_back(&input);

paddle::framework::LoDTensor output1;
std::vector<paddle::framework::LoDTensor*> cpu_fetchs1;
cpu_fetchs1.push_back(&output1);

// Run inference on CPU
TestInference<paddle::platform::CPUPlace, float, true>(
dirname, cpu_feeds, cpu_fetchs1);
LOG(INFO) << output1.dims();

Expand All @@ -133,7 +187,7 @@ TEST(inference, recognize_digits) {
cpu_fetchs2.push_back(&output2);

// Run inference on CUDA GPU
TestInference<paddle::platform::CUDAPlace, float>(
TestInference<paddle::platform::CUDAPlace, float, true>(
dirname, cpu_feeds, cpu_fetchs2);
LOG(INFO) << output2.dims();

Expand Down
14 changes: 12 additions & 2 deletions python/paddle/v2/fluid/io.py
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,12 @@ def save_inference_model(dirname,
prepend_feed_ops(inference_program, feeded_var_names)
append_fetch_ops(inference_program, fetch_var_names)

model_file_name = dirname + "/__model__"
model_file_name = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete this line.
Unlike C++, if else statements don't define a scope in Python. So model_file_name is visible outside.
https://stackoverflow.com/questions/7382638/python-variable-scope-in-if-statements

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, thanks.

if save_file_name == None:
model_file_name = dirname + "/__model__"
else:
model_file_name = dirname + "/__model_combined__"
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming style is not friendly for users. We need to refine this interface.


with open(model_file_name, "wb") as f:
f.write(inference_program.desc.serialize_to_string())

Expand Down Expand Up @@ -384,7 +389,12 @@ def load_inference_model(dirname, executor, load_file_name=None):
if not os.path.isdir(dirname):
raise ValueError("There is no directory named '%s'", dirname)

model_file_name = dirname + "/__model__"
model_file_name = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Please delete this line.

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.

if load_file_name == None:
model_file_name = dirname + "/__model__"
else:
model_file_name = dirname + "/__model_combined__"

with open(model_file_name, "rb") as f:
program_desc_str = f.read()

Expand Down
44 changes: 29 additions & 15 deletions python/paddle/v2/fluid/tests/book/test_recognize_digits.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def conv_net(img, label):
return loss_net(conv_pool_2, label)


def train(nn_type, use_cuda, parallel, save_dirname):
def train(nn_type, use_cuda, parallel, save_dirname, save_param_filename):
if use_cuda and not fluid.core.is_compiled_with_cuda():
return
img = fluid.layers.data(name='img', shape=[1, 28, 28], dtype='float32')
Expand Down Expand Up @@ -140,8 +140,10 @@ def train(nn_type, use_cuda, parallel, save_dirname):
avg_loss_val = numpy.array(avg_loss_set).mean()
if float(acc_val) > 0.85: # test acc > 85%
if save_dirname is not None:
fluid.io.save_inference_model(save_dirname, ["img"],
[prediction], exe)
fluid.io.save_inference_model(
save_dirname, ["img"], [prediction],
exe,
save_file_name=save_param_filename)
return
else:
print(
Expand All @@ -151,7 +153,7 @@ def train(nn_type, use_cuda, parallel, save_dirname):
raise AssertionError("Loss of recognize digits is too large")


def infer(use_cuda, save_dirname=None):
def infer(use_cuda, save_dirname=None, param_filename=None):
if save_dirname is None:
return

Expand All @@ -162,8 +164,8 @@ def infer(use_cuda, save_dirname=None):
# 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).
[inference_program, feed_target_names,
fetch_targets] = fluid.io.load_inference_model(save_dirname, exe)
[inference_program, feed_target_names, fetch_targets
] = fluid.io.load_inference_model(save_dirname, exe, param_filename)

# The input's dimension of conv should be 4-D or 5-D.
# Use normilized image pixels as input data, which should be in the range [-1.0, 1.0].
Expand All @@ -178,36 +180,45 @@ def infer(use_cuda, save_dirname=None):
print("infer results: ", results[0])


def main(use_cuda, parallel, nn_type):
def main(use_cuda, parallel, nn_type, combine):
if not use_cuda and not parallel:
save_dirname = "recognize_digits_" + nn_type + ".inference.model"
save_filename = None
if combine == True:
save_filename = "__params_combined__"
else:
save_dirname = None
save_filename = None

train(
nn_type=nn_type,
use_cuda=use_cuda,
parallel=parallel,
save_dirname=save_dirname)
infer(use_cuda=use_cuda, save_dirname=save_dirname)
save_dirname=save_dirname,
save_param_filename=save_filename)
infer(
use_cuda=use_cuda,
save_dirname=save_dirname,
param_filename=save_filename)


class TestRecognizeDigits(unittest.TestCase):
pass


def inject_test_method(use_cuda, parallel, nn_type):
def inject_test_method(use_cuda, parallel, nn_type, combine):
def __impl__(self):
prog = fluid.Program()
startup_prog = fluid.Program()
scope = fluid.core.Scope()
with fluid.scope_guard(scope):
with fluid.program_guard(prog, startup_prog):
main(use_cuda, parallel, nn_type)
main(use_cuda, parallel, nn_type, combine)

fn = 'test_{0}_{1}_{2}'.format(nn_type, 'cuda'
if use_cuda else 'cpu', 'parallel'
if parallel else 'normal')
fn = 'test_{0}_{1}_{2}_{3}'.format(nn_type, 'cuda'
if use_cuda else 'cpu', 'parallel'
if parallel else 'normal', 'combine'
if combine else 'separate')

setattr(TestRecognizeDigits, fn, __impl__)

Expand All @@ -216,7 +227,10 @@ def inject_all_tests():
for use_cuda in (False, True):
for parallel in (False, True):
for nn_type in ('mlp', 'conv'):
inject_test_method(use_cuda, parallel, nn_type)
inject_test_method(use_cuda, parallel, nn_type, True)

# One unit-test for saving parameters as separate files
inject_test_method(False, False, 'mlp', False)


inject_all_tests()
Expand Down