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

Conversation

@lebeg
Copy link
Contributor

@lebeg lebeg commented Apr 16, 2018

Description

The version checking of cmake was buggy and confusing.

Checklist

Essentials

  • The PR title does not start with [MXNET-$JIRA_ID], since it's a minor change
  • Changes are complete
  • Examples are either not affected by this change

Changes

  • Moved cmake version tracing out of CUDA branching
  • Removed trivial not using CUDA branch
  • Extracted f16 support determination to separate cmake module

@lebeg lebeg requested a review from szha as a code owner April 16, 2018 15:37
@lebeg
Copy link
Contributor Author

lebeg commented Apr 16, 2018

It seems that it's failing due to not picking up MSHADOW_USE_F16C on windows, which should be accoring to dmlc/mshadow@0b4cedd

@marcoabreu can i clean and restart the job somehow without empty triggering commits?

Copy link
Member

@cjolivier01 cjolivier01 left a comment

Choose a reason for hiding this comment

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

There would need to be a CI that checked both USE_OLDCMAKECUDA on and off

@marcoabreu
Copy link
Contributor

@lebeg I granted you the permission

@yajiedesign
Copy link
Contributor

yajiedesign commented Apr 17, 2018

dmlc/mshadow#333 fix fp16 with msvc

@rahul003
Copy link
Member

But the CMakefile doesn't set F16C for MSVC. https://github.com/dmlc/mshadow/blob/0b4cedd7015cc69191f8338a8feaacda90697758/cmake/mshadow.cmake#L51
I'm not sure why mshadow is picking that up.
Any idea?

@yajiedesign
Copy link
Contributor

@rahul003 because when use first_cuda.the cmake/mshadow.cmake not include.

@szha szha requested review from yajiedesign and removed request for szha May 21, 2018 22:33
@lebeg lebeg requested a review from marcoabreu as a code owner May 24, 2018 11:53
@lebeg
Copy link
Contributor Author

lebeg commented May 24, 2018

@cjolivier01 can you take another look?

mxnet_option(USE_SIGNAL_HANDLER "Print stack traces on segfaults." OFF)

if(USE_CUDA AND NOT USE_OLDCMAKECUDA)
message(STATUS "CMake version '${CMAKE_VERSION}' using generator '${CMAKE_GENERATOR}'")
Copy link
Contributor

Choose a reason for hiding this comment

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

can we leave the diagnostics about cmake version and generator?

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 moved it to log unconditionally at the top of the file

@larroy
Copy link
Contributor

larroy commented May 24, 2018

LGTM

@marcoabreu
Copy link
Contributor

@cjolivier01 ping again

@lebeg lebeg mentioned this pull request Jun 7, 2018
4 tasks
@larroy
Copy link
Contributor

larroy commented Jun 11, 2018

@lebeg @cjolivier01 can we get this merged?

@lebeg
Copy link
Contributor Author

lebeg commented Jun 11, 2018

There are still problems for the windows build for some reason.

@yajiedesign
Copy link
Contributor

windows build is some problem.

@lebeg
Copy link
Contributor Author

lebeg commented Jun 12, 2018

I will look into it as soon as I get some time.

@lupesko
Copy link
Contributor

lupesko commented Aug 6, 2018

@lebeg - ping

@lupesko
Copy link
Contributor

lupesko commented Aug 21, 2018

@lebeg - bounce

Copy link
Contributor

@yajiedesign yajiedesign left a comment

Choose a reason for hiding this comment

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

please fix windows build.

@lupesko
Copy link
Contributor

lupesko commented Sep 14, 2018

@lebeg - any update?

@MuhammadNazir
Copy link

Kenapa litecoin tidak bisa masuk

@kalyc
Copy link
Contributor

kalyc commented Sep 14, 2018

Any update on the PR @lebeg
@mxnet-label-bot[pr-awaiting-response]

@marcoabreu marcoabreu added the pr-awaiting-response PR is reviewed and waiting for contributor to respond label Sep 14, 2018
@vandanavk
Copy link
Contributor

@sandeep-krishnamurthy Please change label to pr-awaiting-testing

@sandeep-krishnamurthy sandeep-krishnamurthy added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Sep 28, 2018
@lebeg
Copy link
Contributor Author

lebeg commented Sep 30, 2018

This PR is already a bit old and now it depends on #12331. I will take a look at updating it as soon as possible.

@Roshrini
Copy link
Member

@lebeg Can you resolve conflicts?

@roywei
Copy link
Member

roywei commented Oct 29, 2018

ping @lebeg could you fix the conflicts? once #12331 is merged this one is good to go.

@ankkhedia
Copy link
Contributor

@lebeg #12331 has been merged now. Could you please take a look?

@anirudhacharya
Copy link
Member

@nswamy @sandeep-krishnamurthy the PR seems to be older than a month. can you please close it.

@lebeg feel free to reopen the PR once the changes are ready

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

Labels

pr-awaiting-testing PR is reviewed and waiting CI build and test

Projects

None yet

Development

Successfully merging this pull request may close these issues.