Files
pytorch/test/cpp/c10d/FileStoreTest.cpp
albanD f6a98bbf71 [RAWTHROW][6] test/cpp and test/cpp_extensions: 43 conversions, 5 that must not be (#194773)
## 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
2026-08-31 23:48:04 +00:00

160 lines
4.4 KiB
C++

#include <c10/util/irange.h>
#include "StoreTestCommon.hpp"
#ifndef _WIN32
#include <unistd.h>
#endif
#include <iostream>
#include <thread>
#include <gtest/gtest.h>
#include <torch/csrc/distributed/c10d/FileStore.hpp>
#include <torch/csrc/distributed/c10d/PrefixStore.hpp>
#ifdef _WIN32
std::string tmppath() {
return c10d::test::autoGenerateTmpFilePath();
}
#else
std::string tmppath() {
const char* tmpdir = getenv("TMPDIR");
if (tmpdir == nullptr) {
tmpdir = "/tmp";
}
// Create template
std::vector<char> tmp(256);
auto len = snprintf(tmp.data(), tmp.size(), "%s/testXXXXXX", tmpdir);
tmp.resize(len);
// Create temporary file
auto fd = mkstemp(&tmp[0]);
if (fd == -1) {
// Carries errno as a std::error_code, which TORCH_CHECK cannot express.
// @allow-raw-throw: std::system_error carrying errno
throw std::system_error(errno, std::system_category());
}
close(fd);
return std::string(tmp.data(), tmp.size());
}
#endif
void testGetSet(const std::string& path, const std::string& prefix = "") {
// Basic Set/Get on File Store
{
auto fileStore = c10::make_intrusive<c10d::FileStore>(path, 2);
c10d::PrefixStore store(prefix, fileStore);
c10d::test::set(store, "key0", "value0");
c10d::test::set(store, "key1", "value1");
c10d::test::set(store, "key2", "value2");
c10d::test::check(store, "key0", "value0");
c10d::test::check(store, "key1", "value1");
c10d::test::check(store, "key2", "value2");
auto numKeys = fileStore->getNumKeys();
EXPECT_EQ(numKeys, 4);
// Check compareSet, does not check return value
c10d::test::compareSet(store, "key0", "wrongExpectedValue", "newValue");
c10d::test::check(store, "key0", "value0");
c10d::test::compareSet(store, "key0", "value0", "newValue");
c10d::test::check(store, "key0", "newValue");
// Check deleteKey
c10d::test::deleteKey(store, "key1");
numKeys = fileStore->getNumKeys();
EXPECT_EQ(numKeys, 3);
c10d::test::check(store, "key0", "newValue");
c10d::test::check(store, "key2", "value2");
c10d::test::set(store, "-key0", "value-");
c10d::test::check(store, "key0", "newValue");
c10d::test::check(store, "-key0", "value-");
numKeys = fileStore->getNumKeys();
EXPECT_EQ(numKeys, 4);
c10d::test::deleteKey(store, "-key0");
numKeys = fileStore->getNumKeys();
EXPECT_EQ(numKeys, 3);
c10d::test::check(store, "key0", "newValue");
c10d::test::check(store, "key2", "value2");
}
// Perform get on new instance
{
auto fileStore = c10::make_intrusive<c10d::FileStore>(path, 2);
c10d::PrefixStore store(prefix, fileStore);
c10d::test::check(store, "key0", "newValue");
auto numKeys = fileStore->getNumKeys();
// There will be 4 keys since we still use the same underlying file as the
// other store above.
EXPECT_EQ(numKeys, 4);
}
}
void stressTestStore(std::string path, std::string prefix = "") {
// Hammer on FileStore::add
const auto numThreads = 4;
const auto numIterations = 100;
std::vector<std::thread> threads;
c10d::test::Semaphore sem1, sem2;
for ([[maybe_unused]] const auto i : c10::irange(numThreads)) {
threads.emplace_back([&] {
auto fileStore =
c10::make_intrusive<c10d::FileStore>(path, numThreads + 1);
c10d::PrefixStore store(prefix, fileStore);
sem1.post();
sem2.wait();
for ([[maybe_unused]] const auto j : c10::irange(numIterations)) {
store.add("counter", 1);
}
});
}
sem1.wait(numThreads);
sem2.post(numThreads);
for (auto& thread : threads) {
thread.join();
}
// Check that the counter has the expected value
{
auto fileStore = c10::make_intrusive<c10d::FileStore>(path, numThreads + 1);
c10d::PrefixStore store(prefix, fileStore);
std::string expected = std::to_string(numThreads * numIterations);
c10d::test::check(store, "counter", expected);
}
}
class FileStoreTest : public ::testing::Test {
protected:
void SetUp() override {
path_ = tmppath();
}
void TearDown() override {
unlink(path_.c_str());
}
std::string path_;
};
TEST_F(FileStoreTest, testGetAndSet) {
testGetSet(path_);
}
TEST_F(FileStoreTest, testGetAndSetWithPrefix) {
testGetSet(path_, "testPrefix");
}
TEST_F(FileStoreTest, testStressStore) {
stressTestStore(path_);
}
TEST_F(FileStoreTest, testStressStoreWithPrefix) {
stressTestStore(path_, "testPrefix");
}