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

[WIP][Torch] Seeds / Determinism #361

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

VivekPanyam
Copy link
Collaborator

@VivekPanyam VivekPanyam commented May 27, 2020

This PR implements Torch specific seed and determinism options.

Closes #325

@VivekPanyam VivekPanyam requested a review from selitvin May 27, 2020 04:41
@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.42%. Comparing base (30bbdbc) to head (8a6ad22).
Report is 122 commits behind head on master.

Files Patch % Lines
...rce/neuropod/backends/torchscript/torch_backend.cc 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
- Coverage   89.45%   89.42%   -0.03%     
==========================================
  Files          57       57              
  Lines        3918     3926       +8     
==========================================
+ Hits         3505     3511       +6     
- Misses        413      415       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VivekPanyam VivekPanyam changed the base branch from master to ope_options May 27, 2020 07:22
@VivekPanyam
Copy link
Collaborator Author

@selitvin Should we generalize these options more before landing or do you think it's fine to have framework-specific options in RuntimeOptions?

@@ -233,6 +233,9 @@ TorchNeuropodBackend::TorchNeuropodBackend(const std::string &neuropod_path, con

void TorchNeuropodBackend::load_model_internal()
{
at::globalContext().setDeterministicCuDNN(options_.torch_cudnn_deterministic);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we avoid making these calls all-together is a user opted out to reduce a chance of modifying the default behavior?

VivekPanyam added a commit that referenced this pull request May 28, 2020
This PR passes user-specified `RuntimeOptions` to the OPE worker process when loading a model.

This enables us to easily add options without having to write special logic for OPE.

See #361 and #364 for examples.
Base automatically changed from ope_options to master May 28, 2020 16:45
// Whether or not to run in deterministic mode. See https://pytorch.org/docs/stable/notes/randomness.html#cudnn
// Note: this currently only applies to TorchScript models and affects all torchscript models in the process.
// Should only be used with OPE to avoid this issue.
bool torch_cudnn_deterministic = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

My personal perferences:

Even we know that this is for Torch only for now, I would call it: backends_cudnn_deterministic and backends_cudnn_benchmark. And then comment specifies that currently this is Torch only. So, we are going to add more options probably in future and it is better to keep them generic and name accordingly (if only 1 backend supports it - fine, but it helps to see gaps/difference between backends and even can be a start point to add options to other framework.

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.

[Torch] Seeds / Determinism
3 participants