-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXNET-1191] Maven Jenkins Pipeline with Static Binary and GPU #13767
Conversation
@mxnet-label-bot Add [pr-awaiting-review] |
@mxnet-label-bot add [CI, Java, Maven, Scala] |
Please do a rebase with master |
ci/docker/install/ubuntu_base.sh
Outdated
# the whole docker cache for the image | ||
|
||
set -ex | ||
apt-get update || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why add the boolean OR true condition here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "|| true" means that the line will always evaluate with a zero exit code. For scripts with "set -e", any line evaluating with a nonzero exit code will exit the script. I am not sure why we ignore problems on the update, but this is done throughout the CI install scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super! Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes, some apt repositories are not available and apt-get update fails. If that's the case, we still want to go ahead since all other repositories still updated (think of 1 out of 500 repositories not being available). If the repository we actually need for the installation is actually not available, the install will fail properly and that's fine. But in most cases, it's just an unused and unrelated repository that's not available and thus we just ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a3c7d46 LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran it for a couple of times on my local computer and it is working just fine. Also see @zachgk tested a couple of times on Jenkins, overall looks good.
Description
This PR is the third and final part of the maven Jenkins publishing that adds integration with the static building scripts placed in tools as well as enabling GPU.
Note that this PR depends on (and is based on top of until they are merged) #13450 and #13707. The changes as part of this PR are only those located in a3c7d46.
Please review so the pipeline can be put into production:
@nswamy @lanking520 @andrewfayres @piyushghai @marcoabreu @yzhliu @szha
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.