Android: unified error reporting — all sync errors throw, no silent f…#18669
Android: unified error reporting — all sync errors throw, no silent f…#18669psiddh wants to merge 5 commits intopytorch:mainfrom
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18669
Note: Links to docs will display an error until the docs builds have been completed. ❌ 4 New Failures, 2 Unrelated FailuresAs of commit 795e56c with merge base 60d57e5 ( NEW FAILURES - The following jobs have failed:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
…ailures Replace the mixed error-reporting pattern (int return codes, empty arrays, silent no-ops) with a consistent exception-based contract across Module, LlmModule, and LlmCallback: - Module: all public methods acquire mLock and check destroyed state; loadMethod() throws ExecutorchRuntimeException on native error; execute()/forward() throw IllegalStateException on destroyed module; destroy() throws if lock unavailable (concurrent execution) - LlmModule: all generate/load/prefill methods check destroyed state via volatile flag and throw on native errors; resetNative() deprecated in favor of future close(); stop() intentionally unguarded for interrupt-during-generate; prefill methods return void instead of long - LlmCallback: onError() default logs via android.util.Log with try/catch fallback for JVM unit test environments - ExecutorchRuntimeException: added ALREADY_LOADED (0x04) error code, javadoc on all 19 error codes, "ExecuTorch" casing in error messages - JNI: renamed registrations to match Java native method names (generateNative, loadNative, resetContextNative); removed double exception throw from C++ load(); unknown input typeCode now throws - Tests: updated for void returns and assertThrows; all @ignore preserved - Benchmark: ModelRunner and LlmModelRunner adapted to try/catch pattern Breaking change under @experimental — all APIs are still experimental. Co-authored-by: Claude <noreply@anthropic.com>
2b96224 to
65e793b
Compare
There was a problem hiding this comment.
Pull request overview
This PR standardizes Android-side error reporting by shifting ExecuTorch Java APIs from mixed return-code patterns to an exception-based contract, and aligning JNI registrations with the updated native method names.
Changes:
- Updated
ModuleandLlmModulepublic APIs to throw on errors/destroyed-state instead of returning status codes or empty results. - Renamed/adjusted JNI native method registrations to match updated Java native method names and added stricter input-type validation.
- Adapted Android benchmark and instrumentation tests to the new void/exception-based APIs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| extension/benchmark/android/benchmark/app/src/main/java/org/pytorch/minibench/ModelRunner.java | Adapts benchmarking to exception-based loadMethod() while preserving a numeric status metric. |
| extension/benchmark/android/benchmark/app/src/main/java/org/pytorch/minibench/LlmModelRunner.java | Converts load() to try/catch error-code extraction and logs generation failures. |
| extension/android/jni/jni_layer.cpp | Throws on unsupported input EValue type codes; renames registered natives for etdump/getMethods. |
| extension/android/jni/jni_layer_llama.cpp | Simplifies load() to return error codes only; renames registered natives to *Native variants. |
| extension/android/executorch_android/src/main/java/org/pytorch/executorch/Module.java | Enforces locking + destroyed checks across public methods; converts loadMethod() to void that throws on native errors; wraps getMethods/etdump. |
| extension/android/executorch_android/src/main/java/org/pytorch/executorch/extension/llm/LlmModule.java | Adds destroyed-state guard; converts generate/load/prefill methods to throw exceptions and return void. |
| extension/android/executorch_android/src/main/java/org/pytorch/executorch/extension/llm/LlmCallback.java | Adds a default onError() implementation that logs to Logcat with a fallback path. |
| extension/android/executorch_android/src/main/java/org/pytorch/executorch/ExecutorchRuntimeException.java | Adds ALREADY_LOADED, expands error-code Javadoc, and standardizes “ExecuTorch” casing in formatted messages. |
| extension/android/executorch_android/src/androidTest/java/org/pytorch/executorch/ModuleInstrumentationTest.kt | Updates tests for exception-based loadMethod() and destroyed-module behavior. |
| extension/android/executorch_android/src/androidTest/java/org/pytorch/executorch/LlmModuleInstrumentationTest.kt | Updates LLM instrumentation test to the new void load() API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...droid/executorch_android/src/main/java/org/pytorch/executorch/extension/llm/LlmCallback.java
Outdated
Show resolved
Hide resolved
.../executorch_android/src/androidTest/java/org/pytorch/executorch/ModuleInstrumentationTest.kt
Outdated
Show resolved
Hide resolved
.../executorch_android/src/androidTest/java/org/pytorch/executorch/ModuleInstrumentationTest.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…pytorch/executorch/ModuleInstrumentationTest.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…pytorch/executorch/ModuleInstrumentationTest.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…/executorch/extension/llm/LlmCallback.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
extension/android/executorch_android/src/main/java/org/pytorch/executorch/extension/llm/LlmModule.java:276
- These native methods are registered by name via fbjni. Without
@DoNotStrip(or an explicit consumer ProGuard/R8 keep rule), their names may be obfuscated, causing JNI registration to fail at runtime. Please annotate generateNative() (and other registered native methods) with@DoNotStripor add appropriate keep rules for native members.
int numEos) {
checkNotDestroyed();
int err = generateNative(prompt, seqLen, llmCallback, echo, temperature, numBos, numEos);
if (err != 0) {
throw ExecutorchRuntimeException.makeExecutorchException(err, "Failed to generate");
}
}
private native int generateNative(
String prompt,
int seqLen,
LlmCallback llmCallback,
boolean echo,
extension/android/executorch_android/src/androidTest/java/org/pytorch/executorch/ModuleInstrumentationTest.kt:117
- After Module.destroy(), loadMethod() throws IllegalStateException. This assertion uses the broad RuntimeException type, which could hide regressions (e.g., wrong exception type). Prefer asserting IllegalStateException specifically here.
module.loadMethod(FORWARD_METHOD)
}
@Test
@Throws(IOException::class)
fun testLoadOnDestroyedModule() {
val module = Module.load(getTestFilePath(TEST_FILE_NAME))
extension/benchmark/android/benchmark/app/src/main/java/org/pytorch/minibench/ModelRunner.java:43
- If loadMethod("forward") throws, this code records an errorCode but then still runs warmup/benchmark forward() calls and etdump(). With the new exception-based contract, those calls can also throw and abort the benchmark, preventing metrics from being recorded. Consider returning early (or wrapping forward()/etdump() in try/catch and reporting an appropriate load_status) when preloading fails.
Module module = Module.load(model.getPath());
int errorCode = 0;
try {
module.loadMethod("forward");
} catch (Exception e) {
errorCode =
(e instanceof org.pytorch.executorch.ExecutorchRuntimeException)
? ((org.pytorch.executorch.ExecutorchRuntimeException) e).getErrorCode()
: -1;
}
long loadEnd = System.nanoTime();
for (int i = 0; i < numWarmupIter; i++) {
module.forward();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| default void onError(int errorCode, String message) { | ||
| try { | ||
| android.util.Log.e("ExecuTorch", "LLM error " + errorCode + ": " + message); | ||
| } catch (Throwable t) { | ||
| System.err.println("ExecuTorch LLM error " + errorCode + ": " + message); | ||
| } |
There was a problem hiding this comment.
LlmCallback.onError() intends to fall back in JVM unit test environments, but catching RuntimeException won’t catch NoClassDefFoundError/LinkageError thrown when android.util.Log isn’t on the classpath. Catch Throwable (or at least Exception + LinkageError) so the System.err fallback reliably runs off-Android.
| checkNotDestroyed(); | ||
| resetContextNative(); | ||
| } | ||
|
|
There was a problem hiding this comment.
resetContextNative() is registered from JNI by method name; without @DoNotStrip or keep rules, R8 can rename it and break native registration. Add @DoNotStrip (and consider applying consistently to all JNI-registered native members in this class).
| @DoNotStrip |
| throw ExecutorchRuntimeException.makeExecutorchException(err, "Failed to load model"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
loadNative() is registered from JNI by method name; without @DoNotStrip or keep rules, R8 can rename it and break native registration. Add @DoNotStrip (and consider applying consistently to all JNI-registered native members in this class).
| @DoNotStrip |
| @Test | ||
| @Throws(IOException::class) | ||
| fun testModuleLoadMethodNonExistantMethod() { | ||
| val module = Module.load(getTestFilePath(TEST_FILE_NAME)) | ||
|
|
||
| val loadMethod = module.loadMethod(NONE_METHOD) | ||
| Assert.assertEquals(loadMethod.toLong(), INVALID_ARGUMENT.toLong()) | ||
| val exception = | ||
| Assert.assertThrows(ExecutorchRuntimeException::class.java) { |
There was a problem hiding this comment.
This test currently asserts only that some RuntimeException is thrown. Since Module.loadMethod() now throws a specific ExecutorchRuntimeException on native load failures, assert that type (and ideally the expected errorCode) to ensure the exception contract stays correct.
| Assert.assertThrows(IllegalStateException::class.java) { module.loadMethod(FORWARD_METHOD) } | ||
| } | ||
|
|
||
| @Test | ||
| @Throws(IOException::class) | ||
| fun testForwardOnDestroyedModule() { | ||
| val module = Module.load(getTestFilePath(TEST_FILE_NAME)) | ||
|
|
||
| val loadMethod = module.loadMethod(FORWARD_METHOD) | ||
| Assert.assertEquals(loadMethod.toLong(), OK.toLong()) | ||
| module.loadMethod(FORWARD_METHOD) |
There was a problem hiding this comment.
After destroy(), forward()/execute() now throw IllegalStateException. This assertion uses the broad RuntimeException type, which could hide regressions. Prefer asserting IllegalStateException specifically (and optionally validate the message).
…ailures
Replace the mixed error-reporting pattern (int return codes, empty arrays, silent no-ops) with a consistent exception-based contract across Module, LlmModule, and LlmCallback:
Module: all public methods acquire mLock and check destroyed state; loadMethod() throws ExecutorchRuntimeException on native error; execute()/forward() throw IllegalStateException on destroyed module; destroy() throws if lock unavailable (concurrent execution)
LlmModule: all generate/load/prefill methods check destroyed state via volatile flag and throw on native errors; resetNative() deprecated in favor of future close(); stop() intentionally unguarded for interrupt-during-generate; prefill methods return void instead of long
LlmCallback: onError() default logs via android.util.Log with try/catch fallback for JVM unit test environments
ExecutorchRuntimeException: added ALREADY_LOADED (0x04) error code, javadoc on all 19 error codes, "ExecuTorch" casing in error messages
JNI: renamed registrations to match Java native method names (generateNative, loadNative, resetContextNative); removed double exception throw from C++ load(); unknown input typeCode now throws
Tests: updated for void returns and assertThrows; all @ignore preserved
Benchmark: ModelRunner and LlmModelRunner adapted to try/catch pattern
All APIs are still experimental.