Add inferencer infer #10445
Add inferencer infer #10445
Conversation
jacquesqiao
requested review from
helinwang,
reyoung,
cs2be,
abhinavarora,
wangkuiyi,
sidgoyal78 and
JiayiFeng
python/paddle/fluid/inferencer.py
Outdated
|
|
||
| if param_path: | ||
| self.exe = executor.Executor(place) |
There was a problem hiding this comment.
The place could be None.
python/paddle/fluid/inferencer.py
Outdated
| # 3. run the default_startup program. | ||
|
|
||
| # 4. load params from param_path into scope | ||
| self.param_path = param_path | ||
| self.scope = core.Scope() | ||
| self.place = place |
There was a problem hiding this comment.
self.place is never used.
python/paddle/fluid/inferencer.py
Outdated
| self.fetch_targets] = io.load_inference_model( | ||
| executor=self.exe, dirname=param_path) | ||
| else: | ||
| self.inference_program = framework.Program() |
There was a problem hiding this comment.
Where are parameter values from if param_path is None?
python/paddle/fluid/trainer.py
Outdated
| raise ValueError( | ||
| "program_func should return tuple(loss, predict_vars)") | ||
|
|
||
| loss = out[0] |
There was a problem hiding this comment.
Here is a problem I also encounter in my trainer.test(). The program_func() may returns more than one element. But only one of them('loss') will be used to build the backward part. In the current design, we have no method to point out which one is the loss.
There was a problem hiding this comment.
I create a issue for this problem #10474.
… add-inferencer-infer
… add-inferencer-infer
… add-inferencer-infer
| predict_var = self.infer_func() | ||
| predict_var = self.train_program.block(0).var(predict_var.name) | ||
| exe = executor.Executor(self.place) | ||
| io.save_inference_model(model_path, [], [predict_var], exe) |
There was a problem hiding this comment.
Here the predict_var is in the train_program, so we are using the train_program to build the inference model. Why not use inference_program directly?
There was a problem hiding this comment.
Because save_inference_model will do some other thing to inference_program, we need to rewrite it, but currently, I think rewrite this function do not influence the interface and may introduce bugs.
| if isinstance(event, fluid.EndEpochEvent): | ||
| avg_cost = trainer.test(reader=paddle.dataset.imikolov.test( | ||
| word_dict, N)) | ||
|
|
||
| if avg_cost < 5.0: | ||
| trainer.save_params(save_path) |
There was a problem hiding this comment.
Why don't we use the save_params method?
The function is already implemented. Is it because we need the model to be saved? I thought the save_persistible will do the same thing
| def __init__(self, program_func, optimizer, param_path=None, place=None): | ||
| def __init__(self, | ||
| train_func, | ||
| infer_func, |
There was a problem hiding this comment.
This pattern seems new to me. Did we decide that the Trainer needs to take both train_func and infer_func. Sorry if I missed the conversation.
There was a problem hiding this comment.
because Trainer.save_inference_model need to know the inference program. infer_func is used to get the inference program.
| # 3. run the default_startup program. | ||
|
|
||
| # 4. load params from param_path into scope | ||
| def __init__(self, param_path, place=None): |
There was a problem hiding this comment.
So Inferencer doesn't need to take the Infer_program anymore?
There was a problem hiding this comment.
yes, it can load everything from the saved inference model
… add-inferencer-infer
| if not (bool(feeded_var_names) and all( | ||
| isinstance(name, basestring) for name in feeded_var_names)): | ||
| raise ValueError("'feed_var_names' should be a list of str.") | ||
| if len(feeded_var_names) > 0: |
There was a problem hiding this comment.
Should we throw error if feeded_var_names is not an array? Also I found the correct spell is fed_var_names, not feeded_. And it is being used in many places.
There was a problem hiding this comment.
oh, feeded is wrong, can you help to correct it. I think feed_var_names is ok.
| fluid.optimizer.SGD(learning_rate=0.001), | ||
| place=place) | ||
| trainer.train( | ||
| reader=train_reader, num_epochs=100, event_handler=event_handler) | ||
| reader=train_reader, num_epochs=1, event_handler=event_handler) |
There was a problem hiding this comment.
So the default num_epochs is 1?
There was a problem hiding this comment.
this is for test, I can change it back to 100
… add-inferencer-infer
fix: #10364