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

Scripts: refactor paddle bash shell scripts. #10038

Merged
merged 3 commits into from
Apr 25, 2018

Conversation

wanglei828
Copy link
Contributor

@wanglei828 wanglei828 commented Apr 18, 2018

Fix #10073

@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

@Yancey1989
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
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
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