Use default dtype for torch::tensor(floating_point_values) and torch::tensor(empty braced-init-list) when dtype is not specified #29632

yf225 · 2019-11-12T05:09:29Z

Stack from ghstack:

[REMOVED] #29633 Support C++ tensor advanced indexing
Use default dtype for torch::tensor(floating_point_values) and torch::tensor(empty braced-init-list) when dtype is not specified #29632 Use default dtype for torch::tensor(floating_point_values) and torch::tensor(empty braced-init-list) when dtype is not specified

This PR is BC-breaking in the following way:

Previously, C++ torch::tensor with an integer type (e.g. torch::tensor(1)) or a (nested) braced-init-list of integer types (e.g. torch::tensor({{1, 2}})) produces a tensor with the same dtype. Now it always produces a tensor of dtype torch::kLong (aka. int64_t). matching Python torch.tensor behavior.

Differential Revision: D18465819

…:tensor(empty braced-init-list) when dtype is not specified

kostmo · 2019-11-12T05:21:57Z

CircleCI build failures summary

As of commit 16137e2:

1/1 failures introduced in this PR
0/1 recognized as flaky

Here are the reasons each build failed:

Job	Step	Log excerpt
pytorch_xla_linux_xenial_py3_6_clang7_build	Build	`Failed to run '['/var/lib/jenkins/workspace/xla/scripts/generate_code.sh']'`

This comment was automatically generated by Dr. CI.
Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

This comment has been revised 30 time(s).

… and torch::tensor(empty braced-init-list) when dtype is not specified" Use default dtype for torch::tensor(floating_point_values) and torch::tensor(empty braced-init-list) when dtype is not specified gh-metadata: pytorch pytorch 29632 gh/yf225/66/head

zou3519 · 2019-11-12T18:13:11Z

test/cpp/api/support.h

  return count;
 }

+// A RAII, thread local (!) guard that changes default dtype upon


Just checking, torch::set_default_dtype changes the default dtype locally?

It seems global: https://github.com/pytorch/pytorch/blob/master/c10/core/DefaultDtype.cpp. Orthogonal to this PR, does this need synchronization?

EDIT: Do people design global guards like this in general? I'm only really familiar with pytorch's use of thread-local guards.

Oh sorry, I totally did not realize this was in test code. This is fine in testing code, I was wondering if it was OK as an API.

Thanks for the catch - I think this AutoDefaultDtypeMode guard in its original form was not safe to use if we ever run C++ tests in parallel in multiple threads, since a test in thread A might not have finished when another test in thread B changes the default dtype using torch::set_default_dtype. To address this problem, I made the following two changes:

Add static std::mutex default_dtype_mutex to AutoDefaultDtypeMode, so that usage of AutoDefaultDtypeMode is synchronized and thus always thread-safe.

Change other C++ tests that have set_default_dtype(...) to always use AutoDefaultDtypeMode to guard the whole scope of the test, so that access to set_default_dtype(...) is effectively synchronized within the C++ test suite. (It won't protect us against running a C++ test while changing default dtype through Python within the same process, but such usage seems more intentional than accidental, and the author of that test mechanism should be responsible for making sure such usage is sane.)

I think the AutoDefaultDtypeMode guard is pretty ad-hoc - for now it's only used for the C++ tests, and its RAII mechanism should work :D

Does gtest run tests in parallel / is it able to?

I don't think gtest by itself can run tests in parallel (https://cuhkszlib-xiaoxing.readthedocs.io/en/latest/external/gtest/googletest/docs/FAQ.html#does-google-test-support-running-tests-in-parallel), but there are some test runners that can make it work: https://github.com/google/gtest-parallel. Whether to make them run in parallel is likely out of scope for this PR though.

Based on the design of the tests (they only run single-threaded), it might be better to simplify the data structure by removing the mutex and writing a quick comment that this isn't thread-safe and that it doesn't matter because we only use one thread

zou3519 · 2019-11-12T18:18:37Z

Does it make sense to use the default dtype when the user does torch.tensor(1.0)? 1.0 is "always" a double when it's used in C++

zou3519 · 2019-11-12T18:19:59Z

Does it make sense to use the default dtype when the user does torch.tensor(1.0)? 1.0 is "always" a double when it's used in C++

On second thought, python floats also have double-precision and we use the default dtype for them.

test/cpp/api/tensor.cpp

zou3519 · 2019-11-12T18:27:50Z

torch/csrc/api/include/torch/detail/TensorDataContainer.h

+    // When `scalar_type == at::kDouble`, we know that the user is passing in
+    // a floating-point literal without specifying its type (e.g. `1.0` instead of `1.0f`).
+    // In Python, the dtype of `torch.tensor(1.0)` depends on the value of
+    // `torch.get_default_dtype()`, and we should do the same for C++ `torch::tensor(1.0)`.


It might be nice to have a parity test for this behavior. i.e., if we ever change the default tensor type behavior then we know to change it in both APIs. I don't see this changing anytime soon though so this is more of a nit.

Agreed - I will add it in a follow-up PR to improve the parity test mechanism.

… and torch::tensor(empty braced-init-list) when dtype is not specified" Use default dtype for torch::tensor(floating_point_values) and torch::tensor(empty braced-init-list) when dtype is not specified gh-metadata: pytorch pytorch 29632 gh/yf225/66/head

zou3519

Looks good. I think it would be nice to remove the mutex to simplify the logic (since our tests are single-threaded anyways and probably won't change anytime soon) but let me know your thoughts

zou3519 · 2019-11-13T17:22:16Z

test/cpp/api/support.h

  return count;
 }

+// A RAII, thread local (!) guard that changes default dtype upon


Based on the design of the tests (they only run single-threaded), it might be better to simplify the data structure by removing the mutex and writing a quick comment that this isn't thread-safe and that it doesn't matter because we only use one thread

yf225 · 2019-11-13T17:30:54Z

Looks good. I think it would be nice to remove the mutex to simplify the logic (since our tests are single-threaded anyways and probably won't change anytime soon) but let me know your thoughts

Thanks! I feel that it's probably worth keeping the mutex because changing the global state back and forth in the tests does seems a bit worrying to me, and it might cause hard-to-debug flaky tests if we ever run them in parallel without the mutex.

zou3519 · 2019-11-13T17:50:29Z

Looks good. I think it would be nice to remove the mutex to simplify the logic (since our tests are single-threaded anyways and probably won't change anytime soon) but let me know your thoughts

Thanks! I feel that it's probably worth keeping the mutex because changing the global state back and forth in the tests does seems a bit worrying to me, and it might cause hard-to-debug flaky tests if we ever run them in parallel without the mutex.

That's reasonable, let's keep it then.

facebook-github-bot · 2019-11-14T03:43:07Z

@yf225 merged this pull request in 2bcac59.

Use default dtype for torch::tensor(floating_point_values) and torch:…

3016786

…:tensor(empty braced-init-list) when dtype is not specified

yf225 requested review from ebetica and goldsborough as code owners November 12, 2019 05:09

yf225 mentioned this pull request Nov 12, 2019

[REMOVED] #29633

Closed

Will Feng added 2 commits November 12, 2019 00:44

yf225 added module: bc-breaking Related to a BC-breaking change module: cpp Related to C++ API labels Nov 12, 2019

yf225 requested review from zou3519 and removed request for ebetica and goldsborough November 12, 2019 06:13

zou3519 reviewed Nov 12, 2019

View reviewed changes

test/cpp/api/tensor.cpp Show resolved Hide resolved

zou3519 reviewed Nov 12, 2019

View reviewed changes

Will Feng added 6 commits November 12, 2019 17:46

yf225 mentioned this pull request Nov 12, 2019

Add more tests for torch::arange #29689

Closed

yf225 requested a review from zou3519 November 13, 2019 02:51

Will Feng added 2 commits November 12, 2019 23:30

zou3519 approved these changes Nov 13, 2019

View reviewed changes

facebook-github-bot closed this in 2bcac59 Nov 13, 2019

facebook-github-bot added the merged label Nov 14, 2019

facebook-github-bot deleted the gh/yf225/66/head branch November 17, 2019 15:15

yf225 mentioned this pull request Dec 13, 2019

C++ torch::tensor by default gives a double tensor, which is different from Python torch.tensor behavior #28902

Closed

mruberry added the Merged label Oct 28, 2020

driazati mentioned this pull request Apr 20, 2021

Remove is_variable from tests #56305

Closed

Use default dtype for torch::tensor(floating_point_values) and torch::tensor(empty braced-init-list) when dtype is not specified #29632

Use default dtype for torch::tensor(floating_point_values) and torch::tensor(empty braced-init-list) when dtype is not specified #29632

Uh oh!

Conversation

yf225 commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kostmo commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CircleCI build failures summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zou3519 Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yf225 Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zou3519 commented Nov 12, 2019

Uh oh!

zou3519 commented Nov 12, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yf225 commented Nov 13, 2019

Uh oh!

zou3519 commented Nov 13, 2019

Uh oh!

facebook-github-bot commented Nov 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yf225 commented Nov 12, 2019 •

edited

Loading

kostmo commented Nov 12, 2019 •

edited

Loading

zou3519 Nov 12, 2019 •

edited

Loading

yf225 Nov 13, 2019 •

edited

Loading