Skip to content

Conversation

@wanglei828
Copy link
Contributor

@wanglei828 wanglei828 commented Apr 18, 2018

Fix #10073

@wanglei828 wanglei828 requested review from luotao1 and wangkuiyi April 18, 2018 23:18
@luotao1
Copy link
Contributor

luotao1 commented Apr 19, 2018

@wanglei828

  • Could you write some backgroud why we need to refactor paddle bash shell scripts in the comments, since other people don't know the backgroud?
  • Could you write a simple command example in the comments, since other people don't know how to use this script?

@typhoonzero @Yancey1989
Could you help review this PR? If we update the scripts, we need to update the command on both TeamCity and documents, since we tell users to run following commands to build from sources now.

docker run -it -v $PWD:/paddle -e "WITH_GPU=OFF" -e "WITH_TESTING=OFF" paddlepaddle/paddle_manylinux_devel:cuda8.0_cudnn5 bash -x /paddle/paddle/scripts/docker/build.sh

@Yancey0623
Copy link
Contributor

FROM @luotao1

Could you help review this PR? I

Sure.

Thanks for @wanglei828's PR, could you create an issue to descript the background about this PR?

@panyx0718 panyx0718 self-requested a review April 19, 2018 04:53
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the indentation is not right.

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 fixed the indentation.

@wanglei828
Copy link
Contributor Author

I have created a issue to describe my goal (#10073)

@wangkuiyi
Copy link
Collaborator

Thanks to @wanglei828 for these very carefully developed scripts.

I see duplicated code snippets in the two bash scripts in this PR. I'd suggest to merge them into one.

Also, it would be great to update the documents, to add comments, or to add an issue, anyway, to describe the motivation and usage of the added script.

Given that we currently have multiple scripts for building, it would be helpful to explain what existing scripts could be deleted once after this PR is merged.

@wanglei828
Copy link
Contributor Author

@wangkuiyi @luotao1 I have updated the "README.md" file to describe how to use the new scripts, please help review. Thanks!

@wanglei828
Copy link
Contributor Author

@wangkuiyi Duplicate functions have been merged into one.

@wangkuiyi
Copy link
Collaborator

Thanks to @wanglei828 for this PR. I followed the issue and have some suggestions over there: #10073 (comment)

@wangkuiyi wangkuiyi merged commit b8b2e89 into PaddlePaddle:develop Apr 25, 2018
@wanglei828 wanglei828 deleted the fixscript branch April 26, 2018 02:49
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.

5 participants