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

ci: Add GPU builds #193

Merged
merged 3 commits into from May 10, 2020
Merged

Conversation

ddn0
Copy link
Contributor

@ddn0 ddn0 commented May 7, 2020

The CI won't pass until at least these issues are resolved:

(just for historical context, I noticed these errors while discussing #92 though they are not related to that issue itself)

Closes #190

@ddn0 ddn0 marked this pull request as draft May 7, 2020 22:11
@ddn0 ddn0 marked this pull request as ready for review May 8, 2020 01:33
openmpi-bin \
ssh

cmake -S . -B /tmp/build -DENABLE_DIST_GALOIS=ON -DENABLE_HETERO_GALOIS=ON
Copy link
Member

Choose a reason for hiding this comment

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

-> cmake -S . -B /tmp/build -DGALOIS_ENABLE_DIST=ON -DGALOIS_ENABLE_GPU=ON

openmpi-bin \
ssh

cmake -S . -B /tmp/build -DENABLE_DIST_GALOIS=ON -DENABLE_HETERO_GALOIS=ON
Copy link
Member

Choose a reason for hiding this comment

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

-> cmake -S . -B /tmp/build -DGALOIS_ENABLE_DIST=ON -DGALOIS_ENABLE_GPU=ON

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reminding me! Though this change only needed if this PR goes in after #199 . Otherwise it is actually correct as it stands.

@@ -1,6 +1,3 @@
[submodule "bliss"]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed? I thought this was required for pangolin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already vendored in external directly rather than as a submodule. The submodule itself was just something @chenxuhao made to track bliss upstream but it seems simpler to avoid inserting an intermediary.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's better it's not a submodule but that is not related to this PR. As long as pangolin builds don't break, it's fine to be included in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, the reason the commit is here is because we have to enable submodules for GPU builds, so I took the opportunity to clean up our submodule configuration, which has these unused entries.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, makes sense.

@roshandathathri
Copy link
Member

LGTM

ddn0 added 3 commits May 8, 2020 10:54
Now that we do not have the broken LCI submodule, we can checkout
submodules without error.
Copy link
Member

@insertinterestingnamehere insertinterestingnamehere left a comment

Choose a reason for hiding this comment

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

It'll be great to have these working. Thanks for putting this together. One thing wasn't obvious to me: why do we need to check out submodules in all the builds when the submodules are really only used for the GPU code?

@@ -2,11 +2,8 @@ dist: bionic

language: c++

# At the moment, our LCI submodule has an additional submodule within it

Choose a reason for hiding this comment

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

Good catch. This comment is outdated now that the LCI submodule is gone.

@ddn0
Copy link
Contributor Author

ddn0 commented May 8, 2020

It'll be great to have these working. Thanks for putting this together. One thing wasn't obvious to me: why do we need to check out submodules in all the builds when the submodules are really only used for the GPU code?

You are correct, but I figured it would be easier for maintenance and reduce the number of moving parts if we just standardized what it means to check out the repo (checked out or not vs checked out with submodules, checked out without ....). Alternatively, I imagine my future self to be a little surprised to see CI fail if I added a submodule that the CPU code needed, and my future self would have to add this same submodule checkout code anyway.

@ddn0 ddn0 merged commit df63cfd into IntelligentSoftwareSystems:master May 10, 2020
@ddn0 ddn0 deleted the 190-add-gpu-builds branch May 10, 2020 20:06
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.

Add GPU builds in CI
3 participants