Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ class LlmModuleInstrumentationTest : LlmCallback {
@Test
@Throws(IOException::class, URISyntaxException::class)
fun testGenerate() {
llmModule.load()
val loadResult = llmModule.load()
// Check that the model can be load successfully
assertEquals(OK.toLong(), loadResult.toLong())

llmModule.generate(TEST_PROMPT, SEQ_LEN, this@LlmModuleInstrumentationTest)
assertEquals(results.size.toLong(), SEQ_LEN.toLong())
Expand Down Expand Up @@ -275,6 +277,7 @@ class LlmModuleInstrumentationTest : LlmCallback {
private const val TEST_FILE_NAME = "/stories.pte"
private const val TOKENIZER_FILE_NAME = "/tokenizer.bin"
private const val TEST_PROMPT = "Hello"
private const val OK = 0x00
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test introduces a local OK constant (0x00). To keep tests aligned with the runtime’s source of truth and avoid drift, prefer using ExecutorchRuntimeException.OK (or a shared constant) instead of duplicating the numeric value here.

Suggested change
private const val OK = 0x00

Copilot uses AI. Check for mistakes.
private const val SEQ_LEN = 32
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ class ModuleInstrumentationTest {
fun testModuleLoadMethodAndForward() {
val module = Module.load(getTestFilePath(TEST_FILE_NAME))

module.loadMethod(FORWARD_METHOD)
val loadMethod = module.loadMethod(FORWARD_METHOD)
Assert.assertEquals(loadMethod.toLong(), OK.toLong())

val results = module.forward()
Assert.assertTrue(results[0].isTensor)
Expand Down Expand Up @@ -95,22 +96,17 @@ class ModuleInstrumentationTest {
fun testModuleLoadMethodNonExistantMethod() {
val module = Module.load(getTestFilePath(TEST_FILE_NAME))

val exception =
Assert.assertThrows(ExecutorchRuntimeException::class.java) {
module.loadMethod(NONE_METHOD)
}
Assert.assertEquals(
ExecutorchRuntimeException.INVALID_ARGUMENT,
exception.getErrorCode(),
)
val loadMethod = module.loadMethod(NONE_METHOD)
Assert.assertEquals(loadMethod.toLong(), INVALID_ARGUMENT.toLong())
}

@Test(expected = RuntimeException::class)
@Throws(IOException::class)
fun testNonPteFile() {
val module = Module.load(getTestFilePath(NON_PTE_FILE_NAME))

module.loadMethod(FORWARD_METHOD)
val loadMethod = module.loadMethod(FORWARD_METHOD)
Assert.assertEquals(loadMethod.toLong(), INVALID_ARGUMENT.toLong())
}
Comment on lines 103 to 110
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testNonPteFile is still annotated with @test(expected = RuntimeException::class), but the test body now asserts that loadMethod() returns INVALID_ARGUMENT rather than throwing. Unless Module.load() is expected to throw before reaching the assertions, this test will fail. Update the test to either assert the returned error code (and remove the expected-exception annotation) or keep exception-based assertions consistently.

Copilot uses AI. Check for mistakes.

@Test
Expand All @@ -120,19 +116,22 @@ class ModuleInstrumentationTest {

module.destroy()

Assert.assertThrows(IllegalStateException::class.java) { module.loadMethod(FORWARD_METHOD) }
val loadMethod = module.loadMethod(FORWARD_METHOD)
Assert.assertEquals(loadMethod.toLong(), INVALID_STATE.toLong())
}

@Test
@Throws(IOException::class)
fun testForwardOnDestroyedModule() {
val module = Module.load(getTestFilePath(TEST_FILE_NAME))

module.loadMethod(FORWARD_METHOD)
val loadMethod = module.loadMethod(FORWARD_METHOD)
Assert.assertEquals(loadMethod.toLong(), OK.toLong())

module.destroy()

Assert.assertThrows(IllegalStateException::class.java) { module.forward() }
val results = module.forward()
Assert.assertEquals(0, results.size.toLong())
}

@Ignore(
Expand Down Expand Up @@ -176,5 +175,9 @@ class ModuleInstrumentationTest {
private const val NON_PTE_FILE_NAME = "/test.txt"
private const val FORWARD_METHOD = "forward"
private const val NONE_METHOD = "none"
private const val OK = 0x00
private const val INVALID_STATE = 0x2
private const val INVALID_ARGUMENT = 0x12
private const val ACCESS_FAILED = 0x22
Comment on lines +178 to +181
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests introduce local copies of runtime error-code constants (OK/INVALID_STATE/INVALID_ARGUMENT/ACCESS_FAILED). To avoid drift from the source of truth (ExecutorchRuntimeException and runtime/core/error.h), prefer referencing ExecutorchRuntimeException.* constants directly in assertions.

Copilot uses AI. Check for mistakes.
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,83 +13,34 @@
import java.util.HashMap;
import java.util.Map;

/**
* Base exception for all ExecuTorch runtime errors. Each instance carries an integer error code
* corresponding to the native {@code runtime/core/error.h} values, accessible via {@link
* #getErrorCode()}.
*/
public class ExecutorchRuntimeException extends RuntimeException {
// Error code constants - keep in sync with runtime/core/error.h

// System errors

/** Operation completed successfully. */
public static final int OK = 0x00;

/** An unexpected internal error occurred in the runtime. */
public static final int INTERNAL = 0x01;

/** The runtime or method is in an invalid state for the requested operation. */
public static final int INVALID_STATE = 0x02;

/** The method has finished execution and has no more work to do. */
public static final int END_OF_METHOD = 0x03;

Comment on lines 17 to 23
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExecutorchRuntimeException claims to keep error codes in sync with runtime/core/error.h, but the AlreadyLoaded (0x04) constant/message mapping was removed while it still exists in runtime/core/error.h. This can cause mismatched error reporting for callers. Re-add the missing constant and include it in ERROR_CODE_MESSAGES.

Copilot uses AI. Check for mistakes.
/** A required resource has already been loaded. */
public static final int ALREADY_LOADED = 0x04;

// Logical errors

/** The requested operation is not supported by this build or backend. */
public static final int NOT_SUPPORTED = 0x10;

/** The requested operation has not been implemented. */
public static final int NOT_IMPLEMENTED = 0x11;

/** One or more arguments passed to the operation are invalid. */
public static final int INVALID_ARGUMENT = 0x12;

/** A value or tensor has an unexpected type. */
public static final int INVALID_TYPE = 0x13;

/** A required operator kernel is not registered. */
public static final int OPERATOR_MISSING = 0x14;

/** The maximum number of registered kernels has been exceeded. */
public static final int REGISTRATION_EXCEEDING_MAX_KERNELS = 0x15;

/** A kernel with the same name is already registered. */
public static final int REGISTRATION_ALREADY_REGISTERED = 0x16;

// Resource errors

/** A required resource (file, tensor, program) was not found. */
public static final int NOT_FOUND = 0x20;

/** A memory allocation failed. */
public static final int MEMORY_ALLOCATION_FAILED = 0x21;

/** Access to a resource was denied or failed. */
public static final int ACCESS_FAILED = 0x22;

/** The loaded program is malformed or incompatible. */
public static final int INVALID_PROGRAM = 0x23;

/** External data referenced by the program is invalid or missing. */
public static final int INVALID_EXTERNAL_DATA = 0x24;

/** The system has run out of a required resource. */
public static final int OUT_OF_RESOURCES = 0x25;

// Delegate errors

/** A delegate reported an incompatible model or configuration. */
public static final int DELEGATE_INVALID_COMPATIBILITY = 0x30;

/** A delegate failed to allocate required memory. */
public static final int DELEGATE_MEMORY_ALLOCATION_FAILED = 0x31;

/** A delegate received an invalid or stale handle. */
public static final int DELEGATE_INVALID_HANDLE = 0x32;

private static final Map<Integer, String> ERROR_CODE_MESSAGES;
Expand All @@ -102,7 +53,6 @@ public class ExecutorchRuntimeException extends RuntimeException {
map.put(INTERNAL, "Internal error");
map.put(INVALID_STATE, "Invalid state");
map.put(END_OF_METHOD, "End of method reached");
map.put(ALREADY_LOADED, "Already loaded");
// Logical errors
map.put(NOT_SUPPORTED, "Operation not supported");
map.put(NOT_IMPLEMENTED, "Operation not implemented");
Expand Down Expand Up @@ -134,7 +84,7 @@ static String formatMessage(int errorCode, String details) {

String safeDetails = details != null ? details : "No details provided";
return String.format(
"[ExecuTorch Error 0x%s] %s: %s",
"[Executorch Error 0x%s] %s: %s",
Integer.toHexString(errorCode), baseMessage, safeDetails);
}

Expand Down Expand Up @@ -163,12 +113,10 @@ public ExecutorchRuntimeException(int errorCode, String details) {
this.errorCode = errorCode;
}

/** Returns the numeric error code from {@code runtime/core/error.h}. */
public int getErrorCode() {
return errorCode;
}

/** Returns detailed log output captured from the native runtime, if available. */
public String getDetailedError() {
return ErrorHelper.getDetailedErrorLogs();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

package org.pytorch.executorch;

import android.util.Log;
import com.facebook.jni.HybridData;
import com.facebook.jni.annotations.DoNotStrip;
import com.facebook.soloader.nativeloader.NativeLoader;
Expand Down Expand Up @@ -129,10 +130,11 @@ public EValue[] forward(EValue... inputs) {
* @return return value from the method.
*/
public EValue[] execute(String methodName, EValue... inputs) {
mLock.lock();
try {
mLock.lock();
if (!mHybridData.isValid()) {
throw new IllegalStateException("Module has been destroyed");
Log.e("ExecuTorch", "Attempt to use a destroyed module");
return new EValue[0];
}
return executeNative(methodName, inputs);
} finally {
Expand All @@ -149,17 +151,17 @@ public EValue[] execute(String methodName, EValue... inputs) {
* synchronous, and will block until the method is loaded. Therefore, it is recommended to call
* this on a background thread. However, users need to make sure that they don't execute before
* this function returns.
*
* @return the Error code if there was an error loading the method
*/
public void loadMethod(String methodName) {
mLock.lock();
public int loadMethod(String methodName) {
try {
mLock.lock();
if (!mHybridData.isValid()) {
throw new IllegalStateException("Module has been destroyed");
}
int errorCode = loadMethodNative(methodName);
if (errorCode != 0) {
throw new ExecutorchRuntimeException(errorCode, "Failed to load method: " + methodName);
Log.e("ExecuTorch", "Attempt to use a destroyed module");
return 0x2; // InvalidState
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadMethod() returns a hard-coded 0x2 for the destroyed-module case. Since this class already depends on ExecutorchRuntimeException’s error-code constants (and the comment says “keep in sync with runtime/core/error.h”), prefer using a named constant (e.g., ExecutorchRuntimeException.INVALID_STATE) to avoid magic numbers drifting from native definitions.

Suggested change
return 0x2; // InvalidState
return ExecutorchRuntimeException.INVALID_STATE;

Copilot uses AI. Check for mistakes.
}
return loadMethodNative(methodName);
} finally {
mLock.unlock();
}
Expand All @@ -182,20 +184,8 @@ public void loadMethod(String methodName) {
*
* @return name of methods in this Module
*/
public String[] getMethods() {
mLock.lock();
try {
if (!mHybridData.isValid()) {
throw new IllegalStateException("Module has been destroyed");
}
return getMethodsNative();
} finally {
mLock.unlock();
}
}

@DoNotStrip
private native String[] getMethodsNative();
public native String[] getMethods();
Comment on lines 187 to +188
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getMethods() is now a direct native call with no mLock/mHybridData.isValid() guard. This breaks the class’s existing thread-safety + destroyed-object pattern (execute/loadMethod still lock and check isValid), and it can call into freed native state after destroy(). Consider restoring a Java wrapper that locks + checks isValid (returning an empty array on destroyed), and keep the native method private (e.g., getMethodsNative).

Copilot uses AI. Check for mistakes.

/**
* Get the corresponding @MethodMetadata for a method
Expand All @@ -204,19 +194,11 @@ public String[] getMethods() {
* @return @MethodMetadata for this method
*/
public MethodMetadata getMethodMetadata(String name) {
mLock.lock();
try {
if (!mHybridData.isValid()) {
throw new IllegalStateException("Module has been destroyed");
}
MethodMetadata methodMetadata = mMethodMetadata.get(name);
if (methodMetadata == null) {
throw new IllegalArgumentException("method " + name + " does not exist for this module");
}
return methodMetadata;
} finally {
mLock.unlock();
MethodMetadata methodMetadata = mMethodMetadata.get(name);
if (methodMetadata == null) {
throw new IllegalArgumentException("method " + name + " does not exist for this module");
}
return methodMetadata;
}

@DoNotStrip
Expand All @@ -228,15 +210,7 @@ public static String[] readLogBufferStatic() {

/** Retrieve the in-memory log buffer, containing the most recent ExecuTorch log entries. */
public String[] readLogBuffer() {
mLock.lock();
try {
if (!mHybridData.isValid()) {
throw new IllegalStateException("Module has been destroyed");
}
return readLogBufferNative();
} finally {
mLock.unlock();
}
return readLogBufferNative();
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readLogBuffer() now forwards directly to readLogBufferNative() without any mHybridData.isValid() check. After destroy(), this risks invoking JNI on an invalid HybridData/native pointer (potential crash). Please add the same isValid guard used by execute/loadMethod (return an empty array or similar when destroyed) and consider also guarding with mLock if the native side isn’t thread-safe.

Suggested change
return readLogBufferNative();
mLock.lock();
try {
if (!mHybridData.isValid()) {
return new String[0];
}
return readLogBufferNative();
} finally {
mLock.unlock();
}

Copilot uses AI. Check for mistakes.
}

@DoNotStrip
Expand All @@ -250,20 +224,8 @@ public String[] readLogBuffer() {
* @return true if the etdump was successfully written, false otherwise.
*/
@Experimental
public boolean etdump() {
mLock.lock();
try {
if (!mHybridData.isValid()) {
throw new IllegalStateException("Module has been destroyed");
}
return etdumpNative();
} finally {
mLock.unlock();
}
}

@DoNotStrip
private native boolean etdumpNative();
public native boolean etdump();
Comment on lines 226 to +228
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

etdump() is now a direct native method without an isValid check or locking. Calling this after destroy() (or concurrently with execute()) can race with native teardown and lead to crashes or corrupted dumps. Wrap the call in a Java method that acquires mLock and checks mHybridData.isValid(), returning false when destroyed.

Copilot uses AI. Check for mistakes.

/**
* Explicitly destroys the native Module object. Calling this method is not required, as the
Expand All @@ -279,7 +241,10 @@ public void destroy() {
mLock.unlock();
}
} else {
throw new IllegalStateException("Cannot destroy module while method is executing");
Log.w(
"ExecuTorch",
"Destroy was called while the module was in use. Resources will not be immediately"
+ " released.");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,5 @@ default void onStats(String stats) {}
* @param message Human-readable error description
*/
@DoNotStrip
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);
}
}
default void onError(int errorCode, String message) {}
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LlmCallback.onError’s default implementation was changed from logging to a no-op. If callers don’t override onError, runtime failures will become silent, making it hard to diagnose issues in production/benchmarks. Consider keeping a minimal default log (or at least documenting that implementations should override onError) to preserve baseline observability.

Copilot uses AI. Check for mistakes.
}
Loading
Loading