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

Delete unused Op dependence in CMakeList #4105

Merged
merged 2 commits into from
Sep 14, 2017

Conversation

Yancey1989
Copy link
Contributor

Fixed #4095

recurrent_op
scale_op)
op_library(identity_op DEPS scale_op)
op_library(minus_op DEPS scale_op)
Copy link
Contributor

Choose a reason for hiding this comment

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

如果直接make minus_op此时是否会无法找到依赖而无法编译呢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

应该不会,现在不能单独使用某一个Op,都是all in py_bind里的,而py_bind是会把所有的Op编进去的。

Copy link
Contributor

Choose a reason for hiding this comment

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

make minus_op能跑通么? 只想测C++端的代码写的对不对,不管后面的python绑定。

Copy link
Contributor Author

@Yancey1989 Yancey1989 Sep 14, 2017

Choose a reason for hiding this comment

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

make minus_op build是没问题的,但运行时可能会有找不到scale op的问题,不过怎么单独测试C++端的Code呢?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Xreki 使用c-api的时候,或者做嵌入式开发时,是不是需要能单独运行XXX_op? 上面的做法,把所有op打包到一个.a应该没问题,单独运行的话可能会有问题。

Copy link
Contributor

@Xreki Xreki Sep 14, 2017

Choose a reason for hiding this comment

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

  • 现在所有的op的单测都在python端,而编译单个op.a静态库也不需要依赖其他op。从目前的代码结构来看,移除这些依赖是没有问题的。
  • c-api需要将所有op、或者需要的部分op、以及其他所有的核心代码,编译打包成一个库。
    • 打包所有的op比较简单
    • 只打包一部分op(比如纯预测的库,不需要cost op),则需要显式指定所需要op+所依赖的op,这个难易程度也不好定义。。。

Copy link
Contributor

Choose a reason for hiding this comment

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

@luotao1 @Xreki 感觉可以先merge?目前没看到有单独运行或测试某个op的情况。如果有可以再发一些fix。这个PR可以减少一些cmake代码。

Copy link
Contributor

Choose a reason for hiding this comment

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

可以的,没问题。

@Yancey1989 Yancey1989 requested a review from Xreki September 14, 2017 08:28
Copy link
Contributor

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

LGTM

@luotao1 luotao1 merged commit 7a9c89f into PaddlePaddle:develop Sep 14, 2017
@Yancey1989 Yancey1989 deleted the fix_op_dep branch September 14, 2017 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants