upgrade the TraceLayer.save_inference_model method with add file suffix automatically#31989
Conversation
|
|
|
Thanks for your contribution! |
CtfGo
force-pushed
the
fix-TraceLayer-save
branch
from
a7e7783 to
6e350c1
Compare
CtfGo
force-pushed
the
fix-TraceLayer-save
branch
from
6e350c1 to
92ccb86
Compare
There was a problem hiding this comment.
LGTM
这个更改了一个参数名，dirname -> path，可能会造成2.1和2.0在使用了这个API的部分场景中不兼容，但我个人支持做不兼容修改，有几方面原因：
- 目前paddle存储多个结果的save load接口全局都统一成存储后缀的形式了，这个接口属于漏网之鱼，有必要统一
- dirname这个名字表意明确，与前缀的含义不兼容，更改为path更加通用，长远看比较好
- 2.1和2.0属于大版本不同，少量不兼容问题也合理
- 这个接口是低频接口，使用场景不多，而且参数位置也没有改变，只要用户没有明确使用参数名，就可以继续使用
仅代表个人看法，具体还需要 @lanxianghit @jzhang533 @XiaoguangHu01 看一下
|
|
||
| def setUp(self): | ||
| self.save_path = "./nonexist_dir/fc" | ||
| import shutil |
There was a problem hiding this comment.
NIT (Not Important): However, since it is not so important and this PR needs many approves. I will approve it and you can change in next PR.
Python officially suggests to put imports at top of file, I think you should follow it in your case.
https://www.python.org/dev/peps/pep-0008/#imports
Let me explain my understanding of the pros and cons of importing at top of a file or in a function.
Importing at top of a file:
- More consistent with other programming language.
- We group imports all together and easy to organize
There are several cases I think we can import in a function:
- Conditional import, e.g.
if something:
import xxx
- You would like your function code to work correctly after copy the function code to other file (other file may not contain that import)
But I don't think here we meet the cases for importing in a function :-) so I would suggest to import at the top.
| fc_layer = SimpleFCLayer(3, 4, 2) | ||
| input_var = paddle.to_tensor(np.random.random([4, 3]).astype('float32')) | ||
| with fluid.dygraph.guard(): | ||
| dygraph_out, traced_layer = fluid.dygraph.TracedLayer.trace( |
There was a problem hiding this comment.
We should write new test cases for Paddle 2.0 path. So fluid.dygraph.TracedLayer should be changed to paddle.jit.TracedLayer. However, since it is not so important and this PR needs many approves. I will approve it and you can change in next PR.
There was a problem hiding this comment.
Thank you for your suggestion，I will change these two in next pr.
CtfGo
changed the title
CtfGo
changed the title
7 participants
PR types
Bug fixes
PR changes
APIs
Describe
update the TraceLayer.save_inference_model method:
(1) change the param "dirname" to "path" to indicate it is a path prefix of the saved model, and update docs
(2) save the program and params on the path with ".pdmodel" and ".pdiparams" suffix seperately