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
Build Horovod with temporarily installed CMake if necessary #3371
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
c874211
Use CMake from a temp environment if system version is too old
maxhgerlach c3569be
Removed gpg install from Dockerfiles
maxhgerlach ef92e72
Update install docs
maxhgerlach d0dffc6
Don't preinstall CMake 3.13 in test Dockerfiles
maxhgerlach 6de2b48
Install CMake 3.13 in image for CodeQL
maxhgerlach 072ec9e
Broaden except clause to catch subprocess.CalledProcessError too
maxhgerlach 5ef9e2b
Use CMake from apt in containers based on Ubuntu 20.04
maxhgerlach File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
we could rely here on
setup.py
installing the latest cmake so we always test this really worksThere 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.
I believe that this apt-get line currently installs cmake 3.10 on the Ubuntu 18.04 we use for testing (too old to build Horovod). So yes, as it stands, we rely on setup.py to install a temporary CMake 3.13 to build Horovod and the mechanism from this PR is tested. This would change once we update to Ubuntu 20.04 or later.
I left cmake in here for any non-Horovod builds in this Docker container, e.g., for oneCCL.
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.
Ah, and just for the record: The ppc64le build on Jenkins currently comes with a pretty recent CMake on its own, so using a recent system CMake to build Horovod is also still under test.
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.
Ah, our ancient Ubuntu 18.04 means this
apt-get intall cmake
is not sufficient. Then it makes sense we need to add that extra code to the CodeQL yaml. Once we update to Ubuntu 20.04 we will lose testing that auto-install. We should remember when we upgrade to keep Ubuntu 18.04 for one of the test combinations, e.g. TF 1.15 to see that auto-install feature still being tested (as long as we want to support Ubuntu 18.04 in our test setup).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.
https://github.com/horovod/horovod/runs/4852098212?check_suite_focus=true#step:10:4652
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.
That's a good plan!
Yep, here we can see that it used a CMake binary from
/tmp
: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.
As a side note: In the meantime we have moved our test images to Ubuntu 20.04, but a few test images still use Ubuntu 18.04, so we now test both.