Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

CMake CUDA fixes + NCCL #9672

Merged
merged 10 commits into from
Feb 13, 2018
Merged

CMake CUDA fixes + NCCL #9672

merged 10 commits into from
Feb 13, 2018

Conversation

cjolivier01
Copy link
Member

Description

(Brief description on what this PR is about)

Checklist

Essentials

  • Passed code style checking (make lint)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

szha
szha reviewed Feb 1, 2018
View reviewed changes
CMakeLists.txt Outdated
find_package(CUDA REQUIRED)
add_definitions(-DMSHADOW_USE_CUDA=1)
if(FIRST_CUDA AND (NOT USE_OLDCMAKECUDA))
set(__cuda_toolset "7.5" "8.0" "9.0")
Copy link
Member

Choose a reason for hiding this comment

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

latest is 9.1. does the support for new cuda version have to be manually added?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't actually know -- this was not written by me. It was moved from somewhere else in this file.
I don;t actually know what this section does. I suppose that I can add 9.1, however.

Copy link
Member

Choose a reason for hiding this comment

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

@DickJC123 would you recommend how this section should be written?

szha
szha reviewed Feb 2, 2018
View reviewed changes
CMakeLists.txt
add_definitions(-DMSHADOW_USE_CUDA=1)
if(FIRST_CUDA AND (NOT USE_OLDCMAKECUDA))
if(CUDA_TOOLSET STREQUAL "")
set(CUDA_TOOLSET "${CUDA_VERSION_STRING}")
Copy link
Member

Choose a reason for hiding this comment

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

Is the CUDA_VERSION_STRING automagically set somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, in FindCUDA.cmake -- find_package(CUDA)

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if I want to compile using Cuda 8, although my system supports cuda 9 or does not have a GPU? Would it still pick Cuda 8 according to the configuration?

Copy link
Member Author

Choose a reason for hiding this comment

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

the old code (not written by me) defaulted to Cuda 8 no matter what unless CUDA_TOOLKIT var is set.
my change makes it so that it defaults to whatever version find_package(CUDA) finds, which is the default behavior in older versions (non FIRST_CUDA versions). this is still overridable by CUDA_TOOLKIT var. if you specify a version that way and CMake can’t find it, you get an error (CMake > 9.0 understands cuda compiling).
alternately, you can specify the cuda directory which will be picked up by find_package(CUDA). see cmake’s FindCUDA.cmake for details.

@cjolivier01 cjolivier01 force-pushed the cmake_fix branch 3 times, most recently from 606f8cd to 6a0ded1 Compare February 6, 2018 20:30
@cjolivier01
Copy link
Member Author

why is Windows python runs hanging?

@marcoabreu
Copy link
Contributor

I'd guess that the process is crashing due to some compilation errors, thus making nosetests fail. Maybe Jenkins does not pick this crash up. Interestingly, it's happening on all 4 windows jobs.

@cjolivier01
Copy link
Member Author

cjolivier01 commented Feb 12, 2018 •
edited
Loading

turns out related to project() not being at top, and thus MSVC not being turned on

@cjolivier01 cjolivier01 mentioned this pull request Feb 12, 2018
Closed
7 tasks
@cjolivier01 cjolivier01 merged commit 83f6279 into apache:master Feb 13, 2018
@cjolivier01 cjolivier01 deleted the cmake_fix branch February 13, 2018 06:04
@marcoabreu
Copy link
Contributor

Was this PR approved by anybody or was this a self merge?

@apache apache locked as resolved and limited conversation to collaborators Feb 13, 2018
@szha
Copy link
Member

szha commented Feb 15, 2018

CMake cuda build was broken, reported by @yajiedesign. Fix is in #9798

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@szha szha szha left review comments

@marcoabreu marcoabreu marcoabreu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cjolivier01 @marcoabreu @szha