## Author Notes
Pretty mechanical, nothing special here.
## Agent Notes
Written by Claude Code:
> The two test directories are 48 findings, and they come apart cleanly: 43 are
> stubs and error checks that convert mechanically, and 5 are throws whose type is
> the point of the test they sit in. Getting those 5 wrong would leave the suite
> green while testing the wrong thing, which is the only real risk in this PR.
>
> The 43: `rng_extension.cpp` and `cpp_c10d_extension.cpp` are "not implemented" /
> "does not support" overrides (8 and 10); `aoti_inference/test.cpp` is CUDA API
> error checks (16); the rest are `aoti_custom_class.cpp`,
> `test_custom_class_registrations.cpp` (6, a toy stack machine's error paths) and
> the two `my_from_blob_*` extensions. None of their messages is asserted on
> anywhere - I grepped the tree for each, including the Python suites.
>
> The two `my_from_blob_*` sites get STD_TORCH_CHECK rather than TORCH_CHECK.
> Those TUs include only `torch/csrc/stable/*`, which reaches
> torch/headeronly/util/Exception.h but never c10/util/Exception.h, so TORCH_CHECK
> is not declared there at all. STD_TORCH_CHECK preserves the exception type
> exactly - it throws std::runtime_error, same as the raw throw - but not the
> message: like the serialization-codegen PR earlier in this stack, it prepends
> __func__, __FILE__ and __LINE__, so
>
> before: cudaMalloc failed
> after: my_from_blob_with_cuda_deleter, <path>/my_from_blob_with_deleter.cpp:73,
> cudaMalloc failed
>
> These are the only two sites in this PR where anything observable moves. Nothing
> asserts on either string, both are behind #ifdef LAE_USE_CUDA in a test
> extension, and STD_TORCH_CHECK is already the house style in the sibling
> libtorch_agn_2_* extensions (17 uses), so their messages are decorated the same
> way. The alternative was to leave these two raw and annotated; taking the
> decoration keeps the file consistent with its neighbours.
>
> The 5 that stay raw, with the reason on each site:
>
> ProcessGroupNCCLErrorsTest.cpp TestHeartbeatMonitor::runLoop, 16 lines
> above, catches std::runtime_error by name
> to set hasMonitorThreadCaughtError_. A
> c10::Error is not caught by that, and the
> throw happens on the monitor thread.
> dataloader.cpp The test rethrows e.original_exception and
> asserts it is still a std::invalid_argument
> - the point being that WorkerException
> round-trips the original type.
> parallel.cpp ParallelApplyRethrowsException is about a
> user exception escaping forward().
> FileStoreTest.cpp std::system_error carrying errno as a
> std::error_code.
> OpenRegException.cpp Reports a caller-supplied SourceLocation;
> a TORCH_CHECK there would stamp this
> file's own location instead.
>
> Two of those five are load-bearing in a way I could measure rather than argue,
> and the test plan shows the failures. `parallel.cpp` is the one that is not:
> its assertion is `ASSERT_THROWS_WITH(..., "Badness!")`, a substring match that
> would still pass after conversion. It is annotated for intent, not because it
> would break, and it seemed worth being explicit about that difference rather
> than implying all five were equally forced.
>
> The exclude list goes from 11 entries to 9.
>
> Test Plan:
>
> Half of this is CUDA or distributed and does not build here, so the two CUDA
> files - parallel.cpp and ProcessGroupNCCLErrorsTest.cpp - deliberately received
> comment-only edits, which is the safest possible change to code a configuration
> cannot compile.
>
> For the rest, what matters is whether the *changed lines* were compiled, not the
> files: three of them sit behind preprocessor guards that a plain CPU build skips
> straight over. `aoti_inference/test.cpp`'s 16 conversions are inside
> `#if defined(USE_CUDA) || defined(USE_ROCM)`, and the two `my_from_blob_*` sites
> are inside `#ifdef LAE_USE_CUDA`, which setup.py only defines when CUDA is
> available. The two my_from_blob sites were compiled with the guard forced on,
> against a stub cuda_runtime.h. There is no CUDA toolkit here, so the
> aoti_inference block was not compiled; what I checked instead is that
> `<torch/torch.h>` is included unconditionally above it and does provide
> TORCH_CHECK, and that the block contained no TORCH_CHECK before this change, so
> the macro's availability there is an argument rather than a measurement. Every
> other converted line is unguarded.
>
> BUILD_TEST=0 does not prevent compiling these directly:
>
> aoti_inference/test.cpp syntax-only (needs gmock headers)
> aoti_custom_class.cpp syntax-only
> dataloader.cpp 87 tests passed
> test_custom_class_registrations.cpp compiles and links (no TESTs of its own)
> FileStoreTest.cpp syntax-only
> cpp_c10d_extension.cpp syntax-only (needs Python.h, pybind11)
> rng_extension.cpp syntax-only (same)
> my_from_blob_with_deleter.cpp syntax-only
> my_from_blob_with_lambda_deleter.cpp syntax-only
> OpenRegException.cpp syntax-only (needs the openreg headers)
>
> ```
> g++ -std=c++20 <includes> test/cpp/api/dataloader.cpp gtest-all.o gtest_main.o \
> -Lbuild/lib -lc10 -ltorch_cpu -lpthread && ./a.out
> [ PASSED ] 87 tests.
> ```
>
> Positive control on the dataloader annotation - the same file with the throw
> converted:
>
> ```
> [ FAILED ] DataLoaderTest.TestExceptionsArePropagatedFromWorkers
> ```
>
> So that annotation is required, not preference.
>
> ```
> lintrunner --take RAWTHROW --all-files
> spin lint
> ```
>
> Both clean; test/cpp and test/cpp_extensions go from 48 findings to 0.
> CLANGFORMAT rewrapped three of the new comments, applied via `spin fixlint`.
>
> One scope caveat worth stating plainly: "48 findings to 0" is about the linter's
> view, not the directories. RAWTHROW's include_patterns are `**/*.cpp` and
> `**/*.h`, so four raw throws here stay invisible - three std::system_error sites
> in `test/cpp/c10d/TestUtils.hpp` (two of which are literal copies of the throw
> this PR annotates in FileStoreTest.cpp) and a CUDA error-check macro in
> `test/cpp/aoti_abi_check/cuda/test_kernel_utils.cu`. A planned later phase
> widens include_patterns to .hpp/.cu and picks them up.
>
> One coupling to carry forward, in a file this PR touches:
> `aoti_inference/test.cpp:301` catches `const std::runtime_error&` around
> `update_constant_buffer` and matches "API call failed at" at :302. That throw comes from
> `torch/headeronly/util/shim_utils.h`, which is annotated and unconverted today -
> if a later phase converts it to TORCH_CHECK rather than STD_TORCH_CHECK, this
> catch stops matching.
>
> Not verified locally: the CUDA and distributed builds, and CLANGTIDY.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/194773
Approved by: https://github.com/ezyang
ghstack dependencies: #194379, #194772
## Author Notes
Pretty mechanical, nothing special here.
## Agent Notes
Written by Claude Code:
> The two test directories are 48 findings, and they come apart cleanly: 43 are
> stubs and error checks that convert mechanically, and 5 are throws whose type is
> the point of the test they sit in. Getting those 5 wrong would leave the suite
> green while testing the wrong thing, which is the only real risk in this PR.
>
> The 43: `rng_extension.cpp` and `cpp_c10d_extension.cpp` are "not implemented" /
> "does not support" overrides (8 and 10); `aoti_inference/test.cpp` is CUDA API
> error checks (16); the rest are `aoti_custom_class.cpp`,
> `test_custom_class_registrations.cpp` (6, a toy stack machine's error paths) and
> the two `my_from_blob_*` extensions. None of their messages is asserted on
> anywhere - I grepped the tree for each, including the Python suites.
>
> The two `my_from_blob_*` sites get STD_TORCH_CHECK rather than TORCH_CHECK.
> Those TUs include only `torch/csrc/stable/*`, which reaches
> torch/headeronly/util/Exception.h but never c10/util/Exception.h, so TORCH_CHECK
> is not declared there at all. STD_TORCH_CHECK preserves the exception type
> exactly - it throws std::runtime_error, same as the raw throw - but not the
> message: like the serialization-codegen PR earlier in this stack, it prepends
> __func__, __FILE__ and __LINE__, so
>
> before: cudaMalloc failed
> after: my_from_blob_with_cuda_deleter, <path>/my_from_blob_with_deleter.cpp:73,
> cudaMalloc failed
>
> These are the only two sites in this PR where anything observable moves. Nothing
> asserts on either string, both are behind #ifdef LAE_USE_CUDA in a test
> extension, and STD_TORCH_CHECK is already the house style in the sibling
> libtorch_agn_2_* extensions (17 uses), so their messages are decorated the same
> way. The alternative was to leave these two raw and annotated; taking the
> decoration keeps the file consistent with its neighbours.
>
> The 5 that stay raw, with the reason on each site:
>
> ProcessGroupNCCLErrorsTest.cpp TestHeartbeatMonitor::runLoop, 16 lines
> above, catches std::runtime_error by name
> to set hasMonitorThreadCaughtError_. A
> c10::Error is not caught by that, and the
> throw happens on the monitor thread.
> dataloader.cpp The test rethrows e.original_exception and
> asserts it is still a std::invalid_argument
> - the point being that WorkerException
> round-trips the original type.
> parallel.cpp ParallelApplyRethrowsException is about a
> user exception escaping forward().
> FileStoreTest.cpp std::system_error carrying errno as a
> std::error_code.
> OpenRegException.cpp Reports a caller-supplied SourceLocation;
> a TORCH_CHECK there would stamp this
> file's own location instead.
>
> Two of those five are load-bearing in a way I could measure rather than argue,
> and the test plan shows the failures. `parallel.cpp` is the one that is not:
> its assertion is `ASSERT_THROWS_WITH(..., "Badness!")`, a substring match that
> would still pass after conversion. It is annotated for intent, not because it
> would break, and it seemed worth being explicit about that difference rather
> than implying all five were equally forced.
>
> The exclude list goes from 11 entries to 9.
>
> Test Plan:
>
> Half of this is CUDA or distributed and does not build here, so the two CUDA
> files - parallel.cpp and ProcessGroupNCCLErrorsTest.cpp - deliberately received
> comment-only edits, which is the safest possible change to code a configuration
> cannot compile.
>
> For the rest, what matters is whether the *changed lines* were compiled, not the
> files: three of them sit behind preprocessor guards that a plain CPU build skips
> straight over. `aoti_inference/test.cpp`'s 16 conversions are inside
> `#if defined(USE_CUDA) || defined(USE_ROCM)`, and the two `my_from_blob_*` sites
> are inside `#ifdef LAE_USE_CUDA`, which setup.py only defines when CUDA is
> available. The two my_from_blob sites were compiled with the guard forced on,
> against a stub cuda_runtime.h. There is no CUDA toolkit here, so the
> aoti_inference block was not compiled; what I checked instead is that
> `<torch/torch.h>` is included unconditionally above it and does provide
> TORCH_CHECK, and that the block contained no TORCH_CHECK before this change, so
> the macro's availability there is an argument rather than a measurement. Every
> other converted line is unguarded.
>
> BUILD_TEST=0 does not prevent compiling these directly:
>
> aoti_inference/test.cpp syntax-only (needs gmock headers)
> aoti_custom_class.cpp syntax-only
> dataloader.cpp 87 tests passed
> test_custom_class_registrations.cpp compiles and links (no TESTs of its own)
> FileStoreTest.cpp syntax-only
> cpp_c10d_extension.cpp syntax-only (needs Python.h, pybind11)
> rng_extension.cpp syntax-only (same)
> my_from_blob_with_deleter.cpp syntax-only
> my_from_blob_with_lambda_deleter.cpp syntax-only
> OpenRegException.cpp syntax-only (needs the openreg headers)
>
> ```
> g++ -std=c++20 <includes> test/cpp/api/dataloader.cpp gtest-all.o gtest_main.o \
> -Lbuild/lib -lc10 -ltorch_cpu -lpthread && ./a.out
> [ PASSED ] 87 tests.
> ```
>
> Positive control on the dataloader annotation - the same file with the throw
> converted:
>
> ```
> [ FAILED ] DataLoaderTest.TestExceptionsArePropagatedFromWorkers
> ```
>
> So that annotation is required, not preference.
>
> ```
> lintrunner --take RAWTHROW --all-files
> spin lint
> ```
>
> Both clean; test/cpp and test/cpp_extensions go from 48 findings to 0.
> CLANGFORMAT rewrapped three of the new comments, applied via `spin fixlint`.
>
> One scope caveat worth stating plainly: "48 findings to 0" is about the linter's
> view, not the directories. RAWTHROW's include_patterns are `**/*.cpp` and
> `**/*.h`, so four raw throws here stay invisible - three std::system_error sites
> in `test/cpp/c10d/TestUtils.hpp` (two of which are literal copies of the throw
> this PR annotates in FileStoreTest.cpp) and a CUDA error-check macro in
> `test/cpp/aoti_abi_check/cuda/test_kernel_utils.cu`. A planned later phase
> widens include_patterns to .hpp/.cu and picks them up.
>
> One coupling to carry forward, in a file this PR touches:
> `aoti_inference/test.cpp:301` catches `const std::runtime_error&` around
> `update_constant_buffer` and matches "API call failed at" at :302. That throw comes from
> `torch/headeronly/util/shim_utils.h`, which is annotated and unconverted today -
> if a later phase converts it to TORCH_CHECK rather than STD_TORCH_CHECK, this
> catch stops matching.
>
> Not verified locally: the CUDA and distributed builds, and CLANGTIDY.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/194773
Approved by: https://github.com/pytorchgreenlight
ghstack dependencies: #194378, #194379, #194772
This PR enables `-Winconsistent-missing-destructor-override` and `-Winconsistent-missing-override`
and fixes violations.
<!--
copilot:summary
-->
### <samp>🤖 Generated by Copilot at 47e904e</samp>
This pull request updates the code of various classes and operators in the `caffe2` and `aten` subdirectories to use the `override` specifier instead of the `virtual` keyword for destructors and other virtual functions that override a base class function. This improves the code readability, quality, and consistency with C++ best practices. It also modifies the `./CMakeLists.txt` file to enable warnings for these specifiers, but disable errors.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/104032
Approved by: https://github.com/malfet
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/69394
Modified loops in files under fbsource/fbcode/caffe2/ from the format
```
for(TYPE var=x0;var<x_max;x++)
```
to the format
```
for(const auto var: irange(xmax))
```
This was achieved by running r-barnes's loop upgrader script (D28874212) with some modification to exclude all files under /torch/jit and a number of reversions or unused variable suppression warnings added by hand.
Test Plan: Sandcastle
Reviewed By: malfet
Differential Revision: D32837991
fbshipit-source-id: fc7c4f76d2f32a17a0faf329294b3fe7cb81df32
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/66743
Modified loops in files under fbsource/fbcode/caffe2/ from the format
`for(TYPE var=x0;var<x_max;x++)`
to the format
`for(const auto var: irange(xmax))`
This was achieved by running r-barnes's loop upgrader script (D28874212) with some modification to exclude all files under /torch/jit and a number of reversions or unused variable suppression warnings added by hand.
Test Plan: Sandcastle
Reviewed By: malfet
Differential Revision: D31705359
fbshipit-source-id: c9ea2fbc0f9cd29e97a52dcb203addc5f2abb09b
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/66234
Modified loops in files under fbsource/fbcode/caffe2/ from the format
`for(TYPE var=x0;var<x_max;x++)`
to the format
`for(const auto var: irange(xmax))`
This was achieved by running r-barnes's loop upgrader script (D28874212) with some modification to exclude all files under /torch/jit and a number of reversions or unused variable suppression warnings added by hand.
bypass_size_limit
allow-large-files
Test Plan: Sandcastle
Reviewed By: ngimel
Differential Revision: D30652629
fbshipit-source-id: 0ae6c4bbbb554bad42e372792a6430e1acf15e3e
Summary:
As GoogleTest `TEST` macro is non-compliant with it as well as `DEFINE_DISPATCH`
All changes but the ones to `.clang-tidy` are generated using following script:
```
for i in `find . -type f -iname "*.c*" -or -iname "*.h"|xargs grep cppcoreguidelines-avoid-non-const-global-variables|cut -f1 -d:|sort|uniq`; do sed -i "/\/\/ NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)/d" $i; done
```
Pull Request resolved: https://github.com/pytorch/pytorch/pull/62008
Reviewed By: driazati, r-barnes
Differential Revision: D29838584
Pulled By: malfet
fbshipit-source-id: 1b2f8602c945bd4ce50a9bfdd204755556e31d13
Summary:
This is an automatic change generated by the following script:
```
#!/usr/bin/env python3
from subprocess import check_output, check_call
import os
def get_compiled_files_list():
import json
with open("build/compile_commands.json") as f:
data = json.load(f)
files = [os.path.relpath(node['file']) for node in data]
for idx, fname in enumerate(files):
if fname.startswith('build/') and fname.endswith('.DEFAULT.cpp'):
files[idx] = fname[len('build/'):-len('.DEFAULT.cpp')]
return files
def run_clang_tidy(fname):
check_call(["python3", "tools/clang_tidy.py", "-c", "build", "-x", fname,"-s"])
changes = check_output(["git", "ls-files", "-m"])
if len(changes) == 0:
return
check_call(["git", "commit","--all", "-m", f"NOLINT stubs for {fname}"])
def main():
git_files = check_output(["git", "ls-files"]).decode("ascii").split("\n")
compiled_files = get_compiled_files_list()
for idx, fname in enumerate(git_files):
if fname not in compiled_files:
continue
if fname.startswith("caffe2/contrib/aten/"):
continue
print(f"[{idx}/{len(git_files)}] Processing {fname}")
run_clang_tidy(fname)
if __name__ == "__main__":
main()
```
Pull Request resolved: https://github.com/pytorch/pytorch/pull/56892
Reviewed By: H-Huang
Differential Revision: D27991944
Pulled By: malfet
fbshipit-source-id: 5415e1eb2c1b34319a4f03024bfaa087007d7179
Summary:
*Context:* https://github.com/pytorch/pytorch/issues/53406 added a lint for trailing whitespace at the ends of lines. However, in order to pass FB-internal lints, that PR also had to normalize the trailing newlines in four of the files it touched. This PR adds an OSS lint to normalize trailing newlines.
The changes to the following files (made in 54847d0adb) are the only manually-written parts of this PR:
- `.github/workflows/lint.yml`
- `mypy-strict.ini`
- `tools/README.md`
- `tools/test/test_trailing_newlines.py`
- `tools/trailing_newlines.py`
I would have liked to make this just a shell one-liner like the other three similar lints, but nothing I could find quite fit the bill. Specifically, all the answers I tried from the following Stack Overflow questions were far too slow (at least a minute and a half to run on this entire repository):
- [How to detect file ends in newline?](https://stackoverflow.com/q/38746)
- [How do I find files that do not end with a newline/linefeed?](https://stackoverflow.com/q/4631068)
- [How to list all files in the Git index without newline at end of file](https://stackoverflow.com/q/27624800)
- [Linux - check if there is an empty line at the end of a file [duplicate]](https://stackoverflow.com/q/34943632)
- [git ensure newline at end of each file](https://stackoverflow.com/q/57770972)
To avoid giving false positives during the few days after this PR is merged, we should probably only merge it after https://github.com/pytorch/pytorch/issues/54967.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/54737
Test Plan:
Running the shell script from the "Ensure correct trailing newlines" step in the `quick-checks` job of `.github/workflows/lint.yml` should print no output and exit in a fraction of a second with a status of 0. That was not the case prior to this PR, as shown by this failing GHA workflow run on an earlier draft of this PR:
- https://github.com/pytorch/pytorch/runs/2197446987?check_suite_focus=true
In contrast, this run (after correcting the trailing newlines in this PR) succeeded:
- https://github.com/pytorch/pytorch/pull/54737/checks?check_run_id=2197553241
To unit-test `tools/trailing_newlines.py` itself (this is run as part of our "Test tools" GitHub Actions workflow):
```
python tools/test/test_trailing_newlines.py
```
Reviewed By: malfet
Differential Revision: D27409736
Pulled By: samestep
fbshipit-source-id: 46f565227046b39f68349bbd5633105b2d2e9b19
Summary:
Context: https://github.com/pytorch/pytorch/pull/53299#discussion_r587882857
These are the only hand-written parts of this diff:
- the addition to `.github/workflows/lint.yml`
- the file endings changed in these four files (to appease FB-internal land-blocking lints):
- `GLOSSARY.md`
- `aten/src/ATen/core/op_registration/README.md`
- `scripts/README.md`
- `torch/csrc/jit/codegen/fuser/README.md`
The rest was generated by running this command (on macOS):
```
git grep -I -l ' $' -- . ':(exclude)**/contrib/**' ':(exclude)third_party' | xargs gsed -i 's/ *$//'
```
I looked over the auto-generated changes and didn't see anything that looked problematic.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/53406
Test Plan:
This run (after adding the lint but before removing existing trailing spaces) failed:
- https://github.com/pytorch/pytorch/runs/2043032377
This run (on the tip of this PR) succeeded:
- https://github.com/pytorch/pytorch/runs/2043296348
Reviewed By: walterddr, seemethere
Differential Revision: D26856620
Pulled By: samestep
fbshipit-source-id: 3f0de7f7c2e4b0f1c089eac9b5085a58dd7e0d97
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/28523
New features:
1. Previously, `torch::tensor({true, false, true})` throws `"tensor_cpu" not implemented for 'Bool'`. After this PR, it produces the correct bool tensor, matching the Python API behavior.
2. Tensors with zero-size dimensions are now supported, e.g. `torch::tensor({{}, {}})` produces a tensor with sizes `{2, 0}`, matching the Python API behavior.
BC-breaking bug fixes:
1. Previously, `torch::tensor({{1}, {2}})` produces a tensor of sizes `{2}`. After this PR, it produces a tensor of sizes `{2, 1}`, matching the Python API behavior.
2. Fixed semantics of `torch::tensor(1.1)`: it now returns a 0-dim tensor instead of a 1-dim tensor, matching the Python API behavior.
3. Previously, when passed a non-dtype `TensorOptions` to the `torch::tensor` constructor, it always produces a tensor of dtype `float`. After this PR, it produces tensor of different dtypes based on the dtype of the braced-init-list, matching the behavior of the no-options case.
```cpp
// Previously:
torch::tensor({1, 2, 3}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> float
torch::tensor({{1, 2, 3}}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> float
torch::tensor({1., 2., 3.}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> float
torch::tensor({{1., 2., 3.}}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> float
// Now:
torch::tensor({1, 2, 3}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> int
torch::tensor({{1, 2, 3}}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> int
torch::tensor({1., 2., 3.}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> double
torch::tensor({{1., 2., 3.}}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> double
// As comparison, currently:
torch::tensor({1, 2, 3}).dtype() -> int
torch::tensor({{1, 2, 3}}).dtype() -> int
torch::tensor({1., 2., 3.}).dtype() -> double
torch::tensor({{1., 2., 3.}}).dtype() -> double
```
Notes:
1. From now on, the behavior of `at::tensor(scalar_value)` (which produces a 1-dim tensor) would be different from `torch::tensor(scalar_value)` (which produces a 0-dim tensor). I will fix the behavior of `at::tensor(scalar_value)` in a follow-up PR.
2. From now on, the behavior of `at::tensor({1, 2, 3}, torch::TensorOptions(/*non-dtype-options*/))` (which produces a `float` tensor) would be different from `torch::tensor({1, 2, 3}, torch::TensorOptions(/*non-dtype-options*/))` (which produces a an `int` tensor). I will fix this behavior of `at::tensor` constructor in a follow-up PR.
Context for the changes in this PR:
The motivation comes from fixing the "`torch::tensor({{1}, {2}})` gives tensor of wrong sizes" bug - in order to fix it, I have to move the handling of `at::ArrayRef` and `std::vector` into `InitListTensor` (see below on why we need to do this) and renamed `InitListTensor` to `TensorDataContainer`. After such changes, support for bool values comes out of the box without extra effort, and support for tensors with zero-size dimensions only requires adding a default constructor for `TensorDataContainer`, so I added those two in this PR.
For the semantic change of `torch::tensor(1.1)`, it's actually more effort to preserve the original wrong behavior (i.e. we need to check the sizes of the tensor converted from `TensorDataContainer` and reshape any scalar tensor to a 1-D tensor). I think preserving the original wrong behavior doesn't give us much value, and since the above changes naturally fix the problem, we should just start using the right behavior instead.
For the "constructor with non-dtype options behavior" fix, the code looks simpler and easier to reason about with the fix, so I included it in this PR.
--------
Why we need to move the handling of `at::ArrayRef` and `std::vector` into `TensorDataContainer`:
`torch::tensor({{1}, {2}})` can match this function overload:
`torch::tensor(at::ArrayRef<int> values)`, because `{1}` and `{2}` can be treated as
a list-initialization of an `int` value. However, this will produce a Tensor with sizes `{2}`,
but we actually want a Tensor with sizes `{2, 1}`. In order to avoid matching this function overload,
we removed the function overload and moved the ability to convert `at::ArrayRef<T>`
(and similarly `std::vector<T>`) into `TensorDataContainer`, and since for braced-init-list the
`TensorDataContainer(std::initializer_list<TensorDataContainer>)` constructor is always preferred over all other constructors, it will take the `std::initializer_list` path, and all is good.
Test Plan: Imported from OSS
Differential Revision: D18234625
Pulled By: yf225
fbshipit-source-id: 0f3f6912e82e2117d2103e31b74e7e97baaa8693
Summary:
One of the purposes of the C++ API tests in `test/cpp/api/` should be to check that including `torch/torch.h` is a sufficient prerequisite for using all C++ frontend features. This PR change ensures that.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/27067
Differential Revision: D17856815
Pulled By: yf225
fbshipit-source-id: 49c057bd807b003e4a00f6ba73131d763a0f277a
Summary:
Add a sorting policy to ChunkDataset.
This is considered an advanced parameter for developers who want to apply a 'sorting policy' to the chunk data before sampling into minibatch.
Different than the collate method, this policy is applied on the chunk level instead of minibatch level. When a chunk of data is loaded (multiple chunks if cross_chunk_shuffle_count_ is greater than 1), this policy is targeting to the full loaded data. It will be useful if developers want to perform some pre-processing (like bucketing) to the chunk data before example sampler samples the data.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/23053
Differential Revision: D16537692
Pulled By: colesbury
fbshipit-source-id: cd21ed40ab787a18b8c6dd304e5b806a7a45e6ba
Summary:
This change adds one advanced support for cross-chunk shuffling.
For training with static dataset, the default configuration is at user's disposal. However, in some user cases, over each epoch, new data is added to the current dataset, thus the dataset's size is dynamically changing/increasing. In order to mix the new data and the old data for better random sampling, one approach is to shuffle examples from more than 1 chunks. This feature is supported with this change. By specifying the `cross_chunk_shuffle_count_` on construction, advanced user can specify how many chunks to shuffle example from.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/22347
Differential Revision: D16081378
Pulled By: zhangguanheng66
fbshipit-source-id: fd001dfb9e66947839adecfb9893156fbbce80d0
Summary:
When dealing with large scale dataset, it is handy if we can save the dataset status and resume later. Especially in cases where some unexpected crash happens, user don't need to start over the whole dataset from begining. Instead, they can reload it from the last checkpoint.
This change adds support for checkpoint save/load logic in ChunkDataset.
On ChunkDataset construction, user can specify a file name from which to load the checkpoint. If it is empty, default to start from fresh; otherwise the ChunkDataset will 'fast forward' the chunk sampler to the corresponding checkpoint.
The user can also call ChunkDataset::save() to serialize current status to a file, which can be used later.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/21889
Differential Revision: D16024582
Pulled By: ailzhang
fbshipit-source-id: 1862ab5116f94c9d29da174ce04a91041d06cad5
Summary:
This PR restricts the BatchType template argument of ChunkDataReader to STL
vectors only. Internally, ChunkDataReader was assuming BatchType was a
vector, but the user could pass any type to the template argument,
leading to compiling issues during CPP extensions.
Additionally to the proposed API change, this PR adds missing include
headers to chunk.h. Currently the current implementation works but if
users try to create C++ extensions that implements new ChunkDataReaders
to be along with the existing ChunkDataset, the build will fail due to
the missing headers.
In terms of functionality, nothing has changed. This PR simply makes the
implementation slightly more robust for future extensions.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/19485
Differential Revision: D15261725
Pulled By: soumith
fbshipit-source-id: 38c9465d665392ae6a2d12c5a520a4f501e1a6ca
Summary:
The chunk buffer had a possibility to hang when no data is read and the buffer size is lower than chunk size. We detected this while running with larger dataset and hence the fix. I added a test to mimic the situation and validated that the fix is working. Thank you Xueyun for finding this issue.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/17409
Differential Revision: D14198546
Pulled By: soumith
fbshipit-source-id: b8ca43b0400deaae2ebb6601fdc65b47f32b0554
Summary:
Adding two distrbuted samplers, Random and Sequential to the mix. Similar to python counterpart, DistributedSampler introduces a new method `set_epoch(size_t epoch)` which can be use to shuffle data determinstically between distributed processes.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/16910
Differential Revision: D14130980
Pulled By: soumith
fbshipit-source-id: ec08b7130c01e2fc6dc3693f7ac622a0a6d60f10
Summary:
Previously, the ChunkBuffer depends on the remaining chunk count to signal end of dataloading. This does not work with distributed samplers where each sampler only loads a subset of chunks. This refactor remove the dependency on the remaining chunk count at the ChunkBuffer.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/16868
Differential Revision: D14066517
Pulled By: goldsborough
fbshipit-source-id: 293dfe282ceff326dff0876c2f75c2ee4f4463e2
Summary:
This PR contains the implementation of chunk dataset, with the API proposed in PR https://github.com/pytorch/pytorch/pull/15562
A chunk dataset is derived from StatefulDataset. It utilizes worker threads to prefetches chunk data, splits it into batches and caches them into a queue. When get_batch is called from dataloader, batch data is retrieved from the queue, and data in new chunks will be pushed for later following batches.
Chunk dataset uses two samplers (chunk_sampler and example_sampler) to perform sampling. The chunk_sampler decides which chunk to load, and example_sampler shuffles the examples inside a specific chunk. More detail of this sampling approach can be found here: http://martin.zinkevich.org/publications/nips2010.pdf
Pull Request resolved: https://github.com/pytorch/pytorch/pull/15932
Differential Revision: D13868688
Pulled By: soumith
fbshipit-source-id: a43000c478ca2a3c64cc84b3626d6b8b1ad9a07e
Summary:
Currently re-implements the dataloader for stateful datasets. Outstanding work:
- Refactor DataLoader and DataLoader2 to have common base classes and only differ in specifi pieces of logic,
- Figure out how to not duplicate the `MapDataset` logic for stateful vs. non-stateful
Pull Request resolved: https://github.com/pytorch/pytorch/pull/15096
Differential Revision: D13522043
Pulled By: goldsborough
fbshipit-source-id: 08e461ca51783047f11facc4d27dfa2e4f1e4c2a
Summary:
Make Samplers optionally accept new size in their reset() method. This helps dataloader or dataset to reset the sampler for an epoch or a chunk of data with different sizes.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/13870
Differential Revision: D13240120
Pulled By: soumith
fbshipit-source-id: 19c53f8be13c0fdcf504f0637b0d3e6009a8e599
Summary:
I noticed the test `DataLoaderTest.CanDereferenceIteratorMultipleTimes` doesn't test proper progression of the iterator. I also added a test for using `std::copy`.
Fixes https://github.com/pytorch/pytorch/issues/14276
ebetica ezyang apaszke
Pull Request resolved: https://github.com/pytorch/pytorch/pull/14045
Differential Revision: D13092187
Pulled By: goldsborough
fbshipit-source-id: 57698ec00fa7b914b159677a4ab38b6b25c2860b
Summary:
This PR adds a `SharedDataset` to the C++ frontend data API, which allows wrapping a shared_ptr to a dataset into a class that conforms to the `Dataset` interface (with `get_batch`). This enables use cases where a custom dataset is (1) thread-safe and (2) expensive to copy. All workers will reference a single instance of this dataset. No additional copies are incurred.
jaliyae apaszke
Pull Request resolved: https://github.com/pytorch/pytorch/pull/13800
Differential Revision: D13075610
Pulled By: goldsborough
fbshipit-source-id: 4ffdfd7959d49b042c0e254110085f62a0bfeb6c
Summary:
I think this will be it. So for one, the previous test was bullshit because it was returning the thread id instead of the sample index (which is the thing whose ordering is enforced). Just turning up the number of threads to 10 from 4 made this very obvious. I also think there is a race condition, which may or may not have surfaced, in that there was nothing stopping one worker to get multiple batches, which would screw with the whole ordering logic. I've added a barrier struct such that workers wait for all workers to be in the `get_batch` function before actually doing something.
Fixes https://github.com/pytorch/pytorch/issues/14002
ezyang
Pull Request resolved: https://github.com/pytorch/pytorch/pull/14038
Differential Revision: D13088132
Pulled By: goldsborough
fbshipit-source-id: 4bded63756c6a49502ee07ef8709a03073e7e05f
Summary:
Attempts to unflake the dataloader ordering enforcement test. I think the issue was that the `thread_counter` variable was not atomic. I've made it atomic, and also global just to make it a bit clearer.
Fixes https://github.com/pytorch/pytorch/issues/13634
colesbury SsnL ezyang
Pull Request resolved: https://github.com/pytorch/pytorch/pull/13919
Differential Revision: D13051718
Pulled By: goldsborough
fbshipit-source-id: b9f7f6317701a8b861a1d5c6a9b2b17b44782561
Summary:
In TorchScript and C++ extensions we currently advocate a mix of `torch::` and `at::` namespace usage. In the C++ frontend I had instead exported all symbols from `at::` and some from `c10::` into the `torch::` namespace. This is far, far easier for users to understand, and also avoid bugs around creating tensors vs. variables. The same should from now on be true for the TorchScript C++ API (for running and loading models) and all C++ extensions.
Note that since we're just talking about typedefs, this change does not break any existing code.
Once this lands I will update stuff in `pytorch/tutorials` too.
zdevito ezyang gchanan
Pull Request resolved: https://github.com/pytorch/pytorch/pull/13523
Differential Revision: D12942787
Pulled By: goldsborough
fbshipit-source-id: 76058936bd8707b33d9e5bbc2d0705fc3d820763
Summary:
This PR brings to changes to the recently landed C++ Frontend dataloader:
1. Removes the `size()` method from `BatchDataset`. This makes it cleaner to implement unsized ("infinite stream") datasets. The method was not used much beyond initial configuration.
2. Makes the index type of a dataset a template parameter of `BatchDataset` and `Sampler`. This essentially allows custom index types instead of only `vector<size_t>`. This greatly improves flexibility.
See the `InfiniteStreamDataset` and `TestIndex` datasets in the tests for what this enables.
Some additional minor updates and code movements too.
apaszke SsnL
Pull Request resolved: https://github.com/pytorch/pytorch/pull/12960
Differential Revision: D12893342
Pulled By: goldsborough
fbshipit-source-id: ef03ea0f11a93319e81fba7d52a0ef1a125d3108
Summary:
Implements serialization and deserialization for samplers in the C++ frontend dataloader.
apaszke
Pull Request resolved: https://github.com/pytorch/pytorch/pull/12999
Differential Revision: D10859676
Pulled By: goldsborough
fbshipit-source-id: cd132100fd35323e5a3df33e314511750806f48d
albanD