Skip to content

Commit bd1e461

Browse files
authored
Return an error instead of aborting when device memory cannot be allocated (pytorch#22085)
### What is going on A model can be exported so that some of its memory-planned buffers live on an accelerator instead of on the CPU. When `Module` loads such a model, it asks the runtime for a block of memory on that device. That request can fail for two ordinary reasons that say nothing about the model file being loaded: 1. this build has no allocator registered for that device type, or 2. the device is out of memory. Both were checked with `ET_CHECK_MSG`, which terminates the whole process. So loading a device-annotated model on a machine without the matching backend, or on a busy GPU, killed the process outright. Through the Python bindings (`_load_for_executorch_from_buffer` and friends build one of these `Module` objects) that shows up as an abort and a core dump: no traceback, no error to catch, no chance to fall back to the CPU. Neither condition means the program is broken. They describe the machine it is running on, so `load_method` now returns the error to the caller and the process stays alive. Before this change, both reasons ended the same way: ``` module.load_method("forward"); // never returns, process aborts ``` After, the two reasons stay apart, because `DeviceMemoryBuffer::create` already reports them with different codes: ``` // nothing registered for that device type module.load_method("forward"); // returns Error::NotFound // allocator is there, the device has no room module.load_method("forward"); // returns Error::MemoryAllocationFailed ``` ### Also in this change Two other `MethodMeta` lookups in the same function, one for the buffer size and one for the buffer device, also used `ET_CHECK_MSG`. They return the error now as well, so all three abort sites in that function are gone. Separately, the loop that decides whether a model uses device buffers at all threw away a failed device query, which then read as "this buffer is on the CPU". It returns the error now instead of ignoring it. That last path is not reachable today. The only case the query rejects is an out-of-range buffer index, and the loop bounds already keep the index in range. It is fixed for consistency, not because it can fire. Quietly treating a failed query as CPU would hand a backend host memory, which is a far harder failure to debug than an error return. ### Test coverage `extension/module/test/module_device_memory_test.cpp` was registered in the internal build only, so no open source job ran any of it. This change adds it to `extension/module/test/CMakeLists.txt`, along with the model file it needs, so it now builds and runs as part of `extension_module_test`. ### Test plan * New test `ModuleDeviceMemoryTest.DeviceAllocationFailureIsReportedNotFatal`. It makes the test allocator refuse the request, then checks that `load_method` returns `MemoryAllocationFailed`, that the method is not left half loaded, and that a later attempt with the device healthy allocates normally. * Confirmed the test actually catches the bug. With this fix reverted, the test binary exits with signal 6 (abort) and prints no result at all. With the fix, all 6 tests in the suite pass. * Ran the whole `extension_module_test` binary: 57 tests, all pass. The 51 pre-existing tests are unaffected by adding the device test file to the same binary. * Ran `extension_module_test --gtest_repeat=3`: 57 tests pass on every iteration, exit 0. The suite registers its allocator only once, so process reuse is safe. Put the unconditional registration back and the same command aborts with exit 134. * `clang-format` reports no changes needed on the touched source file. `cmake-format` 0.6.13 and `cmake-lint` report no changes needed on the touched `CMakeLists.txt`. ### Not covered Nothing here runs against real accelerator hardware. The failure is driven through the existing mock allocator, which stands in for a device that is out of memory. The committed test therefore covers the `Error::MemoryAllocationFailed` branch only. The `Error::NotFound` branch, for a device type with no registered allocator, is not exercised by a committed test.
1 parent 368a849 commit bd1e461

5 files changed

Lines changed: 68 additions & 12 deletions

File tree

‎extension/module/module.cpp‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,8 @@ Module::make_planned_memory_with_shared_arenas(
368368
return planned;
369369
}
370370

371-
std::unique_ptr<Module::PlannedMemory> Module::make_planned_memory_with_devices(
371+
runtime::Result<std::unique_ptr<Module::PlannedMemory>>
372+
Module::make_planned_memory_with_devices(
372373
const ET_RUNTIME_NAMESPACE::MethodMeta& method_meta) {
373374
auto planned = std::make_unique<PlannedMemory>();
374375
const size_t num_buffers = method_meta.num_memory_planned_buffers();
@@ -379,9 +380,11 @@ std::unique_ptr<Module::PlannedMemory> Module::make_planned_memory_with_devices(
379380

380381
for (size_t i = 0; i < num_buffers; ++i) {
381382
auto size = method_meta.memory_planned_buffer_size(i);
382-
ET_CHECK_MSG(size.ok(), "Failed to get buffer size for index %zu", i);
383+
ET_CHECK_OK_OR_RETURN_ERROR(
384+
size.error(), "Failed to get buffer size for index %zu", i);
383385
auto device = method_meta.memory_planned_buffer_device(i);
384-
ET_CHECK_MSG(device.ok(), "Failed to get buffer device for index %zu", i);
386+
ET_CHECK_OK_OR_RETURN_ERROR(
387+
device.error(), "Failed to get buffer device for index %zu", i);
385388
planned->planned_devices.push_back(device.get());
386389

387390
if (device->is_cpu()) {
@@ -391,13 +394,17 @@ std::unique_ptr<Module::PlannedMemory> Module::make_planned_memory_with_devices(
391394
} else {
392395
// Allocate device memory via DeviceAllocator and store the RAII buffer.
393396
planned->planned_buffers.emplace_back(); // empty CPU placeholder
397+
// Whether a device allocator exists, and whether the device has room,
398+
// are properties of the machine rather than of the program, so report
399+
// them to the caller instead of aborting the process.
394400
auto dmb = runtime::DeviceMemoryBuffer::create(
395401
size.get(), device->type(), device->index());
396-
ET_CHECK_MSG(
397-
dmb.ok(),
398-
"Failed to allocate device memory for buffer %zu (device_type=%d)",
402+
ET_CHECK_OK_OR_RETURN_ERROR(
403+
dmb.error(),
404+
"Failed to allocate device memory for buffer %zu (device_type=%d, device_index=%d)",
399405
i,
400-
static_cast<int>(device->type()));
406+
static_cast<int>(device->type()),
407+
static_cast<int>(device->index()));
401408
planned->planned_spans.emplace_back(dmb->as_span());
402409
planned->device_buffers.push_back(std::move(dmb.get()));
403410
}
@@ -473,10 +480,16 @@ runtime::Error Module::load_method(
473480
ET_CHECK_OK_OR_RETURN_ERROR(meta_res.error());
474481
auto& meta = meta_res.get();
475482

483+
// A failed device query must not read as "this buffer is on the host",
484+
// which would silently hand a backend host memory. The loop bounds i by
485+
// num_memory_planned_buffers(), so the callee's range check cannot fire
486+
// today; this keeps the failure handled if that ever stops holding.
476487
bool has_device_buffers = false;
477488
for (size_t i = 0; i < meta.num_memory_planned_buffers(); ++i) {
478489
auto dev = meta.memory_planned_buffer_device(i);
479-
if (dev.ok() && !dev->is_cpu()) {
490+
ET_CHECK_OK_OR_RETURN_ERROR(
491+
dev.error(), "Failed to get buffer device for index %zu", i);
492+
if (!dev->is_cpu()) {
480493
has_device_buffers = true;
481494
break;
482495
}
@@ -493,7 +506,9 @@ runtime::Error Module::load_method(
493506

494507
// Device-aware path: allocate CPU and device buffers. The device
495508
// span is owned by the HierarchicalAllocator inside PlannedMemory.
496-
method_holder.planned_memory = make_planned_memory_with_devices(meta);
509+
auto planned_res = make_planned_memory_with_devices(meta);
510+
ET_CHECK_OK_OR_RETURN_ERROR(planned_res.error());
511+
method_holder.planned_memory = std::move(planned_res.get());
497512
planned_memory = method_holder.planned_memory->planned_memory.get();
498513
} else if (!share_memory_arenas_) {
499514
auto sizes_res = get_mem_planned_buffer_sizes(method_name);

‎extension/module/module.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -730,7 +730,8 @@ class Module {
730730
std::unique_ptr<PlannedMemory> make_planned_memory_with_shared_arenas(
731731
const std::vector<size_t>& buffer_sizes,
732732
std::vector<std::vector<uint8_t>>& shared_arenas);
733-
std::unique_ptr<PlannedMemory> make_planned_memory_with_devices(
733+
runtime::Result<std::unique_ptr<PlannedMemory>>
734+
make_planned_memory_with_devices(
734735
const ET_RUNTIME_NAMESPACE::MethodMeta& method_meta);
735736
runtime::Result<std::vector<size_t>> get_mem_planned_buffer_sizes(
736737
const std::string& method_name);

‎extension/module/test/CMakeLists.txt‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ set(EXECUTORCH_ROOT ${CMAKE_CURRENT_SOURCE_DIR}/../../..)
1717

1818
include(${EXECUTORCH_ROOT}/tools/cmake/Test.cmake)
1919

20-
set(_test_srcs module_test.cpp)
20+
set(_test_srcs module_test.cpp module_device_memory_test.cpp)
2121

2222
add_custom_command(
2323
OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/ModuleAdd.pte"
@@ -26,12 +26,15 @@ add_custom_command(
2626
"${CMAKE_CURRENT_BINARY_DIR}/ModuleLinearProgram.pte"
2727
"${CMAKE_CURRENT_BINARY_DIR}/ModuleLinearProgram.ptd"
2828
"${CMAKE_CURRENT_BINARY_DIR}/ModuleSharedState.pte"
29+
"${CMAKE_CURRENT_BINARY_DIR}/ModuleAddWithDevice.pte"
2930
COMMAND ${PYTHON_EXECUTABLE} -m test.models.export_program --modules
3031
"ModuleAdd,ModuleSharedState" --outdir "${CMAKE_CURRENT_BINARY_DIR}"
3132
COMMAND
3233
${PYTHON_EXECUTABLE} -m test.models.export_program --modules
3334
"ModuleAddMul,ModuleLinear" --external-constants --outdir
3435
"${CMAKE_CURRENT_BINARY_DIR}"
36+
COMMAND ${PYTHON_EXECUTABLE} -m test.models.export_program_with_device_info
37+
--outdir "${CMAKE_CURRENT_BINARY_DIR}"
3538
WORKING_DIRECTORY ${EXECUTORCH_ROOT}
3639
)
3740

@@ -43,6 +46,7 @@ add_custom_target(
4346
"${CMAKE_CURRENT_BINARY_DIR}/ModuleLinearProgram.pte"
4447
"${CMAKE_CURRENT_BINARY_DIR}/ModuleLinearProgram.ptd"
4548
"${CMAKE_CURRENT_BINARY_DIR}/ModuleSharedState.pte"
49+
"${CMAKE_CURRENT_BINARY_DIR}/ModuleAddWithDevice.pte"
4650
)
4751

4852
set(test_env
@@ -52,6 +56,7 @@ set(test_env
5256
"ET_MODULE_LINEAR_PROGRAM_PATH=${CMAKE_CURRENT_BINARY_DIR}/ModuleLinearProgram.pte"
5357
"ET_MODULE_LINEAR_DATA_PATH=${CMAKE_CURRENT_BINARY_DIR}/ModuleLinearProgram.ptd"
5458
"ET_MODULE_SHARED_STATE=${CMAKE_CURRENT_BINARY_DIR}/ModuleSharedState.pte"
59+
"ET_MODULE_ADD_WITH_DEVICE_PATH=${CMAKE_CURRENT_BINARY_DIR}/ModuleAddWithDevice.pte"
5560
)
5661

5762
et_cxx_test(

‎extension/module/test/module_device_memory_test.cpp‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
using executorch::extension::Module;
3131
using executorch::runtime::DeviceMemoryBuffer;
3232
using executorch::runtime::Error;
33+
using executorch::runtime::get_device_allocator;
3334
using executorch::runtime::register_device_allocator;
3435
using executorch::runtime::etensor::DeviceType;
3536
using executorch::runtime::testing::MockCudaAllocator;
@@ -40,10 +41,15 @@ class ModuleDeviceMemoryTest : public ::testing::Test {
4041
protected:
4142
static void SetUpTestSuite() {
4243
executorch::runtime::runtime_init();
43-
register_device_allocator(&g_mock_cuda);
44+
// The registry is a process-wide static, so a second registration for the
45+
// same device type aborts. Repeat runs re-enter this function.
46+
if (get_device_allocator(DeviceType::CUDA) == nullptr) {
47+
register_device_allocator(&g_mock_cuda);
48+
}
4449
}
4550

4651
void SetUp() override {
52+
g_mock_cuda.fail_allocations_ = false;
4753
g_mock_cuda.allocate_count_ = 0;
4854
g_mock_cuda.deallocate_count_ = 0;
4955
g_mock_cuda.last_allocate_size_ = 0;
@@ -146,6 +152,27 @@ TEST_F(ModuleDeviceMemoryTest, DeviceModelWithSharedArenasReturnsNotSupported) {
146152
EXPECT_EQ(err, Error::NotSupported);
147153
}
148154

155+
TEST_F(ModuleDeviceMemoryTest, DeviceAllocationFailureIsReportedNotFatal) {
156+
const char* path = std::getenv("ET_MODULE_ADD_WITH_DEVICE_PATH");
157+
ASSERT_NE(path, nullptr) << "ET_MODULE_ADD_WITH_DEVICE_PATH not set";
158+
159+
// Stand in for a device that is out of memory. The caller gets an error and
160+
// the process survives to handle it.
161+
g_mock_cuda.fail_allocations_ = true;
162+
163+
Module module(path);
164+
EXPECT_EQ(module.load_method("forward"), Error::MemoryAllocationFailed);
165+
EXPECT_FALSE(module.is_method_loaded("forward"));
166+
EXPECT_EQ(g_mock_cuda.allocate_count_, 0);
167+
EXPECT_EQ(g_mock_cuda.deallocate_count_, 0);
168+
169+
// The failure must not leave the Module half-built: with the device back,
170+
// the same call reaches the allocator again.
171+
g_mock_cuda.fail_allocations_ = false;
172+
(void)module.load_method("forward");
173+
EXPECT_EQ(g_mock_cuda.allocate_count_, 1);
174+
}
175+
149176
TEST_F(
150177
ModuleDeviceMemoryTest,
151178
LoadMethodAllocatesDeviceMemoryAndDeallocatesOnDestroy) {

‎runtime/core/test/mock_cuda_allocator.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ class MockCudaAllocator : public DeviceAllocator {
3434
// malloc returns memory aligned to alignof(max_align_t), which satisfies
3535
// kDefaultAlignment; the mock only exercises the default alignment.
3636
(void)alignment;
37+
if (fail_allocations_) {
38+
return Error::MemoryAllocationFailed;
39+
}
3740
void* ptr = std::malloc(nbytes);
3841
if (!ptr) {
3942
return Error::MemoryAllocationFailed;
@@ -98,6 +101,7 @@ class MockCudaAllocator : public DeviceAllocator {
98101
}
99102

100103
void reset() {
104+
fail_allocations_ = false;
101105
allocate_count_ = 0;
102106
deallocate_count_ = 0;
103107
h2d_count_ = 0;
@@ -117,6 +121,10 @@ class MockCudaAllocator : public DeviceAllocator {
117121
last_d2h_index_ = -1;
118122
}
119123

124+
/// When set, allocate() reports MemoryAllocationFailed without allocating,
125+
/// so callers can be tested against a device that is out of memory.
126+
bool fail_allocations_ = false;
127+
120128
// Allocation tracking
121129
int allocate_count_ = 0;
122130
int deallocate_count_ = 0;

0 commit comments

Comments
 (0)