Android: consistent error types across all modules#19099
Android: consistent error types across all modules#19099psiddh merged 10 commits intopytorch:mainfrom
Conversation
TrainingModule: implement Closeable, replace Log.e + silent empty returns with IllegalStateException throws. Add checkNotDestroyed() guard on all public methods. SGD: throw IllegalStateException instead of bare RuntimeException when optimizer is destroyed. AsrModule: throw ExecutorchRuntimeException instead of bare RuntimeException on transcription failure. ExecuTorchRuntime.validateFilePath: throw IllegalArgumentException instead of bare RuntimeException, with descriptive message. JNI constructors: wrap ExecuTorchJni and ExecuTorchLlmJni constructor bodies in try-catch so C++ exceptions become ExecutorchRuntimeException instead of generic RuntimeException. This commit was authored with the help of Claude.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19099
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ✅ You can merge normally! (4 Unrelated Failures)As of commit 1d00327 with merge base cb94506 ( FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
Native creation failure now throws ExecutorchRuntimeException with INTERNAL error code, consistent with all other modules. This commit was authored with the help of Claude.
There was a problem hiding this comment.
Pull request overview
Aligns Android-side error handling by replacing generic/implicit failures with consistent, typed exceptions across Java/Kotlin and JNI layers.
Changes:
- Wrap JNI HybridClass constructors to convert C++ exceptions into
ExecutorchRuntimeException. - Update training APIs to implement
Closeable, add destroyed-guards, and replace silent failures/logging withIllegalStateException. - Standardize exception types/messages across modules (
IllegalStateException,IllegalArgumentException,ExecutorchRuntimeException).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| extension/android/jni/jni_layer_llama.cpp | Wrap LLM JNI constructor in try/catch and translate failures to ExecutorchRuntimeException. |
| extension/android/jni/jni_layer.cpp | Wrap Module JNI constructor in try/catch and translate failures to ExecutorchRuntimeException. |
| extension/android/executorch_android/src/main/java/org/pytorch/executorch/training/TrainingModule.java | Implement Closeable, add destroyed guard, and replace log+empty-return with exceptions. |
| extension/android/executorch_android/src/main/java/org/pytorch/executorch/training/SGD.java | Throw IllegalStateException when optimizer is destroyed. |
| extension/android/executorch_android/src/main/java/org/pytorch/executorch/extension/asr/AsrModule.kt | Throw ExecutorchRuntimeException on transcription failure. |
| extension/android/executorch_android/src/main/java/org/pytorch/executorch/ExecuTorchRuntime.java | Throw IllegalArgumentException with more descriptive message on invalid file paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix clang-format indentation in jni_layer_llama.cpp constructor - Add throw; after throwExecutorchException in both JNI constructors to ensure construction fails deterministically - Use import for ExecutorchRuntimeException in AsrModule.kt - Fix validateFilePath Javadoc to document IllegalArgumentException - Split validateFilePath into specific error messages (does not exist, is not a file, is not readable) This commit was authored with the help of Claude.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JNI constructors now catch all native exceptions, not just std::exception. Fix redundant fully-qualified ExecutorchRuntimeException.INTERNAL in AsrModule.kt. This commit was authored with the help of Claude.
This commit was authored with the help of Claude.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This commit was authored with the help of Claude.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JniException takes a jthrowable, not a string. throwExecutorchException already throws via fbjni at the JNI boundary — no need to rethrow. This commit was authored with the help of Claude.
## Summary Reverts the following Android PRs: - #19099 — Android: consistent error types across all modules - #19124 — Android: Module implements Closeable - #19092 — Android: improve error diagnostics for LlmModule and exceptions - #19028 — Ignored Module tests: provide required input tensor Authored with Claude.
TrainingModule: implement Closeable, replace Log.e + silent empty returns with IllegalStateException throws. Add checkNotDestroyed() guard on all public methods.
SGD: throw IllegalStateException instead of bare RuntimeException when optimizer is destroyed.
AsrModule: throw ExecutorchRuntimeException instead of bare RuntimeException on transcription failure.
ExecuTorchRuntime.validateFilePath: throw IllegalArgumentException instead of bare RuntimeException, with descriptive message.
JNI constructors: wrap ExecuTorchJni and ExecuTorchLlmJni constructor bodies in try-catch so C++ exceptions become ExecutorchRuntimeException instead of generic RuntimeException.
This commit was authored with the help of Claude.