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

Exposing use_cudnn #7536

Merged

Conversation

chengduoZH
Copy link
Contributor

@chengduoZH chengduoZH commented Jan 15, 2018

fix #7582

What this PR does includes:

  1. Exposing use_cudnn to python interface.
  2. In Python side, use_cudnn is set True.
  3. In C++ side, use_cudnn = use_cudnn && is_gpu_place(ctx.GetPlace());

@chengduoZH chengduoZH force-pushed the feature/refine_conv_pool_python branch from 839a4ee to b7f1a75 Compare January 15, 2018 12:58
@wangkuiyi
Copy link
Collaborator

@chengduoZH We need an issue to describe the problem to be solved by this PR.

@chengduoZH
Copy link
Contributor Author

@wangkuiyi Thanks!
I have added an issue to describe the problem.

pool_stride=1,
pool_type=None):
pool_type=None,
pool_use_cudnn=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use_cudnn, as the last argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

typhoonzero
typhoonzero previously approved these changes Jan 17, 2018
Copy link
Contributor

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

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

LGTM++

@typhoonzero typhoonzero dismissed their stale review January 17, 2018 02:46

Agree with @dzhwinter's comments should be fixed.

dzhwinter
dzhwinter previously approved these changes Jan 17, 2018
Copy link
Contributor

@dzhwinter dzhwinter left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -70,6 +70,13 @@ void ConvOp::InferShape(framework::InferShapeContext* ctx) const {
framework::OpKernelType ConvOp::GetExpectedKernelType(
const framework::ExecutionContext& ctx) const {
bool use_cudnn = ctx.Attr<bool>("use_cudnn");
use_cudnn &= platform::is_gpu_place(ctx.GetPlace());
Copy link
Member

Choose a reason for hiding this comment

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

I think use_cudnn means that user wants to force use cudnn operator, if env does not support cudnn, the framework should throw an error but not implicit change to use other operator.

Copy link
Contributor

@dzhwinter dzhwinter Jan 17, 2018

Choose a reason for hiding this comment

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

Agree with the comment.
But we need a default policy right now, by default, we need to set use_cudnn True.
As we discussed offline, we reach the agreement that place is also a user configuration. we need to overwrite our default policy, not user specified.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I get the point, seems we still need a priority mechanism to resolve such kind of problem in the future.

jacquesqiao
jacquesqiao previously approved these changes Jan 18, 2018
Copy link
Member

@jacquesqiao jacquesqiao left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jacquesqiao jacquesqiao left a comment

Choose a reason for hiding this comment

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

LGTM

@chengduoZH chengduoZH merged commit f086ebb into PaddlePaddle:develop Jan 18, 2018
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.

use_cudnn should expose to python interface.
5 participants