Skip to content

Commit

Permalink
Only error out when the number of kernels being registered exceeds th…
Browse files Browse the repository at this point in the history
…e limit (pytorch#1487)

Summary:
Pull Request resolved: pytorch#1487

As titled. In some of the CI failures such as this one: https://github.com/pytorch/executorch/actions/runs/7331918023/job/19965267429?pr=1485#step:11:1520

The error message is confusing:
```
The total number of kernels to be registered is larger than the limit 17. 15 kernels are already registered and we're trying to register another 2 kernels.
```

Here I'm changing the ">=" condition to be ">" so that the error message makes sense.

Reviewed By: lucylq

Differential Revision: D52422427

fbshipit-source-id: 645d38bb1b52f27e525016984552a29f06e54ac0
  • Loading branch information
larryliu0820 authored and facebook-github-bot committed Dec 27, 2023
1 parent 7464600 commit ff28629
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 7 deletions.
2 changes: 1 addition & 1 deletion runtime/kernel/operator_registry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ Error OperatorRegistry::register_kernels(const ArrayRef<Kernel>& kernels) {
// have any side effect even if falled multiple times.
::et_pal_init();

if (kernels.size() + this->num_kernels_ >= kMaxNumOfKernels) {
if (kernels.size() + this->num_kernels_ > kMaxNumOfKernels) {
ET_LOG(
Error,
"The total number of kernels to be registered is larger than the limit %" PRIu32
Expand Down
4 changes: 2 additions & 2 deletions runtime/kernel/targets.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def define_common_targets():
)

runtime.cxx_library(
name = "operator_registry_TWO_KERNELS_TEST_ONLY",
name = "operator_registry_MAX_NUM_KERNELS_TEST_ONLY",
srcs = ["operator_registry.cpp"],
exported_headers = ["operator_registry.h"],
visibility = [
Expand All @@ -35,7 +35,7 @@ def define_common_targets():
"//executorch/runtime/core:core",
"//executorch/runtime/core:evalue",
],
preprocessor_flags = ["-DMAX_KERNEL_NUM=2"],
preprocessor_flags = ["-DMAX_KERNEL_NUM=1"],
)

for aten_mode in (True, False):
Expand Down
8 changes: 5 additions & 3 deletions runtime/kernel/test/operator_registry_max_kernel_num_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class OperatorRegistryMaxKernelNumTest : public ::testing::Test {
}
};

// Register one kernel when max_kernel_num=2; success
// Register one kernel when max_kernel_num=1; success
TEST_F(OperatorRegistryMaxKernelNumTest, RegisterOneOp) {
Kernel kernels[] = {Kernel("foo", [](RuntimeContext&, EValue**) {})};
ArrayRef<Kernel> kernels_array = ArrayRef<Kernel>(kernels);
Expand All @@ -37,13 +37,15 @@ TEST_F(OperatorRegistryMaxKernelNumTest, RegisterOneOp) {
EXPECT_TRUE(hasOpsFn("foo"));
}

// Register two kernels when max_kernel_num=2; fail
// Register two kernels when max_kernel_num=1; fail
TEST_F(OperatorRegistryMaxKernelNumTest, RegisterTwoOpsFail) {
Kernel kernels[] = {
Kernel("foo1", [](RuntimeContext&, EValue**) {}),
Kernel("foo2", [](RuntimeContext&, EValue**) {})};
ArrayRef<Kernel> kernels_array = ArrayRef<Kernel>(kernels);
ET_EXPECT_DEATH({ register_kernels(kernels_array); }, "");
ET_EXPECT_DEATH(
{ register_kernels(kernels_array); },
"The total number of kernels to be registered is larger than the limit 1");
}

} // namespace executor
Expand Down
2 changes: 1 addition & 1 deletion runtime/kernel/test/targets.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def define_common_targets():
"operator_registry_max_kernel_num_test.cpp",
],
deps = [
"//executorch/runtime/kernel:operator_registry_TWO_KERNELS_TEST_ONLY",
"//executorch/runtime/kernel:operator_registry_MAX_NUM_KERNELS_TEST_ONLY",
"//executorch/runtime/kernel:kernel_runtime_context",
],
)
Expand Down

0 comments on commit ff28629

Please sign in to comment.