Repository navigation
Add cuDNN deterministic env variable (only for convolution) - #24355
duncanriach wants to merge 1 commit into
Conversation
|
|
||
| CudnnSupport::CudnnSupport(CUDAExecutor* parent) : parent_(parent) {} | ||
| CudnnSupport::CudnnSupport(CUDAExecutor* parent) : parent_(parent) { | ||
| tensorflow::ReadBoolFromEnvVar("TF_CUDNN_DETERMINISTIC", false, |
There was a problem hiding this comment.
Since it's only for convolution, maybe renamed it to TF_CUDNN_DETERMINISTIC_CONV, and cudnn_deterministic_conv_?
There was a problem hiding this comment.
I'm happy with that if it's preferable to others. I chose the more generic name because the intention would be to incrementally enhance its effect so that it makes all cuDNN functionality deterministic (e.g. including pooling). The longer-term goal is to switch all of these modes on with a single ConfigProto bool field (e.g. deterministic), which may include functionality beyond cuDNN.
There was a problem hiding this comment.
Ah, that intention is fine, and TF_CUDNN_DETERMINISTIC is the right name for it. In this case, can you add a TODO(cudnn), saying that this env var needs to support all APIs in cudnn, but currently only supports conv?
There was a problem hiding this comment.
Will do. I'll add that above where the env var is read in the constructor.
There was a problem hiding this comment.
This is done. I added the number of this PR, rather than "cudnn" (which seems ambiguous), so that this conversation can be referenced. Is that acceptable?
There was a problem hiding this comment.
Got it. I'm making that change now.
There was a problem hiding this comment.
Done. Thanks for the guidance and patience.
There was a problem hiding this comment.
Please could someone tell me what "Google internal checks FAILED" means? I cannot see the output from that.
There was a problem hiding this comment.
Sorry for not being responsive. The error I saw was unused result of the call to ReadBoolFromEnvVar(). It returns a Status, and the potential error should be handled.
There was a problem hiding this comment.
Awesome. Thanks, Tim. I'll take account of that when I re-open this request.
6b3e031 to
adc157c
Compare
adc157c to
4785d40
Compare
|
There are some other refinements that I may want to make to this request before it gets pulled. I am going to close it for now, and I may re-open it later. |
|
Because I force-pushed the branch that this pull request is based on, while it was closed, it's not possible to re-open the pull request. I will have to create a new pull request. |
|
See follow-up pull request #24747. |
6 participants
This change is a major component of the recipe for making TensorFlow training reproducible on GPUs.
Setting the environment variable TF_CUDNN_DETERMINISTIC=1 (or true) will ensure that both forward and backwards convolution algorithms are both fixed and deterministic. It overrides autotune and selects deterministic back-prop algorithms.
Attention @azaks2
Note: This pull request was previous issued incorrectly based on r1.12.