mirror of
https://github.com/pytorch/pytorch.git
synced 2026-10-28 18:56:51 +08:00
## 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
160 lines
4.4 KiB
C++
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");
|
|
}
|