Skip to content

Conversation

@wanglei828
Copy link
Contributor

No description provided.

@wanglei828 wanglei828 requested a review from panyx0718 May 31, 2018 03:43
@panyx0718 panyx0718 requested a review from luotao1 May 31, 2018 04:36
@panyx0718
Copy link
Contributor

@luotao1 Can you give some advice. I'm not very good at it.

There is one TODO for the cmake that I haven't fixed:
I haven't make the test depending on test_word2vec and test_image_classification yet.

@panyx0718
Copy link
Contributor

@wanglei828 CI is failing for this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

set_tests_properties(test_inference_${TARGET_NAME}${arg}
PROPERTIES DEPENDS test_${TARGET_NAME})

python单测的名字是test_xxx,而inference_test_ARGS的名字是api_impl,没对上导致依赖没生效。

所以仿照paddle/fluid/inference/tests/book/CMakeLists.txt再来改一下就可以了。

Copy link
Contributor Author

@wanglei828 wanglei828 May 31, 2018

Choose a reason for hiding this comment

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

@luotao1 I think you may not understand this correctly. In the file, I call the function like this:
inference_api_test(api_impl
ARGS test_word2vec test_image_classification)
after parsing,
the variable "inference_test_ARGS" should be "test_word2vec; test_image_classification" which is correct.
when I use:
set_tests_properties(test_paddle_inference_${TARGET_NAME}
PROPERTIES DEPENDS "${inference_test_ARGS}")
it will be interpreted as this:
set_tests_properties(test_paddle_inference_api_impl PROPERTIES DEPENDS "test_word2vec; test_image_classification" )

You can read this reference: https://cmake.org/cmake/help/v3.0/module/CMakeParseArguments.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it.

@wanglei828
Copy link
Contributor Author

@panyx0718 I fixed the CI fail. It is mainly because the test cases are not put in the "WITH_TESTING" control flow.

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!

cc_test(test_paddle_inference_api
SRCS test_paddle_inference_api.cc
DEPS paddle_inference_api)
if(WITH_TESTING)
Copy link
Contributor

Choose a reason for hiding this comment

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

cc_test里面会判断if(WITH_TESTING),这里不需要加了。

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it.

@wanglei828 wanglei828 merged commit 86efecb into PaddlePaddle:develop Jun 1, 2018
@wanglei828 wanglei828 deleted the fixdep branch June 1, 2018 02:23
@panyx0718
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants