## 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
### Changes
- Move ProcessGroup::Work into its own class and update all the references to it / header includes.
#### Motivation
In the future PRs we will repurpose ProcessGroup to instead contain a list of Backends (ProcessGroupNCCL/Gloo/UCC) and perform dispatching to them based on tensor type. This change is prevent a circular dependency with ProcessGroup depending on Backend and Backend depending on ProcessGroup::Work.
Differential Revision: [D38839212](https://our.internmc.facebook.com/intern/diff/D38839212)
Pull Request resolved: https://github.com/pytorch/pytorch/pull/83680
Approved by: https://github.com/kwen2501
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/45181
`init_process_group` and `new_group` update a bunch of global
variables after initializing the actual process group. As a result, there is a
race that after initializing the process group on say rank 0, if we immediately
check the default process group on rank 1 (say via RPC), we might actually get
an error since rank 1 hasn't yet updated its _default_pg variable.
To resolve this issue, I've added barrier() at the end of both of these calls.
This ensures that once these calls return we are guaranteed about correct
initialization on all ranks.
Since these calls are usually done mostly during initialization, it should be
fine to add the overhead of a barrier() here.
#Closes: https://github.com/pytorch/pytorch/issues/40434, https://github.com/pytorch/pytorch/issues/40378
ghstack-source-id: 112923112
Test Plan:
Reproduced the failures in
https://github.com/pytorch/pytorch/issues/40434 and
https://github.com/pytorch/pytorch/issues/40378 and verified that this PR fixes
the issue.
Reviewed By: mrshenli
Differential Revision: D23858025
fbshipit-source-id: c4d5e46c2157981caf3ba1525dec5310dcbc1830
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/40944
This stack adds Work-level timeout for blocking wait.
This PR just changes the API to accept a default wait arg for the wait function in each ProcessGroup backend. The ProcessGroup superclass correctly waits for the given timeout by changing the CV wait to wait_for.
Closes: https://github.com/pytorch/pytorch/issues/37571
ghstack-source-id: 107835735
Test Plan: Tests in 4th PR in this stack
Reviewed By: jiayisuse
Differential Revision: D22107135
fbshipit-source-id: b38c07cb5e79e6c86c205e580336e7918ed96501
Summary:
The original behavior of pytorch c10d only supports built-in c10d backends, such as
nccl/gloo/mpi. This patch is used to extend the c10d capability to support dynamically
loading 3rd party communication libraries which are derived from ProcessGroup base class.
related RFC is in: https://github.com/pytorch/pytorch/issues/27955
Through this way, user just need specify a 3rd party c10d backend name when invoking
torch.distributed.init_process_group(). The proposed logic will try to load corresponding
c10d backend cpp extension automatically. as for how to develop a new 3rd party c10d backend
through cpp extension, pls refer to test/cpp_extensions/cpp_c10d_extension.cpp
Pull Request resolved: https://github.com/pytorch/pytorch/pull/28068
Differential Revision: D19174838
Pulled By: agolynski
fbshipit-source-id: 3409a504a43ce7260e6f9d1207c00e87471fac62
albanD